Firestore: string_format.h: fix use-after-free bug when internally formatting strings (#14306)

Author: dconeybe

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<T>` is
   `Sendable` if `T` is `Sendable`. (#14042)

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 <typename T>
-  FormatArg(T&& value)  // NOLINT(runtime/explicit)
-      : FormatArg{std::forward<T>(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<T>(value), std::move(sink),
+                  internal::FormatChoice<0>{}} {
   }
 
  private:
@@ -79,7 +88,9 @@ class FormatArg : public absl::AlphaNum {
    */
   template <typename T,
             typename = typename std::enable_if<std::is_same<bool, T>{}>::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,15 +101,19 @@ class FormatArg : public absl::AlphaNum {
   template <
       typename T,
       typename = typename std::enable_if<objc::is_objc_pointer<T>{}>::type>
-  FormatArg(T object, internal::FormatChoice<1>)
+  FormatArg(T object,
+            absl::strings_internal::StringifySink&&,
+            internal::FormatChoice<1>)
       : AlphaNum(MakeStringView([object description])) {
   }
 
   /**
    * 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,15 +123,20 @@ 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") {
   }
 
   /**
    * Creates a FormatArg from a character string literal. This is
    * 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,16 +145,21 @@ class FormatArg : public absl::AlphaNum {
    * hexadecimal integer literal.
    */
   template <typename T>
-  FormatArg(T* pointer_value, internal::FormatChoice<4>)
-      : AlphaNum(absl::Hex(reinterpret_cast<uintptr_t>(pointer_value))) {
+  FormatArg(T* pointer_value,
+            absl::strings_internal::StringifySink&& sink,
+            internal::FormatChoice<4>)
+      : AlphaNum(absl::Hex(reinterpret_cast<uintptr_t>(pointer_value)),
+                 std::move(sink)) {
   }
 
   /**
    * As a final fallback, creates a FormatArg from any type of value that
    * absl::AlphaNum accepts.
    */
   template <typename T>
-  FormatArg(T&& value, internal::FormatChoice<5>)
+  FormatArg(T&& value,
+            absl::strings_internal::StringifySink&&,
+            internal::FormatChoice<5>)
       : AlphaNum(std::forward<T>(value)) {
   }
 };

Firestore/core/test/unit/util/string_format_test.cc

@@ -16,13 +16,39 @@
 
 #include "Firestore/core/src/util/string_format.h"
 
+#include <algorithm>
+#include <sstream>
+#include <string>
+
 #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 <typename T>
+std::string hex_address(const T* ptr) {
+  std::ostringstream os;
+  os << std::hex << reinterpret_cast<uintptr_t>(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,