From aa733f5f0841fe725e5c853ad307aaf1f3784714 Mon Sep 17 00:00:00 2001 From: Hiroshi SHIBATA Date: Tue, 28 Jul 2026 14:36:02 +0900 Subject: [PATCH] Stop overwriting the placeholder's heap slot for recursive anchors 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. --- ext/syck/rubyext.c | 130 +++++++++++++++++++++++++++++++++++---------- test/test_yaml.rb | 34 ++++++++++++ 2 files changed, 137 insertions(+), 27 deletions(-) diff --git a/ext/syck/rubyext.c b/ext/syck/rubyext.c index ff0927f..4d7f7e2 100644 --- a/ext/syck/rubyext.c +++ b/ext/syck/rubyext.c @@ -13,29 +13,6 @@ #include #include -typedef struct RVALUE { - union { -#if 0 - struct { - unsigned long flags; /* always 0 for freed obj */ - struct RVALUE *next; - } free; -#endif - struct RBasic basic; - struct RObject object; - /*struct RClass klass;*/ - /*struct RFloat flonum;*/ - /*struct RString string;*/ - struct RArray array; - /*struct RRegexp regexp;*/ - /*struct RHash hash;*/ - /*struct RData data;*/ - /*struct RStruct rstruct;*/ - /*struct RBignum bignum;*/ - /*struct RFile file;*/ - } as; -} RVALUE; - typedef struct { long hash; char *buffer; @@ -705,6 +682,99 @@ yaml_org_handler( SyckNode *n, VALUE *ref ) } static void syck_node_mark( SyckNode *n ); +static VALUE id_hash_new(void); + +/* + * Rewrite every reference to `from` inside `obj` so it points at `to`. + * + * A self-referential anchor (`--- &a\n- *a\n`) is aliased before the anchored + * node is finished, so the parser hands out a BadAlias placeholder and only + * later learns which object the anchor really is. Only containers can hold a + * placeholder, so the walk stops at anything else. `seen` must compare by + * identity, otherwise equal-but-distinct nodes collapse and cycles that are + * already in place would recurse forever. + */ +static void +syck_swap_placeholder(VALUE obj, VALUE from, VALUE to, VALUE seen) +{ + long i; + VALUE ivars; + + if ( SPECIAL_CONST_P( obj ) || OBJ_FROZEN( obj ) ) return; + + switch ( BUILTIN_TYPE( obj ) ) + { + case T_ARRAY: + case T_HASH: + case T_STRUCT: + case T_OBJECT: + break; + + default: + return; + } + + if ( RTEST( rb_hash_lookup( seen, obj ) ) ) return; + rb_hash_aset( seen, obj, Qtrue ); + + switch ( BUILTIN_TYPE( obj ) ) + { + case T_ARRAY: + for ( i = 0; i < RARRAY_LEN( obj ); i++ ) + { + VALUE v = rb_ary_entry( obj, i ); + if ( v == from ) rb_ary_store( obj, i, to ); + else syck_swap_placeholder( v, from, to, seen ); + } + break; + + case T_HASH: + { + VALUE keys = rb_funcall( obj, s_keys, 0 ); + for ( i = 0; i < RARRAY_LEN( keys ); i++ ) + { + VALUE key = rb_ary_entry( keys, i ); + VALUE val = rb_hash_aref( obj, key ); + + if ( key == from || val == from ) + { + VALUE new_key = key == from ? to : key; + VALUE new_val = val == from ? to : val; + + if ( new_key != key ) rb_hash_delete( obj, key ); + rb_hash_aset( obj, new_key, new_val ); + key = new_key; + val = new_val; + } + + syck_swap_placeholder( key, from, to, seen ); + syck_swap_placeholder( val, from, to, seen ); + } + } + break; + + case T_STRUCT: + for ( i = 0; i < RSTRUCT_LEN( obj ); i++ ) + { + VALUE v = rb_struct_aref( obj, LONG2NUM( i ) ); + if ( v == from ) rb_struct_aset( obj, LONG2NUM( i ), to ); + else syck_swap_placeholder( v, from, to, seen ); + } + break; + + default: + break; + } + + ivars = rb_obj_instance_variables( obj ); + for ( i = 0; i < RARRAY_LEN( ivars ); i++ ) + { + ID name = SYM2ID( rb_ary_entry( ivars, i ) ); + VALUE v = rb_ivar_get( obj, name ); + if ( v == from ) rb_ivar_set( obj, name, to ); + else syck_swap_placeholder( v, from, to, seen ); + } +} /* * {native mode} node handler @@ -738,13 +808,19 @@ rb_syck_load_handler(SyckParser *p, SyckNode *n) DATA_PTR( node_wrapper ) = NULL; /* - * ID already set, let's alter the symbol table to accept the new object + * A placeholder was handed out for this node while it was still being + * parsed. Syck used to fuse the two by copying the real object over the + * placeholder's heap slot, which is wrong on every count the GC cares + * about: the 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. The heap + * then stays corrupted until some unrelated allocation crashes. Rewrite + * the references instead. */ if (n->id > 0 && !NIL_P(obj)) { - MEMCPY((void *)n->id, (void *)obj, RVALUE, 1); - MEMZERO((void *)obj, RVALUE, 1); - obj = n->id; + syck_swap_placeholder( obj, (VALUE)n->id, obj, id_hash_new() ); + n->id = obj; } if ( bonus->taint) OBJ_TAINT( obj ); diff --git a/test/test_yaml.rb b/test/test_yaml.rb index 73553e4..931f3e4 100644 --- a/test/test_yaml.rb +++ b/test/test_yaml.rb @@ -1337,6 +1337,40 @@ def test_circular_references assert_equal( inspect_str, Syck::load( a.to_yaml ).inspect ) end + # + # A self-referential anchor is aliased before the anchored node exists, so + # the parser hands out a placeholder and patches it up afterwards. That + # used to be done by overwriting the placeholder's heap slot, which left + # the GC heap corrupted and crashed at some later allocation. + # + def test_self_referential_anchor + [ 1, 3, 6, 50 ].each do |n| + seq = Syck::load( "--- &id001\n" + "- *id001\n" * n ) + assert_equal( n, seq.size ) + seq.each { |item| assert_same( seq, item ) } + end + + map = Syck::load( "--- &id001\nself: *id001\n" ) + assert_same( map, map["self"] ) + + keyed = Syck::load( "--- &id001\n? *id001\n: 1\n" ) + assert_same( keyed, keyed.keys.first ) + + # The alias appears as a key and again below the matching value, so + # rewriting the key must not stop the walk from reaching the rest. + both = Syck::load( "--- &id001\n? *id001\n: \n - *id001\n" ) + key, value = both.first + assert_same( both, key ) + assert_same( both, value[0] ) + + nested = Syck::load( "--- &id001\n- &id002\n - *id001\n- *id002\n" ) + assert_same( nested, nested[0][0] ) + assert_same( nested[0], nested[1] ) + + GC.start + assert_nothing_raised { GC.verify_internal_consistency } + end + # # Test Symbol cycle #