Reland "Move code from base::ByteSwap to base::numerics and fix UB casts"

This is a reland of commit 49599d8641

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}

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 @@ void SHA1Context::Final() {
   Process();
 
   for (auto& t : H) {
-    t = ByteSwap(t);
+    t = numerics::ByteSwap(t);
   }
 }
 
@@ -128,7 +128,7 @@ void SHA1Context::Process() {
   // 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.

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) {

base/sys_byteorder_unittest.cc

@@ -20,46 +20,6 @@ const uint64_t k64BitSwappedTestData = 0x11223344ddccbbaa;
 
 }  // 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)

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 @@ DWORD WINAPI GetFontDataPatch(HDC dc_handle,
   // 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)

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 @@ class MEDIA_EXPORT PassthroughDTSAudioDecoder : public AudioDecoder {
 
   AudioDecoderConfig config_;
 
-  MediaLog* media_log_;
+  raw_ptr<MediaLog> media_log_;
 
   scoped_refptr<AudioBufferMemoryPool> pool_;
 };

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 @@ constexpr size_t kDTSXP2SamplesPerFrame = 1024;
 
 }  // 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 @@ int WrapDTSWithIEC61937(base::span<const uint8_t> input_data_s,
     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;
   }

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 @@ uint32_t MaybeConvertABGRToARGB(uint32_t abgr) {
     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
 }