commit dce81b86b4a71ff8a7c08ee2957ee925de174b6f
author Tom Sepez <tsepez@chromium.org> Mon Nov 08 23:17:31 2021 +0000
committer Pdfium LUCI CQ <pdfium-scoped@luci-project-accounts.iam.gserviceaccount.com> Mon Nov 08 23:17:31 2021 +0000
tree 5195d1c8171b6d522dbe391fda8de29ccabd2ef4
parent a4b742e982eb5cc7a5ea2ac7b30e7bcbed9c526d

Be more careful about sign extension in FXSYS_UINT32_GET_MSBFIRST() etc.

Make definition consistent with those used in fdrm/, which guard against
being applied to a signed char* ptr, getting back, say, a -28, then
sign-extending 1s when casting to uint32_t. We don't seem to do this
anywhere, but it is a good safeguard.

-- Add tests which flunk prior to this change.

Change-Id: I9993d6420947e5d0cd7478424747f7e701045428
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/86974
Commit-Queue: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>

core/fxcrt/byteorder_unittest.cpp

diff --git a/core/fxcrt/byteorder_unittest.cpp b/core/fxcrt/byteorder_unittest.cpp
index 1133a8a..11c3024 100644
--- a/core/fxcrt/byteorder_unittest.cpp
+++ b/core/fxcrt/byteorder_unittest.cpp
@@ -25,6 +25,10 @@
     uint16_t expected =
         FXSYS_UINT16_GET_LSBFIRST(reinterpret_cast<const uint8_t*>(&v16));
     EXPECT_EQ(expected, ByteSwapToLE16(v16)) << v;
+
+    // Check safety against unexpectedly signed bytes.
+    expected = FXSYS_UINT16_GET_LSBFIRST(reinterpret_cast<const int8_t*>(&v16));
+    EXPECT_EQ(expected, ByteSwapToLE16(v16)) << v;
   }
 }
 
@@ -33,6 +37,10 @@
     uint32_t expected =
         FXSYS_UINT32_GET_LSBFIRST(reinterpret_cast<const uint8_t*>(&v));
     EXPECT_EQ(expected, ByteSwapToLE32(v)) << v;
+
+    // Check safety against unexpectedly signed bytes.
+    expected = FXSYS_UINT32_GET_LSBFIRST(reinterpret_cast<const int8_t*>(&v));
+    EXPECT_EQ(expected, ByteSwapToLE32(v)) << v;
   }
 }
 
@@ -43,6 +51,10 @@
     uint16_t expected =
         FXSYS_UINT16_GET_MSBFIRST(reinterpret_cast<const uint8_t*>(&v16));
     EXPECT_EQ(expected, ByteSwapToBE16(v16)) << v;
+
+    // Check safety against unexpectedly signed bytes.
+    expected = FXSYS_UINT16_GET_MSBFIRST(reinterpret_cast<const int8_t*>(&v16));
+    EXPECT_EQ(expected, ByteSwapToBE16(v16)) << v;
   }
 }
 
@@ -51,6 +63,10 @@
     uint32_t expected =
         FXSYS_UINT32_GET_MSBFIRST(reinterpret_cast<const uint8_t*>(&v));
     EXPECT_EQ(expected, ByteSwapToBE32(v)) << v;
+
+    // Check safety against unexpectedly signed bytes.
+    expected = FXSYS_UINT32_GET_MSBFIRST(reinterpret_cast<const int8_t*>(&v));
+    EXPECT_EQ(expected, ByteSwapToBE32(v)) << v;
   }
 }
 

core/fxcrt/fx_system.h

diff --git a/core/fxcrt/fx_system.h b/core/fxcrt/fx_system.h
index b0d4ccf..03ce86b 100644
--- a/core/fxcrt/fx_system.h
+++ b/core/fxcrt/fx_system.h
@@ -118,20 +118,24 @@
 }
 
 // Could be C, but uses C++-style casting.
-#define FXSYS_UINT16_GET_LSBFIRST(p)                            \
-  (static_cast<uint16_t>((static_cast<uint32_t>((p)[1]) << 8) | \
-                         (static_cast<uint32_t>((p)[0]))))
-#define FXSYS_UINT16_GET_MSBFIRST(p)                            \
-  (static_cast<uint16_t>((static_cast<uint32_t>((p)[0]) << 8) | \
-                         (static_cast<uint32_t>((p)[1]))))
-#define FXSYS_UINT32_GET_LSBFIRST(p)       \
-  ((static_cast<uint32_t>((p)[3]) << 24) | \
-   (static_cast<uint32_t>((p)[2]) << 16) | \
-   (static_cast<uint32_t>((p)[1]) << 8) | (static_cast<uint32_t>((p)[0])))
-#define FXSYS_UINT32_GET_MSBFIRST(p)       \
-  ((static_cast<uint32_t>((p)[0]) << 24) | \
-   (static_cast<uint32_t>((p)[1]) << 16) | \
-   (static_cast<uint32_t>((p)[2]) << 8) | (static_cast<uint32_t>((p)[3])))
+#define FXSYS_UINT16_GET_LSBFIRST(p)                               \
+  (static_cast<uint16_t>(                                          \
+      (static_cast<uint32_t>(static_cast<uint8_t>((p)[1])) << 8) | \
+      (static_cast<uint32_t>(static_cast<uint8_t>((p)[0])))))
+#define FXSYS_UINT16_GET_MSBFIRST(p)                               \
+  (static_cast<uint16_t>(                                          \
+      (static_cast<uint32_t>(static_cast<uint8_t>((p)[0])) << 8) | \
+      (static_cast<uint32_t>(static_cast<uint8_t>((p)[1])))))
+#define FXSYS_UINT32_GET_LSBFIRST(p)                             \
+  ((static_cast<uint32_t>(static_cast<uint8_t>((p)[3])) << 24) | \
+   (static_cast<uint32_t>(static_cast<uint8_t>((p)[2])) << 16) | \
+   (static_cast<uint32_t>(static_cast<uint8_t>((p)[1])) << 8) |  \
+   (static_cast<uint32_t>(static_cast<uint8_t>((p)[0]))))
+#define FXSYS_UINT32_GET_MSBFIRST(p)                             \
+  ((static_cast<uint32_t>(static_cast<uint8_t>((p)[0])) << 24) | \
+   (static_cast<uint32_t>(static_cast<uint8_t>((p)[1])) << 16) | \
+   (static_cast<uint32_t>(static_cast<uint8_t>((p)[2])) << 8) |  \
+   (static_cast<uint32_t>(static_cast<uint8_t>((p)[3]))))
 #endif  // __cplusplus
 
 #endif  // CORE_FXCRT_FX_SYSTEM_H_