Firestore: string_format.h: fix use-after-free bug when internally formatting strings #14306
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…extension of the StringifySink object
dconeybe
added a commit
that referenced
this pull request
Jan 6, 2025
…feMemoizerMemoryLeakFix This PR is now based on #14306
…SinkUseAfterFreeFix
cherylEnkidu
reviewed
Jan 9, 2025
…SinkUseAfterFreeFix
…lso check that the expected hex address is indeed contained in the actual formatted string.
|
AI(dconeybe) Add a TODO to improve this implementation in the future to avoid using the absl-internal class Update: Done: 72833ca |
…SinkUseAfterFreeFix
|
As an interesting observation, here is another use-after-free bug caught by address sanitizer that will be fixed by this PR: Namely, when formatting the For completeness, here is the report from ASAN: |
…from encountering the use-after-free bug.
cherylEnkidu
approved these changes
Jan 9, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes the following use-after-free bug detected by the test
TEST(StringFormatTest, Pointer)when run under ASAN:The real issue here is that Firestore is using the class
absl::AlphaNumin a way that it was not intended to be used. Namely,AlphaNumexplicitly documents thathttps://github.com/abseil/abseil-cpp/blob/506f1072c03dc35ba7bc447fd9651cc90e22e816/absl/strings/str_cat.h#L311C1-L315C45
However, the Firestore SDK uses it in a different way, by creating a subclass of it, named
FormatArg:firebase-ios-sdk/Firestore/core/src/util/string_format.h
Line 65 in c092c02
The problem is that one of the constructors of
FormatArgcalls through to a constructor ofAlphaNumthat relies on the lifetime extension of a C++ temporary used as function argument for a reference-type parameter. This works just fine whenAlphaNumis used as a direct argument toStrCat(), as is intended. However, when used in that way that Firestore does, theStringifySinktemporary argument is destroyed prematurely, causing a dangling reference to its internalstd::string, causing a use-after-free bug when that dangling reference is used.Although use-after-free is technically undefined behavior, it manifested in my case by this code
returning
"Hello \x90\x15#T\xFD\x7F\0\0\x88\x15#T"instead of the expected"Hello 7ffffdc94440". Notice how the text after "Hello" is "garbage" in the former case, but is the expected hex-formatted memory address in the latter case. The latter case was produced with the fix in this PR applied.This fix works by having the
StringifySinkbe created by the caller of theFormatArgconstructor (by virtue of it being a default parameter with a default value) and passing along a reference to thatStringifySink. This gains the correct lifetime extension of theStringifySinkobject and fixes the use-after-free.There are, however, two drawbacks to this fix: (a) the
StringifySinkclass is an internal, implementation detail of absl that really should not be used by Firestore and (b) it incurs the cost of creating and destroying aStringifySinkobject even for constructors that don't need it. With respect to (a), since we have pinned to a specific version of absl we should be good and with respect to (b) I hope that the compiler will elide theStringifySinkconstructor calls altogether when the object is not needed since its constructors and destructor have no apparent side effects.I had originally thought that this was a bug in absl; however, after discussion with the absl developers, the root cause was identified as unintended use of the
AlphaNumclass. Googlers can see b/388000898 for the whole discussion.