Wrap mem*() functions to handle zero-length nullptr cases. Pacify ubsan in its most strictest of modes. Consolidate some code in the new fx_memcpy_wrappers.h file. Fix issues in pdfium_unittest. Bug: pdfium:2066 Change-Id: Ic5741fc054c0c83eae3145eab42104e40a5c2107 Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/109930 Commit-Queue: Tom Sepez <tsepez@chromium.org> Reviewed-by: Lei Zhang <thestig@chromium.org>
diff --git a/core/fpdfapi/parser/cpdf_simple_parser_unittest.cpp b/core/fpdfapi/parser/cpdf_simple_parser_unittest.cpp index 965d577..bb2e89f 100644 --- a/core/fpdfapi/parser/cpdf_simple_parser_unittest.cpp +++ b/core/fpdfapi/parser/cpdf_simple_parser_unittest.cpp
@@ -7,6 +7,7 @@ #include <iterator> #include "core/fpdfapi/parser/fpdf_parser_utility.h" +#include "core/fxcrt/fx_memcpy_wrappers.h" #include "testing/gtest/include/gtest/gtest.h" #include "testing/test_support.h" #include "third_party/base/span.h" @@ -57,8 +58,8 @@ EXPECT_EQ(data.expected_size, word.GetLength()) << " for case " << i; if (data.expected_size != word.GetLength()) continue; - EXPECT_EQ( - 0, memcmp(data.expected, word.unterminated_c_str(), data.expected_size)) + EXPECT_EQ(0, FXSYS_memcmp(data.expected, word.unterminated_c_str(), + data.expected_size)) << " for case " << i; } }
diff --git a/core/fxcodec/jpx/jpx_unittest.cpp b/core/fxcodec/jpx/jpx_unittest.cpp index b89a9101..84e6a35 100644 --- a/core/fxcodec/jpx/jpx_unittest.cpp +++ b/core/fxcodec/jpx/jpx_unittest.cpp
@@ -9,6 +9,7 @@ #include "core/fxcodec/jpx/cjpx_decoder.h" #include "core/fxcodec/jpx/jpx_decode_utils.h" +#include "core/fxcrt/fx_memcpy_wrappers.h" #include "core/fxcrt/fx_memory.h" #include "testing/gtest/include/gtest/gtest.h" #include "third_party/libopenjpeg/opj_malloc.h" @@ -424,9 +425,9 @@ opj_image_data_alloc(v.w * v.h * sizeof(OPJ_INT32))); u.data = static_cast<OPJ_INT32*>( opj_image_data_alloc(u.w * u.h * sizeof(OPJ_INT32))); - memset(y.data, 1, y.w * y.h * sizeof(OPJ_INT32)); - memset(u.data, 0, u.w * u.h * sizeof(OPJ_INT32)); - memset(v.data, 0, v.w * v.h * sizeof(OPJ_INT32)); + FXSYS_memset(y.data, 1, y.w * y.h * sizeof(OPJ_INT32)); + FXSYS_memset(u.data, 0, u.w * u.h * sizeof(OPJ_INT32)); + FXSYS_memset(v.data, 0, v.w * v.h * sizeof(OPJ_INT32)); img.comps[0] = y; img.comps[1] = u; img.comps[2] = v;
diff --git a/core/fxcrt/BUILD.gn b/core/fxcrt/BUILD.gn index 6bc74a4..272dd22 100644 --- a/core/fxcrt/BUILD.gn +++ b/core/fxcrt/BUILD.gn
@@ -63,6 +63,7 @@ "fx_extension.cpp", "fx_extension.h", "fx_folder.h", + "fx_memcpy_wrappers.h", "fx_memory.cpp", "fx_memory.h", "fx_memory_wrappers.h", @@ -218,6 +219,7 @@ "fx_bidi_unittest.cpp", "fx_coordinates_unittest.cpp", "fx_extension_unittest.cpp", + "fx_memcpy_wrappers_unittest.cpp", "fx_memory_unittest.cpp", "fx_memory_wrappers_unittest.cpp", "fx_number_unittest.cpp",
diff --git a/core/fxcrt/bytestring.cpp b/core/fxcrt/bytestring.cpp index 2091bf8..7478075 100644 --- a/core/fxcrt/bytestring.cpp +++ b/core/fxcrt/bytestring.cpp
@@ -16,6 +16,7 @@ #include "core/fxcrt/fx_codepage.h" #include "core/fxcrt/fx_extension.h" +#include "core/fxcrt/fx_memcpy_wrappers.h" #include "core/fxcrt/fx_safe_types.h" #include "core/fxcrt/fx_system.h" #include "core/fxcrt/string_pool_template.h" @@ -262,7 +263,7 @@ return m_pData->m_nDataLength == 0; return strlen(ptr) == m_pData->m_nDataLength && - memcmp(ptr, m_pData->m_String, m_pData->m_nDataLength) == 0; + FXSYS_memcmp(ptr, m_pData->m_String, m_pData->m_nDataLength) == 0; } bool ByteString::operator==(ByteStringView str) const { @@ -270,8 +271,8 @@ return str.IsEmpty(); return m_pData->m_nDataLength == str.GetLength() && - memcmp(m_pData->m_String, str.unterminated_c_str(), str.GetLength()) == - 0; + FXSYS_memcmp(m_pData->m_String, str.unterminated_c_str(), + str.GetLength()) == 0; } bool ByteString::operator==(const ByteString& other) const { @@ -297,7 +298,7 @@ size_t len = GetLength(); size_t other_len = ptr ? strlen(ptr) : 0; - int result = memcmp(c_str(), ptr, std::min(len, other_len)); + int result = FXSYS_memcmp(c_str(), ptr, std::min(len, other_len)); return result < 0 || (result == 0 && len < other_len); } @@ -311,7 +312,7 @@ size_t len = GetLength(); size_t other_len = other.GetLength(); - int result = memcmp(c_str(), other.c_str(), std::min(len, other_len)); + int result = FXSYS_memcmp(c_str(), other.c_str(), std::min(len, other_len)); return result < 0 || (result == 0 && len < other_len); } @@ -441,8 +442,8 @@ ReallocBeforeWrite(old_length); size_t chars_to_copy = old_length - removal_length + 1; - memmove(m_pData->m_String + index, m_pData->m_String + removal_length, - chars_to_copy); + FXSYS_memmove(m_pData->m_String + index, m_pData->m_String + removal_length, + chars_to_copy); m_pData->m_nDataLength = old_length - count; return m_pData->m_nDataLength; } @@ -534,8 +535,8 @@ const size_t new_length = cur_length + 1; ReallocBeforeWrite(new_length); - memmove(m_pData->m_String + index + 1, m_pData->m_String + index, - new_length - index); + FXSYS_memmove(m_pData->m_String + index + 1, m_pData->m_String + index, + new_length - index); m_pData->m_String[index] = ch; m_pData->m_nDataLength = new_length; return new_length; @@ -548,8 +549,8 @@ if (!IsValidIndex(start)) return absl::nullopt; - const char* pStr = static_cast<const char*>( - memchr(m_pData->m_String + start, ch, m_pData->m_nDataLength - start)); + const char* pStr = static_cast<const char*>(FXSYS_memchr( + m_pData->m_String + start, ch, m_pData->m_nDataLength - start)); return pStr ? absl::optional<size_t>( static_cast<size_t>(pStr - m_pData->m_String)) : absl::nullopt; @@ -668,13 +669,13 @@ for (size_t i = 0; i < nCount; i++) { const char* pTarget = FX_strstr(pStart, static_cast<int>(pEnd - pStart), pOld.unterminated_c_str(), nSourceLen); - memcpy(pDest, pStart, pTarget - pStart); + FXSYS_memcpy(pDest, pStart, pTarget - pStart); pDest += pTarget - pStart; - memcpy(pDest, pNew.unterminated_c_str(), pNew.GetLength()); + FXSYS_memcpy(pDest, pNew.unterminated_c_str(), pNew.GetLength()); pDest += pNew.GetLength(); pStart = pTarget + nSourceLen; } - memcpy(pDest, pStart, pEnd - pStart); + FXSYS_memcpy(pDest, pStart, pEnd - pStart); m_pData.Swap(pNewData); return nCount; } @@ -686,7 +687,8 @@ 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 = memcmp(m_pData->m_String, str.unterminated_c_str(), min_len); + int result = + FXSYS_memcmp(m_pData->m_String, str.unterminated_c_str(), min_len); if (result != 0) return result; if (this_len == that_len) @@ -738,8 +740,8 @@ if (pos) { ReallocBeforeWrite(len); size_t nDataLength = len - pos; - memmove(m_pData->m_String, m_pData->m_String + pos, - (nDataLength + 1) * sizeof(char)); + FXSYS_memmove(m_pData->m_String, m_pData->m_String + pos, + (nDataLength + 1) * sizeof(char)); m_pData->m_nDataLength = nDataLength; } }
diff --git a/core/fxcrt/fx_memcpy_wrappers.h b/core/fxcrt/fx_memcpy_wrappers.h new file mode 100644 index 0000000..2dc8c1c --- /dev/null +++ b/core/fxcrt/fx_memcpy_wrappers.h
@@ -0,0 +1,86 @@ +// Copyright 2023 The PDFium Authors +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +// Original code copyright 2014 Foxit Software Inc. http://www.foxitsoftware.com + +#ifndef CORE_FXCRT_FX_MEMCPY_WRAPPERS_H_ +#define CORE_FXCRT_FX_MEMCPY_WRAPPERS_H_ + +#include <stddef.h> +#include <stdint.h> +#include <string.h> +#include <wchar.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) { + return len ? memcmp(ptr1, ptr2, len) : 0; +} + +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) { + return len ? memcpy(ptr1, ptr2, len) : ptr1; +} + +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) { + return len ? memmove(ptr1, ptr2, len) : ptr1; +} + +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) { + return len ? memset(ptr1, val, len) : ptr1; +} + +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) { + return len ? memchr(ptr1, val, len) : nullptr; +} + +inline const wchar_t* FXSYS_wmemchr(const wchar_t* ptr1, + wchar_t val, + size_t len) { + return len ? wmemchr(ptr1, val, len) : nullptr; +} + +// Overloaded functions for C++ templates +inline size_t FXSYS_len(const char* ptr) { + return strlen(ptr); +} + +inline size_t FXSYS_len(const wchar_t* ptr) { + return wcslen(ptr); +} + +inline int FXSYS_cmp(const char* ptr1, const char* ptr2, size_t len) { + return FXSYS_memcmp(ptr1, ptr2, len); +} + +inline int FXSYS_cmp(const wchar_t* ptr1, const wchar_t* ptr2, size_t len) { + return FXSYS_wmemcmp(ptr1, ptr2, len); +} + +inline const char* FXSYS_chr(const char* ptr, char ch, size_t len) { + return reinterpret_cast<const char*>(FXSYS_memchr(ptr, ch, len)); +} + +inline const wchar_t* FXSYS_chr(const wchar_t* ptr, wchar_t ch, size_t len) { + return FXSYS_wmemchr(ptr, ch, len); +} + +#endif // CORE_FXCRT_FX_MEMCPY_WRAPPERS_H_
diff --git a/core/fxcrt/fx_memcpy_wrappers_unittest.cpp b/core/fxcrt/fx_memcpy_wrappers_unittest.cpp new file mode 100644 index 0000000..9d29050 --- /dev/null +++ b/core/fxcrt/fx_memcpy_wrappers_unittest.cpp
@@ -0,0 +1,57 @@ +// Copyright 2023 The PDFium Authors +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "core/fxcrt/fx_memcpy_wrappers.h" + +#include "testing/gtest/include/gtest/gtest.h" + +TEST(fxcrt, FXSYS_memset) { + // Test passes if it does not trigger UBSAN warnings. + EXPECT_EQ(nullptr, 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)); +} + +TEST(fxcrt, FXSYS_memcpy) { + // Test passes if it does not trigger UBSAN warnings. + EXPECT_EQ(nullptr, 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)); +} + +TEST(fxcrt, FXSYS_memmove) { + // Test passes if it does not trigger UBSAN warnings. + EXPECT_EQ(nullptr, 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)); +} + +TEST(fxcrt, FXSYS_memcmp) { + // Test passes if it does not trigger UBSAN warnings. + EXPECT_EQ(0, 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)); +} + +TEST(fxcrt, FXSYS_memchr) { + // Test passes if it does not trigger UBSAN warnings. + EXPECT_EQ(nullptr, 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)); +}
diff --git a/core/fxcrt/fx_system.h b/core/fxcrt/fx_system.h index 52f9ce5..baa2a8a 100644 --- a/core/fxcrt/fx_system.h +++ b/core/fxcrt/fx_system.h
@@ -92,31 +92,6 @@ // C++-only section -// Overloaded functions for C++ templates -inline size_t FXSYS_len(const char* ptr) { - return strlen(ptr); -} - -inline size_t FXSYS_len(const wchar_t* ptr) { - return wcslen(ptr); -} - -inline int FXSYS_cmp(const char* ptr1, const char* ptr2, size_t len) { - return memcmp(ptr1, ptr2, len); -} - -inline int FXSYS_cmp(const wchar_t* ptr1, const wchar_t* ptr2, size_t len) { - return wmemcmp(ptr1, ptr2, len); -} - -inline const char* FXSYS_chr(const char* ptr, char ch, size_t len) { - return reinterpret_cast<const char*>(memchr(ptr, ch, len)); -} - -inline const wchar_t* FXSYS_chr(const wchar_t* ptr, wchar_t ch, size_t len) { - return wmemchr(ptr, ch, len); -} - // Could be C, but uses C++-style casting. #define FXSYS_UINT16_GET_LSBFIRST(p) \ (static_cast<uint16_t>( \
diff --git a/core/fxcrt/span_util.h b/core/fxcrt/span_util.h index 99fc25c..683d586 100644 --- a/core/fxcrt/span_util.h +++ b/core/fxcrt/span_util.h
@@ -5,8 +5,7 @@ #ifndef CORE_FXCRT_SPAN_UTIL_H_ #define CORE_FXCRT_SPAN_UTIL_H_ -#include <string.h> - +#include "core/fxcrt/fx_memcpy_wrappers.h" #include "third_party/base/check_op.h" #include "third_party/base/span.h" @@ -17,10 +16,8 @@ typename U, typename = pdfium::internal::EnableIfLegalSpanConversion<T, U>> void spancpy(pdfium::span<T> dst, pdfium::span<U> src) { - if (src.size_bytes()) { - CHECK_GE(dst.size_bytes(), src.size_bytes()); - memcpy(dst.data(), src.data(), src.size_bytes()); - } + CHECK_GE(dst.size_bytes(), src.size_bytes()); + FXSYS_memcpy(dst.data(), src.data(), src.size_bytes()); } // Bounds-checked moves from spans into spans. @@ -28,26 +25,20 @@ typename U, typename = pdfium::internal::EnableIfLegalSpanConversion<T, U>> void spanmove(pdfium::span<T> dst, pdfium::span<U> src) { - if (src.size_bytes()) { - CHECK_GE(dst.size_bytes(), src.size_bytes()); - memmove(dst.data(), src.data(), src.size_bytes()); - } + CHECK_GE(dst.size_bytes(), src.size_bytes()); + FXSYS_memmove(dst.data(), src.data(), src.size_bytes()); } // Bounds-checked sets into spans. template <typename T> void spanset(pdfium::span<T> dst, uint8_t val) { - if (dst.size_bytes()) { - memset(dst.data(), val, dst.size_bytes()); - } + FXSYS_memset(dst.data(), val, dst.size_bytes()); } // Bounds-checked zeroing of spans. template <typename T> void spanclr(pdfium::span<T> dst) { - if (dst.size_bytes()) { - memset(dst.data(), 0, dst.size_bytes()); - } + FXSYS_memset(dst.data(), 0, dst.size_bytes()); } } // namespace fxcrt
diff --git a/core/fxcrt/string_data_template.cpp b/core/fxcrt/string_data_template.cpp index bc04534..3e45bec 100644 --- a/core/fxcrt/string_data_template.cpp +++ b/core/fxcrt/string_data_template.cpp
@@ -10,6 +10,7 @@ #include <new> +#include "core/fxcrt/fx_memcpy_wrappers.h" #include "core/fxcrt/fx_memory.h" #include "core/fxcrt/fx_safe_types.h" #include "third_party/base/check.h" @@ -73,7 +74,7 @@ size_t nLen) { DCHECK_GE(nLen, 0); DCHECK_LE(nLen, m_nAllocLength); - memcpy(m_String, pStr, nLen * sizeof(CharType)); + FXSYS_memcpy(m_String, pStr, nLen * sizeof(CharType)); m_String[nLen] = 0; } @@ -84,7 +85,7 @@ DCHECK_GE(offset, 0); DCHECK_GE(nLen, 0); DCHECK_LE(offset + nLen, m_nAllocLength); - memcpy(m_String + offset, pStr, nLen * sizeof(CharType)); + FXSYS_memcpy(m_String + offset, pStr, nLen * sizeof(CharType)); m_String[offset + nLen] = 0; }
diff --git a/core/fxcrt/string_view_template.h b/core/fxcrt/string_view_template.h index 8a2c6b2..1f3eca3 100644 --- a/core/fxcrt/string_view_template.h +++ b/core/fxcrt/string_view_template.h
@@ -13,6 +13,7 @@ #include <iterator> #include <type_traits> +#include "core/fxcrt/fx_memcpy_wrappers.h" #include "core/fxcrt/fx_system.h" #include "third_party/abseil-cpp/absl/types/optional.h" #include "third_party/base/span.h"
diff --git a/core/fxcrt/widestring.cpp b/core/fxcrt/widestring.cpp index f87447d..94b6a61 100644 --- a/core/fxcrt/widestring.cpp +++ b/core/fxcrt/widestring.cpp
@@ -14,6 +14,7 @@ #include "core/fxcrt/fx_codepage.h" #include "core/fxcrt/fx_extension.h" +#include "core/fxcrt/fx_memcpy_wrappers.h" #include "core/fxcrt/fx_safe_types.h" #include "core/fxcrt/fx_system.h" #include "core/fxcrt/string_pool_template.h" @@ -465,7 +466,7 @@ return m_pData->m_nDataLength == 0; return wcslen(ptr) == m_pData->m_nDataLength && - wmemcmp(ptr, m_pData->m_String, m_pData->m_nDataLength) == 0; + FXSYS_wmemcmp(ptr, m_pData->m_String, m_pData->m_nDataLength) == 0; } bool WideString::operator==(WideStringView str) const { @@ -473,8 +474,8 @@ return str.IsEmpty(); return m_pData->m_nDataLength == str.GetLength() && - wmemcmp(m_pData->m_String, str.unterminated_c_str(), - str.GetLength()) == 0; + FXSYS_wmemcmp(m_pData->m_String, str.unterminated_c_str(), + str.GetLength()) == 0; } bool WideString::operator==(const WideString& other) const { @@ -504,8 +505,8 @@ size_t len = GetLength(); size_t other_len = str.GetLength(); - int result = - wmemcmp(c_str(), str.unterminated_c_str(), std::min(len, other_len)); + int result = FXSYS_wmemcmp(c_str(), str.unterminated_c_str(), + std::min(len, other_len)); return result < 0 || (result == 0 && len < other_len); } @@ -769,8 +770,8 @@ const size_t new_length = cur_length + 1; ReallocBeforeWrite(new_length); - wmemmove(m_pData->m_String + index + 1, m_pData->m_String + index, - new_length - index); + FXSYS_wmemmove(m_pData->m_String + index + 1, m_pData->m_String + index, + new_length - index); m_pData->m_String[index] = ch; m_pData->m_nDataLength = new_length; return new_length; @@ -783,8 +784,8 @@ if (!IsValidIndex(start)) return absl::nullopt; - const wchar_t* pStr = - wmemchr(m_pData->m_String + start, ch, m_pData->m_nDataLength - start); + const wchar_t* pStr = FXSYS_wmemchr(m_pData->m_String + start, ch, + m_pData->m_nDataLength - start); return pStr ? absl::optional<size_t>( static_cast<size_t>(pStr - m_pData->m_String)) : absl::nullopt; @@ -905,13 +906,13 @@ const wchar_t* pTarget = FX_wcsstr(pStart, static_cast<size_t>(pEnd - pStart), pOld.unterminated_c_str(), nSourceLen); - wmemcpy(pDest, pStart, pTarget - pStart); + FXSYS_wmemcpy(pDest, pStart, pTarget - pStart); pDest += pTarget - pStart; - wmemcpy(pDest, pNew.unterminated_c_str(), pNew.GetLength()); + FXSYS_wmemcpy(pDest, pNew.unterminated_c_str(), pNew.GetLength()); pDest += pNew.GetLength(); pStart = pTarget + nSourceLen; } - wmemcpy(pDest, pStart, pEnd - pStart); + FXSYS_wmemcpy(pDest, pStart, pEnd - pStart); m_pData.Swap(pNewData); return count; } @@ -1010,7 +1011,7 @@ 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 = wmemcmp(m_pData->m_String, str.m_pData->m_String, min_len); + int result = FXSYS_wmemcmp(m_pData->m_String, str.m_pData->m_String, min_len); if (result != 0) return result; if (this_len == that_len)