Reland "Move code from base::ByteSwap to base::numerics and fix UB casts"
This is a reland of commit 49599d86419c742d12d118e12bf30b79f64c69b0
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: I6a067c0bee14898006163a5706bff5e9947843f7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5366281
Owners-Override: danakj <danakj@chromium.org>
Commit-Queue: danakj <danakj@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1272244}
diff --git a/base/hash/sha1_nacl.cc b/base/hash/sha1_nacl.cc
index 4e9f0d5..8ebeb0993 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/sys_byteorder.h"
+#include "base/numerics/byte_conversions.h"
namespace base {
// Implementation of SHA-1. Only handles data in byte-sized blocks,
@@ -84,7 +84,7 @@
Process();
for (auto& t : H) {
- t = ByteSwap(t);
+ t = numerics::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] = ByteSwap(W[t]);
+ W[t] = numerics::ByteSwap(W[t]);
}
// b.
diff --git a/base/sys_byteorder.h b/base/sys_byteorder.h
index f3ec517..02de1f4e 100644
--- a/base/sys_byteorder.h
+++ b/base/sys_byteorder.h
@@ -22,28 +22,6 @@
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 7b68d2c0..fe9bf26 100644
--- a/base/sys_byteorder_unittest.cc
+++ b/base/sys_byteorder_unittest.cc
@@ -20,46 +20,6 @@
} // 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 d71bd63..eaeb0bf 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::ByteSwap(base::strict_cast<uint32_t>(table_tag)), table_offset,
+ base::numerics::ByteSwap(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 6cec47a..d360258 100644
--- a/media/filters/passthrough_dts_audio_decoder.h
+++ b/media/filters/passthrough_dts_audio_decoder.h
@@ -5,6 +5,7 @@
#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"
@@ -60,7 +61,7 @@
AudioDecoderConfig config_;
- MediaLog* media_log_;
+ raw_ptr<MediaLog> media_log_;
scoped_refptr<AudioBufferMemoryPool> pool_;
};
diff --git a/media/formats/dts/dts_util.cc b/media/formats/dts/dts_util.cc
index fe05ced..febf8f1 100644
--- a/media/formats/dts/dts_util.cc
+++ b/media/formats/dts/dts_util.cc
@@ -4,8 +4,9 @@
#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"
@@ -121,8 +122,8 @@
} // namespace
-int WrapDTSWithIEC61937(base::span<const uint8_t> input_data_s,
- base::span<uint8_t> output_data_s,
+int WrapDTSWithIEC61937(base::span<const uint8_t> input,
+ base::span<uint8_t> output,
AudioCodec dts_codec_type) {
if (dts_codec_type == AudioCodec::kDTS) {
// IEC 61937 frame for DTS-CA (IEC 61937-5) is defined as
@@ -131,35 +132,36 @@
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_data_s.size();
+ // Output bytes: header + data + optional 2-byte alignment.
+ size_t output_bytes = sizeof(kDTSCAHeader) + input.size();
if (output_bytes & 1)
output_bytes++;
- // 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()) {
+ // Header + input data must fit in output buffer, limited to one DTS frame.
+ if (input.size() > kDTSFrameSize - sizeof(kDTSCAHeader) ||
+ output_bytes > output.size()) {
return 0;
}
- // Copy header to output buffer
- memcpy(output_data_s.data(), kDTSCAHeader, sizeof(kDTSCAHeader));
+ // Copy header to output buffer.
+ auto [output_header, output_rem] = output.split_at<sizeof(kDTSCAHeader)>();
+ output_header.copy_from(kDTSCAHeader);
- // 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);
+ // 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];
+ }
- 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);
+ // Zero fill the remaining output buffer.
+ std::ranges::fill(output_padding, uint8_t{0});
return kDTSFrameSize;
}
diff --git a/media/renderers/paint_canvas_video_renderer_unittest.cc b/media/renderers/paint_canvas_video_renderer_unittest.cc
index dec650c..9e5ea70 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/sys_byteorder.h"
+#include "base/numerics/byte_conversions.h"
#include "base/test/task_environment.h"
#include "build/build_config.h"
#include "cc/paint/paint_flags.h"
@@ -411,7 +411,8 @@
SK_A32_SHIFT == 24
return abgr;
#else
- return (base::ByteSwap(abgr & 0x00FFFFFF) >> 8) | (abgr & 0xFF000000);
+ return (base::numerics::ByteSwap(abgr & 0x00FFFFFF) >> 8) |
+ (abgr & 0xFF000000);
#endif
}
