Fix heap corruption when a YAML anchor references itself - #61
Merged
Conversation
A self-referential anchor is aliased before the anchored node exists, so
the parser hands out a BadAlias placeholder and fused the two afterwards
by copying the real object over the placeholder's slot. That slot is
sized for the placeholder, the copied flags carry the source object's age
bits, no write barrier fires, and zeroing the source turns a live object
into T_NONE behind the GC's back. Rewrite the references instead.
test_circular_references alone left the live slot count off by one per
load, and the resulting crash surfaced at an unrelated allocation:
$ ruby -Ilib -e 'require "syck"
a = []; a[0] = a; a[1] = a; y = a.to_yaml
100.times { Syck.load(y) }
GC.start(full_mark: true, immediate_sweep: true)
GC.verify_internal_consistency'
[BUG] inconsistent live slot number: expect 20491, but 20391.
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
1 participant
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.
A self-referential anchor such as
--- &a\n- *a\nis aliased before the anchored node exists, so the parser hands out aBadAliasplaceholder and fuses the two afterwards by copying the real object over the placeholder's heap slot. That slot is sized for the placeholder, the copied flags carry the source object's age bits, no write barrier fires, and zeroing the source turns a live object intoT_NONEbehind the GC's back. The heap then stays corrupted until an unrelated allocation crashes, which is why the CI backtraces point atnewobj_initwith no syck frames anywhere in them.test_circular_referencestriggers this on every run and leaves the live slot count off by one per load.Six or more self-references make it deterministic.
ObjectSpace.dumpshows an array claiming"length":6, "embedded":trueinside a"slot_size":40slot, so the tail of the element list is whatever sits in the adjacent slots.This walks the finished subtree and rewrites references to the placeholder instead, covering arrays, hash keys and values, structs, and instance variables. Verified on Ruby 2.7, 3.1, 3.2, 3.3.11, 3.4, and 4.0. The new test segfaults on the current
masterbuild and passes with the fix.