From 039bc2cfbeac35557eeaeb769a2d715e21f41705 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Florin=20Cri=C8=99an?= Date: Tue, 15 Jun 2021 13:08:14 +0300 Subject: [PATCH 1/2] fix #7074 Safely handle setlocale `setlocale` returns a pointer to a buffer containing the current locale name. This needs to be copied into a `std::string` or it will be overwritten by the next call. Trying to call `setlocale` with a non-null, invalid pointer can have unpredictable results, such as ``` [ RUN ] StringPrintfTest.Multibyte minkernel\crts\ucrt\src\appcrt\convert\mbstowcs.cpp(246) : Assertion failed: (pwcs == nullptr && sizeInWords == 0) || (pwcs != nullptr && sizeInWords > 0) ``` `setlocale` can also return a `nullptr` if it fails, but we assert against that. --- src/google/protobuf/stubs/stringprintf_unittest.cc | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/google/protobuf/stubs/stringprintf_unittest.cc b/src/google/protobuf/stubs/stringprintf_unittest.cc index 37172a9d970..a42f9457df4 100644 --- a/src/google/protobuf/stubs/stringprintf_unittest.cc +++ b/src/google/protobuf/stubs/stringprintf_unittest.cc @@ -91,7 +91,9 @@ TEST(StringPrintfTest, Multibyte) { // out of memory while trying to determine destination buffer size. // see b/4194543. - char* old_locale = setlocale(LC_CTYPE, nullptr); + char* old_locale_c = setlocale(LC_CTYPE, nullptr); + ASSERT_TRUE(old_locale_c != nullptr); + std::string old_locale = old_locale_c; // Push locale with multibyte mode setlocale(LC_CTYPE, "en_US.utf8"); @@ -115,15 +117,17 @@ TEST(StringPrintfTest, Multibyte) { EXPECT_TRUE(value.empty() || value == buf); delete[] buf; - setlocale(LC_CTYPE, old_locale); + setlocale(LC_CTYPE, old_locale.c_str()); } TEST(StringPrintfTest, NoMultibyte) { // No multibyte handling, but the string contains funny chars. - char* old_locale = setlocale(LC_CTYPE, nullptr); + char* old_locale_c = setlocale(LC_CTYPE, nullptr); + ASSERT_TRUE(old_locale_c != nullptr); + std::string old_locale = old_locale_c; setlocale(LC_CTYPE, "POSIX"); std::string value = StringPrintf("%.*s", 3, "\375\067s"); - setlocale(LC_CTYPE, old_locale); + setlocale(LC_CTYPE, old_locale.c_str()); EXPECT_EQ("\375\067s", value); } From 64cb893a3045ea92a4a871b55ac53c9b3bc5851e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Florin=20Cri=C8=99an?= Date: Tue, 15 Jun 2021 13:19:33 +0300 Subject: [PATCH 2/2] stringprintf_unittest.cc: Replace `new char[n+1]` with `std::array` Prefer safer alternative to naked pointers. This is a follow-up to 1dd313cd0611ac13257c075a977d46c874c42652 --- src/google/protobuf/stubs/stringprintf_unittest.cc | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/google/protobuf/stubs/stringprintf_unittest.cc b/src/google/protobuf/stubs/stringprintf_unittest.cc index a42f9457df4..63f38bfe1eb 100644 --- a/src/google/protobuf/stubs/stringprintf_unittest.cc +++ b/src/google/protobuf/stubs/stringprintf_unittest.cc @@ -32,6 +32,7 @@ #include +#include #include #include @@ -108,14 +109,13 @@ TEST(StringPrintfTest, Multibyte) { // Repeat with longer string, to make sure that the dynamically // allocated path in StringAppendV is handled correctly. - int n = 2048; - char* buf = new char[n+1]; - memset(buf, ' ', n-3); - memcpy(buf + n - 3, kInvalidCodePoint, 4); - value = StringPrintf("%.*s", n, buf); + const size_t n = 2048; + std::array buf; + memset(&buf[0], ' ', n-3); + memcpy(&buf[0] + n - 3, kInvalidCodePoint, 4); + value = StringPrintf("%.*s", n, &buf[0]); // See GRTEv2 vs. GRTEv3 comment above. - EXPECT_TRUE(value.empty() || value == buf); - delete[] buf; + EXPECT_TRUE(value.empty() || value == &buf[0]); setlocale(LC_CTYPE, old_locale.c_str()); }