From 01061eaf757eab569b8f0037070f1de8e3188b85 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 9 Jan 2025 18:02:21 -0500 Subject: [PATCH] Firestore: string_format.h: fix use-after-free bug when internally formatting strings (#14306) --- Firestore/CHANGELOG.md | 3 ++ Firestore/core/src/util/string_format.h | 47 ++++++++++++++----- .../core/test/unit/util/string_format_test.cc | 45 ++++++++++++++++-- 3 files changed, 79 insertions(+), 16 deletions(-) diff --git a/Firestore/CHANGELOG.md b/Firestore/CHANGELOG.md index ee7ea12f87f..ad57793f111 100644 --- a/Firestore/CHANGELOG.md +++ b/Firestore/CHANGELOG.md @@ -1,3 +1,6 @@ +# Unreleased +- [fixed] Fixed use-after-free bug when internally formatting strings. (#14306) + # 11.6.0 - [fixed] Add conditional `Sendable` conformance so `ServerTimestamp` is `Sendable` if `T` is `Sendable`. (#14042) diff --git a/Firestore/core/src/util/string_format.h b/Firestore/core/src/util/string_format.h index 2322dba0138..ae905787f2a 100644 --- a/Firestore/core/src/util/string_format.h +++ b/Firestore/core/src/util/string_format.h @@ -62,11 +62,20 @@ struct FormatChoice<5> {}; * formatting of the value as an unsigned integer. * * Otherwise the value is interpreted as anything absl::AlphaNum accepts. */ -class FormatArg : public absl::AlphaNum { +class FormatArg final : public absl::AlphaNum { public: template - FormatArg(T&& value) // NOLINT(runtime/explicit) - : FormatArg{std::forward(value), internal::FormatChoice<0>{}} { + FormatArg( + T&& value, + // TODO(b/388888512) Remove the usage of StringifySink since it is not + // part of absl's public API. Moreover, subclassing AlphaNum is not + // supported either, so find a way to do this without these two caveats. + // See https://github.com/firebase/firebase-ios-sdk/pull/14331 for a + // partial proposal. + absl::strings_internal::StringifySink&& sink = + {}) // NOLINT(runtime/explicit) + : FormatArg{std::forward(value), std::move(sink), + internal::FormatChoice<0>{}} { } private: @@ -79,7 +88,9 @@ class FormatArg : public absl::AlphaNum { */ template {}>::type> - FormatArg(T bool_value, internal::FormatChoice<0>) + FormatArg(T bool_value, + absl::strings_internal::StringifySink&&, + internal::FormatChoice<0>) : AlphaNum(bool_value ? "true" : "false") { } @@ -90,7 +101,9 @@ class FormatArg : public absl::AlphaNum { template < typename T, typename = typename std::enable_if{}>::type> - FormatArg(T object, internal::FormatChoice<1>) + FormatArg(T object, + absl::strings_internal::StringifySink&&, + internal::FormatChoice<1>) : AlphaNum(MakeStringView([object description])) { } @@ -98,7 +111,9 @@ class FormatArg : public absl::AlphaNum { * Creates a FormatArg from any Objective-C Class type. Objective-C Class * types are a special struct that aren't of a type derived from NSObject. */ - FormatArg(Class object, internal::FormatChoice<1>) + FormatArg(Class object, + absl::strings_internal::StringifySink&&, + internal::FormatChoice<1>) : AlphaNum(MakeStringView(NSStringFromClass(object))) { } #endif @@ -108,7 +123,10 @@ class FormatArg : public absl::AlphaNum { * handled specially to avoid ambiguity with generic pointers, which are * handled differently. */ - FormatArg(std::nullptr_t, internal::FormatChoice<2>) : AlphaNum("null") { + FormatArg(std::nullptr_t, + absl::strings_internal::StringifySink&&, + internal::FormatChoice<2>) + : AlphaNum("null") { } /** @@ -116,7 +134,9 @@ class FormatArg : public absl::AlphaNum { * handled specially to avoid ambiguity with generic pointers, which are * handled differently. */ - FormatArg(const char* string_value, internal::FormatChoice<3>) + FormatArg(const char* string_value, + absl::strings_internal::StringifySink&&, + internal::FormatChoice<3>) : AlphaNum(string_value == nullptr ? "null" : string_value) { } @@ -125,8 +145,11 @@ class FormatArg : public absl::AlphaNum { * hexadecimal integer literal. */ template - FormatArg(T* pointer_value, internal::FormatChoice<4>) - : AlphaNum(absl::Hex(reinterpret_cast(pointer_value))) { + FormatArg(T* pointer_value, + absl::strings_internal::StringifySink&& sink, + internal::FormatChoice<4>) + : AlphaNum(absl::Hex(reinterpret_cast(pointer_value)), + std::move(sink)) { } /** @@ -134,7 +157,9 @@ class FormatArg : public absl::AlphaNum { * absl::AlphaNum accepts. */ template - FormatArg(T&& value, internal::FormatChoice<5>) + FormatArg(T&& value, + absl::strings_internal::StringifySink&&, + internal::FormatChoice<5>) : AlphaNum(std::forward(value)) { } }; diff --git a/Firestore/core/test/unit/util/string_format_test.cc b/Firestore/core/test/unit/util/string_format_test.cc index b9236017a98..dfbcc303b99 100644 --- a/Firestore/core/test/unit/util/string_format_test.cc +++ b/Firestore/core/test/unit/util/string_format_test.cc @@ -16,13 +16,39 @@ #include "Firestore/core/src/util/string_format.h" +#include +#include +#include + #include "absl/strings/string_view.h" +#include "gmock/gmock.h" #include "gtest/gtest.h" namespace firebase { namespace firestore { namespace util { +namespace { + +using testing::HasSubstr; +using testing::MatchesRegex; + +std::string lowercase(const std::string& str) { + std::string lower_str = str; + std::transform(lower_str.begin(), lower_str.end(), lower_str.begin(), + [](auto c) { return std::tolower(c); }); + return lower_str; +} + +template +std::string hex_address(const T* ptr) { + std::ostringstream os; + os << std::hex << reinterpret_cast(ptr); + return os.str(); +} + +} // namespace + TEST(StringFormatTest, Empty) { EXPECT_EQ("", StringFormat("")); EXPECT_EQ("", StringFormat("%s", std::string().c_str())); @@ -72,14 +98,23 @@ TEST(StringFormatTest, Bool) { EXPECT_EQ("Hello false", StringFormat("Hello %s", false)); } -TEST(StringFormatTest, Pointer) { - // pointers implicitly convert to bool. Make sure this doesn't happen in - // this API. - int value = 4; - EXPECT_NE("Hello true", StringFormat("Hello %s", &value)); +TEST(StringFormatTest, NullPointer) { + // pointers implicitly convert to bool. Make sure this doesn't happen here. EXPECT_EQ("Hello null", StringFormat("Hello %s", nullptr)); } +TEST(StringFormatTest, NonNullPointer) { + // pointers implicitly convert to bool. Make sure this doesn't happen here. + int value = 4; + + const std::string formatted_string = StringFormat("Hello %s", &value); + + const std::string hex_address_regex = "(0x)?[0123456789abcdefABCDEF]+"; + EXPECT_THAT(formatted_string, MatchesRegex("Hello " + hex_address_regex)); + const std::string expected_hex_address = lowercase(hex_address(&value)); + EXPECT_THAT(lowercase(formatted_string), HasSubstr(expected_hex_address)); +} + TEST(StringFormatTest, Mixed) { EXPECT_EQ("string=World, bool=true, int=42, float=1.5", StringFormat("string=%s, bool=%s, int=%s, float=%s", "World", true,