Repository navigation
Zero the document in yaml_document_initialize on failure - #353
Closed
afonsojanu wants to merge 1 commit into
Closed
afonsojanu wants to merge 1 commit into
afonsojanu wants to merge 1 commit into
Conversation
yaml_parser_initialize() and yaml_emitter_initialize() both memset their object to zero before doing any fallible work, so a caller can always safely pass the object to the matching _delete() function even after a failed _initialize() call. yaml_document_initialize() was missing this: if it failed partway through, for example because a supplied tag directive was not valid UTF-8, it returned 0 without ever touching *document. A caller that follows the same defensive pattern used everywhere else in this library, initialize then unconditionally delete regardless of the return value, ends up calling yaml_document_delete() on a document that still holds whatever was previously on the stack. That is exactly what the OSS-Fuzz dumper harness for this project does, and it is how issue #312 (SEGV in yaml_free at api.c:53) reproduces: yaml_document_delete() dereferences the garbage nodes.start/nodes.top left over from an earlier stack frame and crashes trying to free it. I confirmed this with ASan/UBSan against the exact commit and input attached to #312, and the crash still reproduces on current master. Adding the same memset() used by the other two _initialize() functions makes a failed document safely deletable, matching the guarantee the rest of the API already provides. Added tests/test-document-initialize.c, which builds a tag directive with an invalid UTF-8 prefix to force the failure path, then checks the document is left zeroed and that deleting it doesn't crash.
Author
|
Closing this one too, same reason: starting fresh and clearing out everything I had open. |
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.
yaml_parser_initialize() and yaml_emitter_initialize() both memset their object to zero before doing any fallible work, which is what lets a caller safely pass the object to the matching
_delete()function even after a failed_initialize()call.yaml_document_initialize()was missing this: when it fails partway through (for example when a supplied tag directive isn't valid UTF-8), it returns 0 without ever writing to*document.A caller that follows the same pattern used elsewhere in this library and around the codebase, initialize then unconditionally delete regardless of the return value, ends up calling
yaml_document_delete()on a document that still holds whatever was previously on the stack. That's exactly what the OSS-Fuzz dumper harness for this project does, and it's how #312 reproduces:yaml_document_delete()dereferences the leftovernodes.start/nodes.topfrom an earlier stack frame and crashes trying to free it.I reproduced this with ASan/UBSan using the exact commit and POC attached to #312, and it still crashes the same way on current master:
With tracing added to the harness, the crash happens right after
copy_document()fails on the very first document (yaml_document_initialize()rejects a tag directive with invalid UTF-8), and the fuzzer's cleanup loop then deletes that never-initialized document. Adding the samememset()the other two_initialize()functions already use fixes it: after the change the exact same input runs clean under ASan/UBSan.Added
tests/test-document-initialize.c, which builds a tag directive with an invalid UTF-8 prefix to force the failure path, then asserts the document comes back zeroed and that deleting it doesn't crash. It's wired into bothtests/Makefile.amandtests/CMakeLists.txtalongside the existing tests. I ran it both ways: it fails (assertion on the memcmp) without the fix and passes with it, and the full CMake test suite (version,reader,nesting,document-initialize) passes.Fixes #312.