Fix segfault when a formatted insertion produces zero characters - #37
Open
afonsojanu wants to merge 1 commit into
Open
afonsojanu wants to merge 1 commit into
afonsojanu wants to merge 1 commit into
Conversation
cc_str_insert_wrapped_fmt_args (which backs push_fmt and insert_fmt) counts the total length of all formatted arguments, then makes room for them and shifts the string's tail (including the null terminator) to open up space. When that total comes out to zero, e.g. pushing an empty string into a string container that has never had any storage allocated, the "make room" check passes trivially (0 + 0 is not greater than 0), so no allocation happens, and the code falls through straight to the memmove and header update. For a freshly initialized string, the container still points at a shared, read-only placeholder used to represent an empty, unallocated string. Writing to it (even a self-copy of the terminator, or just incrementing size by zero) crashes. cc_str_insert_n already guards against this by returning early when the number of elements to insert is zero. This adds the same early return to cc_str_insert_wrapped_fmt_args, and a regression test covering push_fmt with an empty string for all three of CC's string element types.
|
Thanks for looking into the mechanics on how to fix this. +1 for copying a mechanic used elsewhere already. |
This file contains hidden or 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
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.
Fixes #36.
cc_str_insert_wrapped_fmt_args(the function behindpush_fmtandinsert_fmt) first counts up the total length of everything it needs to format and insert, then, if that total exceeds the current capacity, grows the buffer, and only then does the actual memmove/insert.The problem is when the total comes out to zero, for instance
push_fmt(&s, "")on a container that hasn't allocated anything yet.cc_str_size(cntr) + total > cc_str_cap(cntr)is0 + 0 > 0, which is false, so the growth step is skipped entirely. Execution falls straight through to the memmove and tocc_str_hdr(cntr)->size += total. For a never-allocated string,cntrstill points at CC's shared placeholder for an empty string, which lives in read-only memory, so both of those touch memory that can't be written to and the process crashes.cc_str_insert_nalready has a guard for exactly this situation: it returns immediately whenn == 0, before it ever looks at the container's capacity. I added the equivalent check tocc_str_insert_wrapped_fmt_args, right after the length-counting loop and before the growth step, so a zero-length formatted insertion is a no-op rather than falling through to code that assumes there's something to write.I reproduced the crash from the issue under ASan first (SIGBUS in
cc_str_insert_wrapped_fmt_args, matching what DevSolar described), confirmed the fix removes it, and added a test tounit_tests.ccoveringpush_fmtwith an empty string across all three of CC's string element types (char,char16_t,char32_t). The full unit test suite still passes clean under ASan/UBSan (including the fault-injection pass that simulates realloc failures), andtests_against_stl.cppstill compiles against the patched header.