Get rid of shift UB in big-endian conversion code

Bug: 1124897
Change-Id: Idede30d03e3c0aadf78a9386553d81a638c369c4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2399750
Commit-Queue: Eugene Zemtsov <eugene@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#805235}

base/big_endian.h

@@ -22,12 +22,15 @@ namespace base {
 template<typename T>
 inline void ReadBigEndian(const char buf[], T* out) {
   static_assert(std::is_integral<T>::value, "T has to be an integral type.");
-  *out = buf[0];
+  // Make an unsigned version of the output type to make shift possible
+  // without UB.
+  typename std::make_unsigned<T>::type unsigned_result = uint8_t{buf[0]};
   for (size_t i = 1; i < sizeof(T); ++i) {
-    *out <<= 8;
+    unsigned_result <<= 8;
     // Must cast to uint8_t to avoid clobbering by sign extension.
-    *out |= static_cast<uint8_t>(buf[i]);
+    unsigned_result |= static_cast<uint8_t>(buf[i]);
   }
+  *out = unsigned_result;
 }
 
 // Write an integer (signed or unsigned) |val| to |buf| in Big Endian order.
@@ -35,9 +38,10 @@ inline void ReadBigEndian(const char buf[], T* out) {
 template<typename T>
 inline void WriteBigEndian(char buf[], T val) {
   static_assert(std::is_integral<T>::value, "T has to be an integral type.");
+  auto unsigned_val = static_cast<typename std::make_unsigned<T>::type>(val);
   for (size_t i = 0; i < sizeof(T); ++i) {
-    buf[sizeof(T)-i-1] = static_cast<char>(val & 0xFF);
-    val >>= 8;
+    buf[sizeof(T) - i - 1] = static_cast<char>(unsigned_val & 0xFF);
+    unsigned_val >>= 8;
   }
 }
 
@@ -52,6 +56,16 @@ inline void WriteBigEndian<uint8_t>(char buf[], uint8_t val) {
   buf[0] = static_cast<char>(val);
 }
 
+template <>
+inline void ReadBigEndian<int8_t>(const char buf[], int8_t* out) {
+  *out = buf[0];
+}
+
+template <>
+inline void WriteBigEndian<int8_t>(char buf[], int8_t val) {
+  buf[0] = static_cast<char>(val);
+}
+
 // Allows reading integers in network order (big endian) while iterating over
 // an underlying buffer. All the reading functions advance the internal pointer.
 class BASE_EXPORT BigEndianReader {

base/big_endian_unittest.cc

@@ -13,6 +13,67 @@
 
 namespace base {
 
+TEST(ReadBigEndianTest, ReadSignedPositive) {
+  char data[] = {0x0A, 0x0B, 0x0C, 0x0D, 0x0E, 0x0F, 0x1A, 0X2A};
+  int8_t s8 = 0;
+  int16_t s16 = 0;
+  int32_t s32 = 0;
+  int64_t s64 = 0;
+  ReadBigEndian(data, &s8);
+  ReadBigEndian(data, &s16);
+  ReadBigEndian(data, &s32);
+  ReadBigEndian(data, &s64);
+  EXPECT_EQ(0x0A, s8);
+  EXPECT_EQ(0x0A0B, s16);
+  EXPECT_EQ(int32_t{0x0A0B0C0D}, s32);
+  EXPECT_EQ(int64_t{0x0A0B0C0D0E0F1A2All}, s64);
+}
+
+TEST(ReadBigEndianTest, ReadSignedNegative) {
+  char data[] = {0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0XFF};
+  int8_t s8 = 0;
+  int16_t s16 = 0;
+  int32_t s32 = 0;
+  int64_t s64 = 0;
+  ReadBigEndian(data, &s8);
+  ReadBigEndian(data, &s16);
+  ReadBigEndian(data, &s32);
+  ReadBigEndian(data, &s64);
+  EXPECT_EQ(-1, s8);
+  EXPECT_EQ(-1, s16);
+  EXPECT_EQ(-1, s32);
+  EXPECT_EQ(-1, s64);
+}
+
+TEST(ReadBigEndianTest, ReadUnsignedSigned) {
+  char data[] = {0xA0, 0xB0, 0xC0, 0xD0, 0xE0, 0xF0, 0xA1, 0XA2};
+  uint8_t u8 = 0;
+  uint16_t u16 = 0;
+  uint32_t u32 = 0;
+  uint64_t u64 = 0;
+  ReadBigEndian(data, &u8);
+  ReadBigEndian(data, &u16);
+  ReadBigEndian(data, &u32);
+  ReadBigEndian(data, &u64);
+  EXPECT_EQ(0xA0, u8);
+  EXPECT_EQ(0xA0B0, u16);
+  EXPECT_EQ(0xA0B0C0D0, u32);
+  EXPECT_EQ(0xA0B0C0D0E0F0A1A2ull, u64);
+}
+
+TEST(ReadBigEndianTest, TryAll16BitValues) {
+  using signed_type = int16_t;
+  char data[sizeof(signed_type)];
+  for (int i = std::numeric_limits<signed_type>::min();
+       i <= std::numeric_limits<signed_type>::max(); i++) {
+    signed_type expected = i;
+    signed_type actual = 0;
+    WriteBigEndian(data, expected);
+    ReadBigEndian(data, &actual);
+    EXPECT_EQ(expected, actual);
+  }
+}
+
 TEST(BigEndianReaderTest, ReadsValues) {
   char data[] = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xA, 0xB, 0xC, 0xD, 0xE, 0xF,
                   0x1A, 0x2B, 0x3C, 0x4D, 0x5E };