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>
diff --git a/core/fxcrt/bytestring.cpp b/core/fxcrt/bytestring.cpp
index b3cca98..ab54614 100644
--- a/core/fxcrt/bytestring.cpp
+++ b/core/fxcrt/bytestring.cpp
@@ -218,17 +218,23 @@
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 @@
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 @@
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 @@
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 7751eee..195824d 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 9d29050..3055444 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 0bafa78..daf2c0c 100644
--- a/core/fxcrt/span_util.h
+++ b/core/fxcrt/span_util.h
@@ -30,7 +30,10 @@
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 @@
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 @@
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 @@
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 @@
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 57c1c39..56dbece 100644
--- a/core/fxcrt/widestring.cpp
+++ b/core/fxcrt/widestring.cpp
@@ -510,17 +510,23 @@
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 @@
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 @@
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)
