(Hopefully) Fix use after free#54
Merged
Merged
Conversation
Closed
RDIL
added a commit
to RDIL/syck
that referenced
this pull request
Apr 18, 2026
While working on ruby#54, I found that ASAN flags this line because this function call tries to read 53 bytes of a 50-byte string. That's not right!
Merged
|
Strangely I had the same fix from Claude only a few days ago. The test program i wrote ran ok, but not had time to check this out in the main product yet. In my testing I found the check for n being null "if ( n == NULL ) return;" didn't seem to be required but there is no harm in it. |
hsbt
pushed a commit
to RDIL/syck
that referenced
this pull request
Jun 24, 2026
While working on ruby#54, I found that ASAN flags this line because this function call tries to read 53 bytes of a 50-byte string. That's not right!
hsbt
force-pushed
the
reece/use-after-free
branch
from
cf30f72 to
4dacaa3
Compare
Between v1.4.1 and v1.5.1.1, 6793d14 added a GC mark function to the `SyckNode` Data wrapper created inside `rb_syck_load_handler`. The change fixed one crash (GC collecting node children mid-parse) but opened a new one: the parser frees the node the moment the handler returns, so any later GC that still reaches the wrapper, via a conservative stack root, calls the mark function on a dangling pointer.
hsbt
force-pushed
the
reece/use-after-free
branch
from
4dacaa3 to
29aba52
Compare
Member
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
3 participants
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.
Disclaimer: this patch was written with the assistance of a large language model (Claude Opus 4.7 with 1M context). I am very unfamiliar with C, so it's completely possible that it has made any number of mistakes.
Between v1.4.1 and v1.5.1.1, #40 added a GC mark function to the
SyckNodeData wrapper created insiderb_syck_load_handler. The change fixed one crash (GC collecting node children mid-parse) but opened a new one: the parser frees the node the moment the handler returns, so any later GC that still reaches the wrapper, via a conservative stack root, calls the mark function on a dangling pointer.Possibly fixes #50 (it gets rid of it in my testing, but it's possible this has other implications I haven't thought of, or is only a partial fix)
cc @peterzhu2118
Resources used while making this PR: https://gist.github.com/RDIL/5e2bda4041ed3b804449472a9bf7d809
Note: build is blocked on #56