Revert "Move code from base::ByteSwap to base::numerics and fix UB casts"
This reverts commit 49599d86419c742d12d118e12bf30b79f64c69b0.
Reason for revert: cause tree closure https://ci.chromium.org/ui/p/chromium/builders/ci/mac-arm64-on-arm64-rel/101537/overview
Original change's description:
> Move code from base::ByteSwap to base::numerics and fix UB casts
>
> While moving ByteSwap() callers, I found the DTS code doing byte swaps
> by casting byte spans to uint16_t spans, which causes UB if they are
> not aligned. The function explicitly handles non-16-bit aligned inputs
> earlier, so clearly it's expected and we should not CHECK on the
> alignment. So instead, change the code to work with bytes and do the
> swaps in a for loop.
>
> While here, make use of the new split_at() to split the output span
> up into disjoint subspans that are each written to.
>
> R=dalecurtis@chromium.org
>
> Bug: 40284755
> Change-Id: I11c557fd3e0f6fd64555b299f3492ca7722aeae2
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5366084
> Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
> Owners-Override: danakj <danakj@chromium.org>
> Commit-Queue: danakj <danakj@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1271821}
Bug: 40284755
Change-Id: Id0991e5586fcb2d23dc1ee8c411c23a8b1c37570
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5366353
Auto-Submit: Phillis Tang <phillis@chromium.org>
Owners-Override: Phillis Tang <phillis@chromium.org>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1271825}
diff --git a/base/hash/sha1_nacl.cc b/base/hash/sha1_nacl.cc
index 8ebeb0993..4e9f0d5 100644
--- a/base/hash/sha1_nacl.cc
+++ b/base/hash/sha1_nacl.cc
@@ -9,7 +9,7 @@
#include <string_view>
#include "base/hash/sha1.h"
-#include "base/numerics/byte_conversions.h"
+#include "base/sys_byteorder.h"
namespace base {
// Implementation of SHA-1. Only handles data in byte-sized blocks,
@@ -84,7 +84,7 @@
Process();
for (auto& t : H) {
- t = numerics::ByteSwap(t);
+ t = ByteSwap(t);
}
}
@@ -128,7 +128,7 @@
// W and M are in a union, so no need to memcpy.
// memcpy(W, M, sizeof(M));
for (t = 0; t < 16; ++t) {
- W[t] = numerics::ByteSwap(W[t]);
+ W[t] = ByteSwap(W[t]);
}
// b.
diff --git a/base/sys_byteorder.h b/base/sys_byteorder.h
index 02de1f4e..f3ec517 100644
--- a/base/sys_byteorder.h
+++ b/base/sys_byteorder.h
@@ -22,6 +22,28 @@
namespace base {
+// Returns a value with all bytes in |x| swapped, i.e. reverses the endianness.
+// TODO(danakj): Replace with base::numerics::byteswap().
+inline constexpr uint16_t ByteSwap(uint16_t x) {
+ // Forward to templated function in //base/numerics.
+ return numerics::ByteSwap(x);
+}
+
+inline constexpr uint32_t ByteSwap(uint32_t x) {
+ // Forward to templated function in //base/numerics.
+ return numerics::ByteSwap(x);
+}
+
+inline constexpr uint64_t ByteSwap(uint64_t x) {
+ // Forward to templated function in //base/numerics.
+ return numerics::ByteSwap(x);
+}
+
+inline constexpr uintptr_t ByteSwapUintPtrT(uintptr_t x) {
+ // Forward to templated function in //base/numerics.
+ return numerics::ByteSwap(x);
+}
+
// Converts the bytes in |x| from host order (endianness) to little endian, and
// returns the result.
inline constexpr uint16_t ByteSwapToLE16(uint16_t x) {
diff --git a/base/sys_byteorder_unittest.cc b/base/sys_byteorder_unittest.cc
index fe9bf26..7b68d2c0 100644
--- a/base/sys_byteorder_unittest.cc
+++ b/base/sys_byteorder_unittest.cc
@@ -20,6 +20,46 @@
} // namespace
+TEST(ByteOrderTest, ByteSwap16) {
+ uint16_t swapped = base::ByteSwap(k16BitTestData);
+ EXPECT_EQ(k16BitSwappedTestData, swapped);
+ uint16_t reswapped = base::ByteSwap(swapped);
+ EXPECT_EQ(k16BitTestData, reswapped);
+}
+
+TEST(ByteOrderTest, ByteSwap32) {
+ uint32_t swapped = base::ByteSwap(k32BitTestData);
+ EXPECT_EQ(k32BitSwappedTestData, swapped);
+ uint32_t reswapped = base::ByteSwap(swapped);
+ EXPECT_EQ(k32BitTestData, reswapped);
+}
+
+TEST(ByteOrderTest, ByteSwap64) {
+ uint64_t swapped = base::ByteSwap(k64BitTestData);
+ EXPECT_EQ(k64BitSwappedTestData, swapped);
+ uint64_t reswapped = base::ByteSwap(swapped);
+ EXPECT_EQ(k64BitTestData, reswapped);
+}
+
+TEST(ByteOrderTest, ByteSwapUintPtrT) {
+#if defined(ARCH_CPU_64_BITS)
+ const uintptr_t test_data = static_cast<uintptr_t>(k64BitTestData);
+ const uintptr_t swapped_test_data =
+ static_cast<uintptr_t>(k64BitSwappedTestData);
+#elif defined(ARCH_CPU_32_BITS)
+ const uintptr_t test_data = static_cast<uintptr_t>(k32BitTestData);
+ const uintptr_t swapped_test_data =
+ static_cast<uintptr_t>(k32BitSwappedTestData);
+#else
+#error architecture not supported
+#endif
+
+ uintptr_t swapped = base::ByteSwapUintPtrT(test_data);
+ EXPECT_EQ(swapped_test_data, swapped);
+ uintptr_t reswapped = base::ByteSwapUintPtrT(swapped);
+ EXPECT_EQ(test_data, reswapped);
+}
+
TEST(ByteOrderTest, ByteSwapToLE16) {
uint16_t le = base::ByteSwapToLE16(k16BitTestData);
#if defined(ARCH_CPU_LITTLE_ENDIAN)
diff --git a/content/child/font_warmup_win.cc b/content/child/font_warmup_win.cc
index eaeb0bf..d71bd63 100644
--- a/content/child/font_warmup_win.cc
+++ b/content/child/font_warmup_win.cc
@@ -17,11 +17,11 @@
#include "base/memory/raw_ptr.h"
#include "base/memory/ref_counted.h"
#include "base/no_destructor.h"
-#include "base/numerics/byte_conversions.h"
#include "base/numerics/safe_conversions.h"
#include "base/numerics/safe_math.h"
#include "base/strings/utf_string_conversions.h"
#include "base/synchronization/lock.h"
+#include "base/sys_byteorder.h"
#include "base/trace_event/trace_event.h"
#include "base/win/iat_patch_function.h"
#include "build/build_config.h"
@@ -295,7 +295,7 @@
// the correct answer for emulating GDI. |table_tag| must also have its
// byte order swapped to counter the swap which occurs in the called method.
size_t length = typeface->getTableData(
- base::numerics::ByteSwap(uint32_t{table_tag}), table_offset,
+ base::ByteSwap(base::strict_cast<uint32_t>(table_tag)), table_offset,
buffer ? buffer_length : INT32_MAX, buffer);
// We can't distinguish between an empty table and an error.
if (length == 0)
diff --git a/media/filters/passthrough_dts_audio_decoder.h b/media/filters/passthrough_dts_audio_decoder.h
index d360258..6cec47a 100644
--- a/media/filters/passthrough_dts_audio_decoder.h
+++ b/media/filters/passthrough_dts_audio_decoder.h
@@ -5,7 +5,6 @@
#ifndef MEDIA_FILTERS_PASSTHROUGH_DTS_AUDIO_DECODER_H_
#define MEDIA_FILTERS_PASSTHROUGH_DTS_AUDIO_DECODER_H_
-#include "base/memory/raw_ptr.h"
#include "media/base/audio_buffer.h"
#include "media/base/audio_decoder.h"
#include "media/base/media_log.h"
@@ -61,7 +60,7 @@
AudioDecoderConfig config_;
- raw_ptr<MediaLog> media_log_;
+ MediaLog* media_log_;
scoped_refptr<AudioBufferMemoryPool> pool_;
};
diff --git a/media/formats/dts/dts_util.cc b/media/formats/dts/dts_util.cc
index febf8f1..fe05ced 100644
--- a/media/formats/dts/dts_util.cc
+++ b/media/formats/dts/dts_util.cc
@@ -4,9 +4,8 @@
#include "media/formats/dts/dts_util.h"
-#include <algorithm>
-
#include "base/logging.h"
+#include "base/sys_byteorder.h"
#include "media/base/audio_parameters.h"
#include "media/base/bit_reader.h"
#include "media/formats/dts/dts_stream_parser.h"
@@ -122,8 +121,8 @@
} // namespace
-int WrapDTSWithIEC61937(base::span<const uint8_t> input,
- base::span<uint8_t> output,
+int WrapDTSWithIEC61937(base::span<const uint8_t> input_data_s,
+ base::span<uint8_t> output_data_s,
AudioCodec dts_codec_type) {
if (dts_codec_type == AudioCodec::kDTS) {
// IEC 61937 frame for DTS-CA (IEC 61937-5) is defined as
@@ -132,36 +131,35 @@
static constexpr uint8_t kDTSCAHeader[] = {0x72, 0xF8, 0x1F, 0x4E,
0x0B, 0x00, 0x00, 0x20};
- // Output bytes: header + data + optional 2-byte alignment.
- size_t output_bytes = sizeof(kDTSCAHeader) + input.size();
+ // Output bytes: header + data + optional 2-byte alignment
+ size_t output_bytes = sizeof(kDTSCAHeader) + input_data_s.size();
if (output_bytes & 1)
output_bytes++;
- // Header + input data must fit in output buffer, limited to one DTS frame.
- if (input.size() > kDTSFrameSize - sizeof(kDTSCAHeader) ||
- output_bytes > output.size()) {
+ // Header + input data must fit in output buffer, limited to one DTS frame
+ if (input_data_s.size() > kDTSFrameSize - sizeof(kDTSCAHeader) ||
+ output_bytes > output_data_s.size()) {
return 0;
}
- // Copy header to output buffer.
- auto [output_header, output_rem] = output.split_at<sizeof(kDTSCAHeader)>();
- output_header.copy_from(kDTSCAHeader);
+ // Copy header to output buffer
+ memcpy(output_data_s.data(), kDTSCAHeader, sizeof(kDTSCAHeader));
- // Perform 16-bit byte swap while copying from input to output. If the input
- // buffer is not even-sized, we drop the last byte.
- //
- // NOTE: This was historically done with a cast to `uint16_t*` however the
- // input is not correctly aligned for that, so the dereference of the
- // pointer would cause UB.
- const size_t byte_pairs = input.size() / 2u;
- auto [output_data, output_padding] = output_rem.split_at(byte_pairs * 2u);
- for (size_t i = 0u; i < byte_pairs; ++i) {
- output_data[2u * i] = input[2u * i + 1u];
- output_data[2u * i + 1u] = input[2u * i];
- }
+ // Use 16-bit span for 16-bit byte swap
+ base::span<const uint16_t> input_16(
+ reinterpret_cast<const uint16_t*>(input_data_s.data()),
+ input_data_s.size() / 2);
+ output_data_s = output_data_s.subspan(sizeof(kDTSCAHeader));
+ base::span<uint16_t> output_16(
+ reinterpret_cast<uint16_t*>(output_data_s.data()),
+ output_data_s.size() / 2);
- // Zero fill the remaining output buffer.
- std::ranges::fill(output_padding, uint8_t{0});
+ auto output_16_iterator = base::ranges::transform(
+ input_16.begin(), input_16.end(), output_16.begin(),
+ [](uint16_t n) -> uint16_t { return base::ByteSwap(n); });
+
+ // Zero fill the remaining output buffer
+ std::fill(output_16_iterator, output_16.end(), 0);
return kDTSFrameSize;
}
diff --git a/media/renderers/paint_canvas_video_renderer_unittest.cc b/media/renderers/paint_canvas_video_renderer_unittest.cc
index 9e5ea70..dec650c 100644
--- a/media/renderers/paint_canvas_video_renderer_unittest.cc
+++ b/media/renderers/paint_canvas_video_renderer_unittest.cc
@@ -9,7 +9,7 @@
#include "base/logging.h"
#include "base/memory/aligned_memory.h"
#include "base/memory/raw_ptr.h"
-#include "base/numerics/byte_conversions.h"
+#include "base/sys_byteorder.h"
#include "base/test/task_environment.h"
#include "build/build_config.h"
#include "cc/paint/paint_flags.h"
@@ -411,8 +411,7 @@
SK_A32_SHIFT == 24
return abgr;
#else
- return (base::numerics::ByteSwap(abgr & 0x00FFFFFF) >> 8) |
- (abgr & 0xFF000000);
+ return (base::ByteSwap(abgr & 0x00FFFFFF) >> 8) | (abgr & 0xFF000000);
#endif
}
