Mark fx_memcpy_wrappers.h functions UNSAFE_BUFFER_USAGE.

These are no safer than their underlying counterparts as far as
spatial safety, though they do avoid UB surrounding nullptrs.

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

5 files changed, 94 insertions, 40 deletions

diff --git a/core/fxcrt/bytestring.cpp b/core/fxcrt/bytestring.cpp
index b3cca9889..ab5461404 100644
--- a/core/fxcrt/bytestring.cpp
+++ b/core/fxcrt/bytestring.cpp
@@ -218,17 +218,23 @@ bool ByteString::operator==(const char* ptr) const {
   if (!ptr)
     return m_pData->m_nDataLength == 0;
 
+  // SAFETY: `m_nDataLength` is within `m_String`, and the strlen() call
+  // ensures there are `m_nDataLength` bytes at `ptr` before the terminator.
   return strlen(ptr) == m_pData->m_nDataLength &&
-         FXSYS_memcmp(ptr, m_pData->m_String, m_pData->m_nDataLength) == 0;
+         UNSAFE_BUFFERS(
+             FXSYS_memcmp(ptr, m_pData->m_String, m_pData->m_nDataLength)) == 0;
 }
 
 bool ByteString::operator==(ByteStringView str) const {
   if (!m_pData)
     return str.IsEmpty();
 
+  // SAFETY: `str` has `GetLength()` valid bytes in `unterminated_c_str()`,
+  // `m_nDataLength` is within `m_String`, and equality comparison.
   return m_pData->m_nDataLength == str.GetLength() &&
-         FXSYS_memcmp(m_pData->m_String, str.unterminated_c_str(),
-                      str.GetLength()) == 0;
+         UNSAFE_BUFFERS(FXSYS_memcmp(
+             m_pData->m_String, str.unterminated_c_str(), str.GetLength())) ==
+             0;
 }
 
 bool ByteString::operator==(const ByteString& other) const {
@@ -254,7 +260,10 @@ bool ByteString::operator<(const char* ptr) const {
 
   size_t len = GetLength();
   size_t other_len = ptr ? strlen(ptr) : 0;
-  int result = FXSYS_memcmp(c_str(), ptr, std::min(len, other_len));
+
+  // SAFETY: Comparison limited to minimum valid length of either argument.
+  int result =
+      UNSAFE_BUFFERS(FXSYS_memcmp(c_str(), ptr, std::min(len, other_len)));
   return result < 0 || (result == 0 && len < other_len);
 }
 
@@ -268,7 +277,10 @@ bool ByteString::operator<(const ByteString& other) const {
 
   size_t len = GetLength();
   size_t other_len = other.GetLength();
-  int result = FXSYS_memcmp(c_str(), other.c_str(), std::min(len, other_len));
+
+  // SAFETY: Comparison limited to minimum valid length of either argument.
+  int result = UNSAFE_BUFFERS(
+      FXSYS_memcmp(c_str(), other.c_str(), std::min(len, other_len)));
   return result < 0 || (result == 0 && len < other_len);
 }
 
@@ -344,8 +356,10 @@ int ByteString::Compare(ByteStringView str) const {
   size_t this_len = m_pData->m_nDataLength;
   size_t that_len = str.GetLength();
   size_t min_len = std::min(this_len, that_len);
-  int result =
-      FXSYS_memcmp(m_pData->m_String, str.unterminated_c_str(), min_len);
+
+  // SAFETY: Comparison limited to minimum valid length of either argument.
+  int result = UNSAFE_BUFFERS(
+      FXSYS_memcmp(m_pData->m_String, str.unterminated_c_str(), min_len));
   if (result != 0)
     return result;
   if (this_len == that_len)
diff --git a/core/fxcrt/fx_memcpy_wrappers.h b/core/fxcrt/fx_memcpy_wrappers.h
index 7751eeec0..195824d7b 100644
--- a/core/fxcrt/fx_memcpy_wrappers.h
+++ b/core/fxcrt/fx_memcpy_wrappers.h
@@ -12,49 +12,67 @@
 #include <string.h>
 #include <wchar.h>
 
+#include "core/fxcrt/compiler_specific.h"
+
 // Wrappers to avoid the zero-length w/NULL arg gotchas in C spec. Use these
 // if there is a possibility of a NULL arg (or a bad arg) that is to be ignored
 // when the length is zero, otherwise just call the C Run Time Library function
 // itself.
-inline int FXSYS_memcmp(const void* ptr1, const void* ptr2, size_t len) {
+UNSAFE_BUFFER_USAGE inline int FXSYS_memcmp(const void* ptr1,
+                                            const void* ptr2,
+                                            size_t len) {
   return len ? memcmp(ptr1, ptr2, len) : 0;
 }
 
-inline int FXSYS_wmemcmp(const wchar_t* ptr1, const wchar_t* ptr2, size_t len) {
+UNSAFE_BUFFER_USAGE inline int FXSYS_wmemcmp(const wchar_t* ptr1,
+                                             const wchar_t* ptr2,
+                                             size_t len) {
   return len ? wmemcmp(ptr1, ptr2, len) : 0;
 }
 
-inline void* FXSYS_memcpy(void* ptr1, const void* ptr2, size_t len) {
+UNSAFE_BUFFER_USAGE inline void* FXSYS_memcpy(void* ptr1,
+                                              const void* ptr2,
+                                              size_t len) {
   return len ? memcpy(ptr1, ptr2, len) : ptr1;
 }
 
-inline wchar_t* FXSYS_wmemcpy(wchar_t* ptr1, const wchar_t* ptr2, size_t len) {
+UNSAFE_BUFFER_USAGE inline wchar_t* FXSYS_wmemcpy(wchar_t* ptr1,
+                                                  const wchar_t* ptr2,
+                                                  size_t len) {
   return len ? wmemcpy(ptr1, ptr2, len) : ptr1;
 }
 
-inline void* FXSYS_memmove(void* ptr1, const void* ptr2, size_t len) {
+UNSAFE_BUFFER_USAGE inline void* FXSYS_memmove(void* ptr1,
+                                               const void* ptr2,
+                                               size_t len) {
   return len ? memmove(ptr1, ptr2, len) : ptr1;
 }
 
-inline wchar_t* FXSYS_wmemmove(wchar_t* ptr1, const wchar_t* ptr2, size_t len) {
+UNSAFE_BUFFER_USAGE inline wchar_t* FXSYS_wmemmove(wchar_t* ptr1,
+                                                   const wchar_t* ptr2,
+                                                   size_t len) {
   return len ? wmemmove(ptr1, ptr2, len) : ptr1;
 }
 
-inline void* FXSYS_memset(void* ptr1, int val, size_t len) {
+UNSAFE_BUFFER_USAGE inline void* FXSYS_memset(void* ptr1, int val, size_t len) {
   return len ? memset(ptr1, val, len) : ptr1;
 }
 
-inline wchar_t* FXSYS_wmemset(wchar_t* ptr1, int val, size_t len) {
+UNSAFE_BUFFER_USAGE inline wchar_t* FXSYS_wmemset(wchar_t* ptr1,
+                                                  int val,
+                                                  size_t len) {
   return len ? wmemset(ptr1, val, len) : ptr1;
 }
 
-inline const void* FXSYS_memchr(const void* ptr1, int val, size_t len) {
+UNSAFE_BUFFER_USAGE inline const void* FXSYS_memchr(const void* ptr1,
+                                                    int val,
+                                                    size_t len) {
   return len ? memchr(ptr1, val, len) : nullptr;
 }
 
-inline const wchar_t* FXSYS_wmemchr(const wchar_t* ptr1,
-                                    wchar_t val,
-                                    size_t len) {
+UNSAFE_BUFFER_USAGE inline const wchar_t* FXSYS_wmemchr(const wchar_t* ptr1,
+                                                        wchar_t val,
+                                                        size_t len) {
   return len ? wmemchr(ptr1, val, len) : nullptr;
 }
 
diff --git a/core/fxcrt/fx_memcpy_wrappers_unittest.cpp b/core/fxcrt/fx_memcpy_wrappers_unittest.cpp
index 9d2905028..3055444bf 100644
--- a/core/fxcrt/fx_memcpy_wrappers_unittest.cpp
+++ b/core/fxcrt/fx_memcpy_wrappers_unittest.cpp
@@ -8,50 +8,50 @@
 
 TEST(fxcrt, FXSYS_memset) {
   // Test passes if it does not trigger UBSAN warnings.
-  EXPECT_EQ(nullptr, FXSYS_memset(nullptr, 0, 0));
+  EXPECT_EQ(nullptr, UNSAFE_BUFFERS(FXSYS_memset(nullptr, 0, 0)));
 }
 
 TEST(fxcrt, FXSYS_wmemset) {
   // Test passes if it does not trigger UBSAN warnings.
-  EXPECT_EQ(nullptr, FXSYS_wmemset(nullptr, 0, 0));
+  EXPECT_EQ(nullptr, UNSAFE_BUFFERS(FXSYS_wmemset(nullptr, 0, 0)));
 }
 
 TEST(fxcrt, FXSYS_memcpy) {
   // Test passes if it does not trigger UBSAN warnings.
-  EXPECT_EQ(nullptr, FXSYS_memcpy(nullptr, nullptr, 0));
+  EXPECT_EQ(nullptr, UNSAFE_BUFFERS(FXSYS_memcpy(nullptr, nullptr, 0)));
 }
 
 TEST(fxcrt, FXSYS_wmemcpy) {
   // Test passes if it does not trigger UBSAN warnings.
-  EXPECT_EQ(nullptr, FXSYS_wmemcpy(nullptr, nullptr, 0));
+  EXPECT_EQ(nullptr, UNSAFE_BUFFERS(FXSYS_wmemcpy(nullptr, nullptr, 0)));
 }
 
 TEST(fxcrt, FXSYS_memmove) {
   // Test passes if it does not trigger UBSAN warnings.
-  EXPECT_EQ(nullptr, FXSYS_memmove(nullptr, nullptr, 0));
+  EXPECT_EQ(nullptr, UNSAFE_BUFFERS(FXSYS_memmove(nullptr, nullptr, 0)));
 }
 
 TEST(fxcrt, FXSYS_wmemmove) {
   // Test passes if it does not trigger UBSAN warnings.
-  EXPECT_EQ(nullptr, FXSYS_wmemmove(nullptr, nullptr, 0));
+  EXPECT_EQ(nullptr, UNSAFE_BUFFERS(FXSYS_wmemmove(nullptr, nullptr, 0)));
 }
 
 TEST(fxcrt, FXSYS_memcmp) {
   // Test passes if it does not trigger UBSAN warnings.
-  EXPECT_EQ(0, FXSYS_memcmp(nullptr, nullptr, 0));
+  EXPECT_EQ(0, UNSAFE_BUFFERS(FXSYS_memcmp(nullptr, nullptr, 0)));
 }
 
 TEST(fxcrt, FXSYS_wmemcmp) {
   // Test passes if it does not trigger UBSAN warnings.
-  EXPECT_EQ(0, FXSYS_wmemcmp(nullptr, nullptr, 0));
+  EXPECT_EQ(0, UNSAFE_BUFFERS(FXSYS_wmemcmp(nullptr, nullptr, 0)));
 }
 
 TEST(fxcrt, FXSYS_memchr) {
   // Test passes if it does not trigger UBSAN warnings.
-  EXPECT_EQ(nullptr, FXSYS_memchr(nullptr, 0, 0));
+  EXPECT_EQ(nullptr, UNSAFE_BUFFERS(FXSYS_memchr(nullptr, 0, 0)));
 }
 
 TEST(fxcrt, FXSYS_wmemchr) {
   // Test passes if it does not trigger UBSAN warnings.
-  EXPECT_EQ(nullptr, FXSYS_wmemchr(nullptr, 0, 0));
+  EXPECT_EQ(nullptr, UNSAFE_BUFFERS(FXSYS_wmemchr(nullptr, 0, 0)));
 }
diff --git a/core/fxcrt/span_util.h b/core/fxcrt/span_util.h
index 0bafa7895..daf2c0cfb 100644
--- a/core/fxcrt/span_util.h
+++ b/core/fxcrt/span_util.h
@@ -30,7 +30,10 @@ template <typename T1,
 inline pdfium::span<T1> spancpy(pdfium::span<T1, N1, P1> dst,
                                 pdfium::span<T2, N2, P2> src) {
   CHECK_GE(dst.size(), src.size());
-  FXSYS_memcpy(dst.data(), src.data(), src.size_bytes());
+  // SAFETY: SFINAE ensures `sizeof(T1)` equals `sizeof(T2)`, so comparing
+  // `size()` for equality ensures `size_bytes()` are equal, and `size_bytes()`
+  // accurately describes `data()`.
+  UNSAFE_BUFFERS(FXSYS_memcpy(dst.data(), src.data(), src.size_bytes()));
   return dst.subspan(src.size());
 }
 
@@ -48,7 +51,10 @@ template <typename T1,
 pdfium::span<T1> spanmove(pdfium::span<T1, N1, P1> dst,
                           pdfium::span<T2, N2, P2> src) {
   CHECK_GE(dst.size(), src.size());
-  FXSYS_memmove(dst.data(), src.data(), src.size_bytes());
+  // SAFETY: SFINAE ensures `sizeof(T1)` equals `sizeof(T2)`, so comparing
+  // `size()` for equality ensures `size_bytes()` are equal, and `size_bytes()`
+  // accurately describes `data()`.
+  UNSAFE_BUFFERS(FXSYS_memmove(dst.data(), src.data(), src.size_bytes()));
   return dst.subspan(src.size());
 }
 
@@ -59,7 +65,8 @@ template <typename T,
           typename = std::enable_if_t<std::is_trivially_constructible_v<T> &&
                                       std::is_trivially_destructible_v<T>>>
 void spanset(pdfium::span<T, N, P> dst, uint8_t val) {
-  FXSYS_memset(dst.data(), val, dst.size_bytes());
+  // SAFETY: `dst.size_bytes()` accurately describes `dst.data()`.
+  UNSAFE_BUFFERS(FXSYS_memset(dst.data(), val, dst.size_bytes()));
 }
 
 // Bounds-checked zeroing of spans.
@@ -69,7 +76,8 @@ template <typename T,
           typename = std::enable_if_t<std::is_trivially_constructible_v<T> &&
                                       std::is_trivially_destructible_v<T>>>
 void spanclr(pdfium::span<T, N, P> dst) {
-  FXSYS_memset(dst.data(), 0, dst.size_bytes());
+  // SAFETY: `dst.size_bytes()` accurately describes `dst.data()`.
+  UNSAFE_BUFFERS(FXSYS_memset(dst.data(), 0, dst.size_bytes()));
 }
 
 // Bounds-checked byte-for-byte equality of same-sized spans. This is
@@ -84,8 +92,11 @@ template <typename T1,
                                       std::is_trivially_copyable_v<T1> &&
                                       std::is_trivially_copyable_v<T2>>>
 bool span_equals(pdfium::span<T1, N1, P1> s1, pdfium::span<T2, N2, P2> s2) {
+  // SAFETY: For both `s1` and `s2`, there are `size_bytes()` valid bytes at
+  // the corresponding `data()`, and the sizes are the same.
   return s1.size_bytes() == s2.size_bytes() &&
-         FXSYS_memcmp(s1.data(), s2.data(), s1.size_bytes()) == 0;
+         UNSAFE_BUFFERS(FXSYS_memcmp(s1.data(), s2.data(), s1.size_bytes())) ==
+             0;
 }
 
 // Returns the first position where `needle` occurs in `haystack`.
diff --git a/core/fxcrt/widestring.cpp b/core/fxcrt/widestring.cpp
index 57c1c39e5..56dbece1a 100644
--- a/core/fxcrt/widestring.cpp
+++ b/core/fxcrt/widestring.cpp
@@ -510,17 +510,23 @@ bool WideString::operator==(const wchar_t* ptr) const {
   if (!ptr)
     return m_pData->m_nDataLength == 0;
 
+  // SAFTEY: `wsclen()` comparison ensures there are `m_nDataLength` wchars at
+  // `ptr` before the terminator, and `m_nDataLength` is within `m_String`.
   return wcslen(ptr) == m_pData->m_nDataLength &&
-         FXSYS_wmemcmp(ptr, m_pData->m_String, m_pData->m_nDataLength) == 0;
+         UNSAFE_BUFFERS(FXSYS_wmemcmp(ptr, m_pData->m_String,
+                                      m_pData->m_nDataLength)) == 0;
 }
 
 bool WideString::operator==(WideStringView str) const {
   if (!m_pData)
     return str.IsEmpty();
 
+  // SAFTEY: Comparison ensure there are `m_nDataLength` wchars in `str`
+  // and `m_nDataLength is within `m_String`.
   return m_pData->m_nDataLength == str.GetLength() &&
-         FXSYS_wmemcmp(m_pData->m_String, str.unterminated_c_str(),
-                       str.GetLength()) == 0;
+         UNSAFE_BUFFERS(FXSYS_wmemcmp(
+             m_pData->m_String, str.unterminated_c_str(), str.GetLength())) ==
+             0;
 }
 
 bool WideString::operator==(const WideString& other) const {
@@ -550,8 +556,10 @@ bool WideString::operator<(WideStringView str) const {
 
   size_t len = GetLength();
   size_t other_len = str.GetLength();
-  int result = FXSYS_wmemcmp(c_str(), str.unterminated_c_str(),
-                             std::min(len, other_len));
+
+  // SAFETY: Comparison limited to minimum valid length of either argument.
+  int result = UNSAFE_BUFFERS(FXSYS_wmemcmp(c_str(), str.unterminated_c_str(),
+                                            std::min(len, other_len)));
   return result < 0 || (result == 0 && len < other_len);
 }
 
@@ -792,7 +800,10 @@ int WideString::Compare(const WideString& str) const {
   size_t this_len = m_pData->m_nDataLength;
   size_t that_len = str.m_pData->m_nDataLength;
   size_t min_len = std::min(this_len, that_len);
-  int result = FXSYS_wmemcmp(m_pData->m_String, str.m_pData->m_String, min_len);
+
+  // SAFTEY: Comparison limited to minimum length of either argument.
+  int result = UNSAFE_BUFFERS(
+      FXSYS_wmemcmp(m_pData->m_String, str.m_pData->m_String, min_len));
   if (result != 0)
     return result;
   if (this_len == that_len)