From e258a81d6972574b7a68a7919c6a70418650ae04 Mon Sep 17 00:00:00 2001 From: David Roger Date: Fri, 4 Aug 2023 12:26:45 +0000 Subject: [PATCH] Revert "[PA] Add CStringBuilder to replace std::ostringstream used by LogMessage." This reverts commit 07a88f3ddd387eb740f5ed30186ee17b3fb9c07b. Reason for revert: breaks MSAN bot https://ci.chromium.org/ui/p/chromium/builders/ci/Linux%20ChromiumOS%20MSan%20Tests/37602/test-results [ RUN ] CStringBuilderTestPA.Char ==313264==WARNING: MemorySanitizer: use-of-uninitialized-value #0 0x563432178a89 in partition_alloc::internal::base::strings::CStringBuilder::c_str() ./../../base/allocator/partition_allocator/partition_alloc_base/strings/cstring_builder.cc:145:7 #1 0x563431d9652c in partition_alloc::internal::base::strings::CStringBuilderTestPA_Char_Test::TestBody() ./../../base/allocator/partition_allocator/partition_alloc_base/strings/cstring_builder_pa_unittest.cc:33:3 #2 0x5634322101ba in HandleExceptionsInMethodIfSupported ./../../third_party/googletest/src/googletest/src/gtest-internal-inl.h:0:10 #3 0x5634322101ba in testing::Test::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:2670:5 #4 0x563432212ae1 in testing::TestInfo::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:2849:11 #5 0x563432214b72 in testing::TestSuite::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:3008:30 #6 0x563432242552 in testing::internal::UnitTestImpl::RunAllTests() ./../../third_party/googletest/src/googletest/src/gtest.cc:5866:44 #7 0x563432241624 in HandleExceptionsInMethodIfSupported ./../../third_party/googletest/src/googletest/src/gtest-internal-inl.h:0:10 #8 0x563432241624 in testing::UnitTest::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:5440:10 #9 0x5634329698b0 in RUN_ALL_TESTS ./../../third_party/googletest/src/googletest/include/gtest/gtest.h:2284:73 #10 0x5634329698b0 in base::TestSuite::Run() ./../../base/test/test_suite.cc:461:16 #11 0x5634329cc377 in Run ./../../base/functional/callback.h:152:12 #12 0x5634329cc377 in RunTestSuite ./../../base/test/launcher/unit_test_launcher.cc:179:38 #13 0x5634329cc377 in base::(anonymous namespace)::LaunchUnitTestsInternal(base::OnceCallback, unsigned long, int, unsigned long, bool, base::RepeatingCallback, base::OnceCallback) ./../../base/test/launcher/unit_test_launcher.cc:240:10 #14 0x5634329cba64 in base::LaunchUnitTests(int, char**, base::OnceCallback, unsigned long) ./../../base/test/launcher/unit_test_launcher.cc:288:10 #15 0x5634328f3de4 in main ./../../base/test/run_all_unittests.cc:70:10 #16 0x7f308e293082 in __libc_start_main ??:0:0 #17 0x56342e6a1349 in _start ??:0:0 Uninitialized value was created by an allocation of 'builder' in the stack frame #0 0x563431d96365 in partition_alloc::internal::base::strings::CStringBuilderTestPA_Char_Test::TestBody() ./../../base/allocator/partition_allocator/partition_alloc_base/strings/cstring_builder_pa_unittest.cc:31:3 SUMMARY: MemorySanitizer: use-of-uninitialized-value (/b/s/w/ir/out/Release/base_unittests+0x43f5a89) (BuildId: 039152aa25034492) Exiting Original change's description: > [PA] Add CStringBuilder to replace std::ostringstream used by LogMessage. > > Since std::ostringstream allocates and deallocates memory from heap, c.f. https://source.chromium.org/chromium/chromium/src/+/refs/heads/main:buildtools/third_party/libc++/trunk/src/ios.cpp > > std::ostringstream is not available inside memory allocation. Instead > add CStringBuilder (not resize, fixed buffer size) for LogMessage. > > Change-Id: I8051978487acc5fc9b976d6085909b43f81d9d0d > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4744311 > Reviewed-by: Yuki Shiino > Commit-Queue: Takashi Sakamoto > Cr-Commit-Position: refs/heads/main@{#1179481} Change-Id: Idb29e3252d9fe67955c0ae25ab46640ebfdae336 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4748189 Auto-Submit: David Roger Owners-Override: David Roger Commit-Queue: David Roger Bot-Commit: Rubber Stamper Cr-Commit-Position: refs/heads/main@{#1179539} --- base/BUILD.gn | 1 - base/allocator/partition_allocator/BUILD.gn | 2 - .../strings/cstring_builder.cc | 216 ------------------ .../strings/cstring_builder.h | 57 ----- .../strings/cstring_builder_pa_unittest.cc | 145 ------------ 5 files changed, 421 deletions(-) delete mode 100644 base/allocator/partition_allocator/partition_alloc_base/strings/cstring_builder.cc delete mode 100644 base/allocator/partition_allocator/partition_alloc_base/strings/cstring_builder.h delete mode 100644 base/allocator/partition_allocator/partition_alloc_base/strings/cstring_builder_pa_unittest.cc diff --git a/base/BUILD.gn b/base/BUILD.gn index 4c96f230754cf2..56b977605353ec 100644 --- a/base/BUILD.gn +++ b/base/BUILD.gn @@ -3802,7 +3802,6 @@ test("base_unittests") { "allocator/partition_allocator/partition_alloc_base/logging_pa_unittest.cc", "allocator/partition_allocator/partition_alloc_base/rand_util_pa_unittest.cc", "allocator/partition_allocator/partition_alloc_base/scoped_clear_last_error_pa_unittest.cc", - "allocator/partition_allocator/partition_alloc_base/strings/cstring_builder_pa_unittest.cc", "allocator/partition_allocator/partition_alloc_base/strings/safe_sprintf_pa_unittest.cc", "allocator/partition_allocator/partition_alloc_base/strings/stringprintf_pa_unittest.cc", "allocator/partition_allocator/partition_alloc_base/thread_annotations_pa_unittest.cc", diff --git a/base/allocator/partition_allocator/BUILD.gn b/base/allocator/partition_allocator/BUILD.gn index 6f147969618a2d..9a26e0ad38c854 100644 --- a/base/allocator/partition_allocator/BUILD.gn +++ b/base/allocator/partition_allocator/BUILD.gn @@ -376,8 +376,6 @@ source_set("allocator_base") { "partition_alloc_base/rand_util.cc", "partition_alloc_base/rand_util.h", "partition_alloc_base/scoped_clear_last_error.h", - "partition_alloc_base/strings/cstring_builder.cc", - "partition_alloc_base/strings/cstring_builder.h", "partition_alloc_base/strings/safe_sprintf.cc", "partition_alloc_base/strings/safe_sprintf.h", "partition_alloc_base/strings/stringprintf.cc", diff --git a/base/allocator/partition_allocator/partition_alloc_base/strings/cstring_builder.cc b/base/allocator/partition_allocator/partition_alloc_base/strings/cstring_builder.cc deleted file mode 100644 index 33f3926a51adba..00000000000000 --- a/base/allocator/partition_allocator/partition_alloc_base/strings/cstring_builder.cc +++ /dev/null @@ -1,216 +0,0 @@ -// Copyright 2023 The Chromium Authors -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "base/allocator/partition_allocator/partition_alloc_base/strings/cstring_builder.h" - -#include "base/allocator/partition_allocator/partition_alloc_base/debug/debugging_buildflags.h" -#include "base/allocator/partition_allocator/partition_alloc_base/strings/safe_sprintf.h" -#include "build/build_config.h" - -#if !BUILDFLAG(IS_WIN) -#include -#endif - -#include -#include - -#if BUILDFLAG(PA_DCHECK_IS_ON) -#include "base/allocator/partition_allocator/partition_alloc_base/check.h" -#define PA_RAW_DCHECK PA_RAW_CHECK -#else -#define PA_RAW_DCHECK(x) \ - do { \ - if (x) { \ - } \ - } while (0) -#endif - -namespace partition_alloc::internal::base::strings { - -namespace { - -constexpr size_t kNumDigits10 = 5u; - -constexpr uint64_t Pow10(unsigned exp) { - uint64_t ret = 1; - for (unsigned i = 0; i < exp; ++i) { - ret *= 10U; - } - return ret; -} - -constexpr uint64_t Log10(uint64_t value) { - uint64_t ret = 0; - while (value != 0u) { - value = value / 10u; - ++ret; - } - return ret; -} - -constexpr uint64_t GetDigits10(unsigned num_digits10) { - return Pow10(num_digits10); -} - -} // namespace - -template -void CStringBuilder::PutInteger(T value) { - // We need an array of chars whose size is: - // - floor(log10(max value)) + 1 chars for the give value, and - // - 1 char for '-' (if negative) - // - 1 char for '\0' - char buffer[Log10(std::numeric_limits::max()) + 3]; - ssize_t n = base::strings::SafeSPrintf(buffer, "%d", value); - PA_RAW_DCHECK(n >= 0); - PA_RAW_DCHECK(static_cast(n) < sizeof(buffer)); - PutText(buffer, n); -} - -CStringBuilder& CStringBuilder::operator<<(char ch) { - PutText(&ch, 1); - return *this; -} - -CStringBuilder& CStringBuilder::operator<<(const char* text) { - PutText(text); - return *this; -} - -CStringBuilder& CStringBuilder::operator<<(float value) { - PutFloatingPoint(value, kNumDigits10); - return *this; -} - -CStringBuilder& CStringBuilder::operator<<(double value) { - PutFloatingPoint(value, kNumDigits10); - return *this; -} - -CStringBuilder& CStringBuilder::operator<<(int value) { - PutInteger(value); - return *this; -} - -CStringBuilder& CStringBuilder::operator<<(unsigned int value) { - PutInteger(value); - return *this; -} - -CStringBuilder& CStringBuilder::operator<<(long value) { - PutInteger(value); - return *this; -} - -CStringBuilder& CStringBuilder::operator<<(unsigned long value) { - PutInteger(value); - return *this; -} - -CStringBuilder& CStringBuilder::operator<<(long long value) { - PutInteger(value); - return *this; -} - -CStringBuilder& CStringBuilder::operator<<(unsigned long long value) { - PutInteger(value); - return *this; -} - -CStringBuilder& CStringBuilder::operator<<(const void* value) { - if (!value) { - PutText("(nil)"); - } else { - // We need an array of chars whose size is: - // - 2 chars per 1 byte(00-FF), totally sizeof(const void*) * 2 chars, - // - 2 chars for "0x", - // - 1 char for '\0', - char buffer[sizeof(const void*) * 2 + 2 + 1]; - ssize_t n = base::strings::SafeSPrintf(buffer, "%p", value); - PA_RAW_DCHECK(n > 0); - PA_RAW_DCHECK(static_cast(n) < sizeof(buffer)); - PutText(buffer, n); - } - return *this; -} - -CStringBuilder& CStringBuilder::operator<<(std::nullptr_t) { - PutText("nullptr"); - return *this; -} - -const char* CStringBuilder::c_str() { - PA_RAW_DCHECK(buffer_ <= ptr_ && ptr_ < buffer_ + kBufferSize); - if (*ptr_ != '\0') { - *ptr_ = '\0'; - } - return buffer_; -} - -void CStringBuilder::PutFloatingPoint(double value, unsigned num_digits10) { - switch (fpclassify(value)) { - case FP_INFINITE: - PutText(value < 0 ? "-inf" : "inf"); - break; - case FP_NAN: - PutText("NaN"); - break; - case FP_ZERO: - PutText("0"); - break; - case FP_SUBNORMAL: - // Denormalized values are not supported. - PutNormalFloatingPoint(value > 0 ? std::numeric_limits::min() - : -std::numeric_limits::min(), - num_digits10); - break; - case FP_NORMAL: - default: - PutNormalFloatingPoint(value, num_digits10); - break; - } -} - -void CStringBuilder::PutNormalFloatingPoint(double value, - unsigned num_digits10) { - if (value < 0) { - PutText("-", 1); - value = -value; - } - - int exponent = floor(log10(value)); - double significand = value / pow(10, exponent); - - char buffer[64]; - ssize_t n = base::strings::SafeSPrintf( - buffer, "%d", lrint(significand * GetDigits10(num_digits10))); - PA_RAW_DCHECK(n > 0); - PA_RAW_DCHECK(static_cast(n) < sizeof(buffer)); - PutText(buffer, 1); - if (n > 1) { - PutText(".", 1); - PutText(buffer + 1, n - 1); - } - if (exponent != 0) { - n = base::strings::SafeSPrintf(buffer, "e%s%d", exponent > 0 ? "+" : "", - exponent); - PA_RAW_DCHECK(n > 0); - PA_RAW_DCHECK(static_cast(n) < sizeof(buffer)); - PutText(buffer, n); - } -} - -void CStringBuilder::PutText(const char* text) { - PutText(text, strlen(text)); -} - -void CStringBuilder::PutText(const char* text, size_t length) { - PA_RAW_DCHECK(buffer_ <= ptr_ && ptr_ < buffer_ + kBufferSize); - while (ptr_ < buffer_ + kBufferSize - 1 && length > 0 && *text != '\0') { - *ptr_++ = *text++; - --length; - } -} - -} // namespace partition_alloc::internal::base::strings diff --git a/base/allocator/partition_allocator/partition_alloc_base/strings/cstring_builder.h b/base/allocator/partition_allocator/partition_alloc_base/strings/cstring_builder.h deleted file mode 100644 index 96acc68becbaa5..00000000000000 --- a/base/allocator/partition_allocator/partition_alloc_base/strings/cstring_builder.h +++ /dev/null @@ -1,57 +0,0 @@ -// Copyright 2023 The Chromium Authors -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#ifndef BASE_ALLOCATOR_PARTITION_ALLOCATOR_PARTITION_ALLOC_BASE_STRINGS_CSTRING_BUILDER_H_ -#define BASE_ALLOCATOR_PARTITION_ALLOCATOR_PARTITION_ALLOC_BASE_STRINGS_CSTRING_BUILDER_H_ - -#include "base/allocator/partition_allocator/partition_alloc_base/component_export.h" -#include "build/build_config.h" - -#include - -#if !BUILDFLAG(IS_WIN) -#include -#endif - -namespace partition_alloc::internal::base::strings { - -// Similar to std::ostringstream, but creates a C string, i.e. nul-terminated -// char-type string, instead of std::string. To use inside memory allocation, -// this method must not allocate any memory with malloc, aligned_malloc, -// calloc, and so on. -class PA_COMPONENT_EXPORT(PARTITION_ALLOC) CStringBuilder { - public: - static constexpr size_t kBufferSize = 1024u; - - CStringBuilder() : ptr_(buffer_) {} - - CStringBuilder& operator<<(char ch); - CStringBuilder& operator<<(const char* text); - CStringBuilder& operator<<(float value); - CStringBuilder& operator<<(double value); - CStringBuilder& operator<<(int value); - CStringBuilder& operator<<(unsigned int value); - CStringBuilder& operator<<(long value); - CStringBuilder& operator<<(unsigned long value); - CStringBuilder& operator<<(long long value); - CStringBuilder& operator<<(unsigned long long value); - CStringBuilder& operator<<(const void* value); - CStringBuilder& operator<<(std::nullptr_t); - const char* c_str(); - - private: - template - void PutInteger(T value); - void PutFloatingPoint(double value, unsigned num_digits10); - void PutNormalFloatingPoint(double value, unsigned num_digits10); - void PutText(const char* text); - void PutText(const char* text, size_t length); - - char buffer_[kBufferSize]; - char* ptr_; -}; - -} // namespace partition_alloc::internal::base::strings - -#endif // BASE_ALLOCATOR_PARTITION_ALLOCATOR_PARTITION_ALLOC_BASE_STRINGS_CSTRING_BUILDER_H_ diff --git a/base/allocator/partition_allocator/partition_alloc_base/strings/cstring_builder_pa_unittest.cc b/base/allocator/partition_allocator/partition_alloc_base/strings/cstring_builder_pa_unittest.cc deleted file mode 100644 index 1dbcaa9dadc96a..00000000000000 --- a/base/allocator/partition_allocator/partition_alloc_base/strings/cstring_builder_pa_unittest.cc +++ /dev/null @@ -1,145 +0,0 @@ -// Copyright 2023 The Chromium Authors -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "base/allocator/partition_allocator/partition_alloc_base/strings/cstring_builder.h" - -#include "build/build_config.h" -#include "testing/gtest/include/gtest/gtest.h" - -namespace partition_alloc::internal::base::strings { - -namespace { - -template -void FillBuffer(CStringBuilder& builder, T value, unsigned count) { - for (unsigned i = 0; i < count; ++i) { - builder << value; - } -} - -} // namespace - -TEST(CStringBuilderTestPA, String) { - CStringBuilder builder; - const char buffer[] = "Buffer\n"; - builder << "Hello, World" << '\n' << buffer << '\n'; - EXPECT_EQ("Hello, World\nBuffer\n\n", std::string(builder.c_str())); -} - -TEST(CStringBuilderTestPA, Char) { - CStringBuilder builder; - builder << 'c' << 'h' << 'a' << ' ' << 'r' << '\\' << '\n'; - EXPECT_EQ("cha r\\\n", std::string(builder.c_str())); - builder << '\0' << '\n'; - EXPECT_EQ("cha r\\\n\n", std::string(builder.c_str())); -} - -TEST(CStringBuilderTestPA, Integer) { - CStringBuilder builder1; - builder1 << std::numeric_limits::max(); - EXPECT_EQ("18446744073709551615", std::string(builder1.c_str())); - - builder1 << " " << std::numeric_limits::min(); - EXPECT_EQ("18446744073709551615 -9223372036854775808", - std::string(builder1.c_str())); - - CStringBuilder builder2; - builder2 << std::numeric_limits::max(); - EXPECT_EQ("9223372036854775807", std::string(builder2.c_str())); - - CStringBuilder builder3; - builder3 << std::numeric_limits::min(); - EXPECT_EQ("-9223372036854775808", std::string(builder3.c_str())); -} - -TEST(CStringBuilderTestPA, FloatingPoint) { - CStringBuilder builder1; - builder1 << 3.1415926; - EXPECT_EQ("3.14159", std::string(builder1.c_str())); - - CStringBuilder builder2; - builder2 << 0.0000725692; - EXPECT_EQ("7.25692e-5", std::string(builder2.c_str())); - - // Zero - CStringBuilder builder3; - builder3 << 0.0; - EXPECT_EQ("0", std::string(builder3.c_str())); - - // min() - CStringBuilder builder4; - builder4 << std::numeric_limits::min(); - EXPECT_EQ("2.22507e-308", std::string(builder4.c_str())); - - // Subnormal value - CStringBuilder builder5; - builder5 << std::numeric_limits::denorm_min(); - // denorm_min() < min() - EXPECT_EQ("2.22507e-308", std::string(builder5.c_str())); - - // Positive Infinity - CStringBuilder builder6; - builder6 << std::numeric_limits::infinity(); - EXPECT_EQ("inf", std::string(builder6.c_str())); - - // Negative Infinity - CStringBuilder builder7; - builder7 << -std::numeric_limits::infinity(); - EXPECT_EQ("-inf", std::string(builder7.c_str())); - - // max() - CStringBuilder builder8; - builder8 << std::numeric_limits::max(); - EXPECT_EQ("1.79769e+308", std::string(builder8.c_str())); - - // NaN - CStringBuilder builder9; - builder9 << nan(""); - EXPECT_EQ("NaN", std::string(builder9.c_str())); -} - -TEST(CStringBuilderTestPA, FillBuffer) { - CStringBuilder builder1; - FillBuffer(builder1, ' ', CStringBuilder::kBufferSize * 2); - EXPECT_EQ(CStringBuilder::kBufferSize - 1, strlen(builder1.c_str())); - - CStringBuilder builder2; - FillBuffer(builder2, 3.141592653, CStringBuilder::kBufferSize * 2); - EXPECT_EQ(CStringBuilder::kBufferSize - 1, strlen(builder2.c_str())); - - CStringBuilder builder3; - FillBuffer(builder3, 3.14159f, CStringBuilder::kBufferSize * 2); - EXPECT_EQ(CStringBuilder::kBufferSize - 1, strlen(builder3.c_str())); - - CStringBuilder builder4; - FillBuffer(builder4, 65535u, CStringBuilder::kBufferSize * 2); - EXPECT_EQ(CStringBuilder::kBufferSize - 1, strlen(builder4.c_str())); - - CStringBuilder builder5; - FillBuffer(builder5, "Dummy Text", CStringBuilder::kBufferSize * 2); - EXPECT_EQ(CStringBuilder::kBufferSize - 1, strlen(builder5.c_str())); -} - -TEST(CStringBuilderTestPA, Pointer) { - CStringBuilder builder1; - char* str = reinterpret_cast(0x80000000u); - void* ptr = str; - builder1 << ptr; - EXPECT_EQ("0x80000000", std::string(builder1.c_str())); - - CStringBuilder builder2; - builder2 << reinterpret_cast(0xdeadbeafu); - EXPECT_EQ("0xDEADBEAF", std::string(builder2.c_str())); - - // nullptr - CStringBuilder builder3; - builder3 << nullptr; - EXPECT_EQ("nullptr", std::string(builder3.c_str())); - - CStringBuilder builder4; - builder4 << reinterpret_cast(0); - EXPECT_EQ("(nil)", std::string(builder4.c_str())); -} - -} // namespace partition_alloc::internal::base::strings