From 3ed146837d490159a9ce04540f74369cf6f5b488 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Thu, 6 Aug 2026 11:51:24 -0700 Subject: [PATCH] Don't alloca a buffer sized by the document S_ALLOCA_N is plain alloca (syck.h:52), and two call sites size it from data the parsed document controls, with no upper bound: * rubyext.c syck_set_ivars -- sized by an ivar NAME, one alloca per key, reached from Syck.load via `!ruby/object:Foo` plus a mapping. * rubyext.c rb_syck_compile -- sized by the whole compiled buffer, reached from the public Syck.compile. Both become ALLOCV_N, which uses alloca for small sizes and a Ruby-managed heap buffer once the size is worth it, and frees on unwind -- so the two callers stay exception-safe across rb_iv_set and rb_str_new, which the plain malloc/free version would not be. Measured on a Thread (1 MiB stack -- the shape a web or job worker actually has), ruby 3.4.10 arm64-darwin23, 3 runs per cell, against two separately built .bundles: before after ivar name 4 MiB SystemStackError ok ivar name 16 MiB SystemStackError ok Syck.compile 4 MiB SystemStackError ok Syck.compile 16 MiB SystemStackError ok Ruby's own stack-limit check is what currently turns these into a rescuable SystemStackError rather than a fault, on both platforms I could test -- so this is a robustness fix, not a security fix, and I would rather say so than overstate it. The reason to make it anyway is that the protection is Ruby's and not syck's: alloca with an attacker-controlled size is a stack-clash primitive, and whether it lands in the guard page or in something writable depends on mapping layout, platform, thread stack size and frame layout, none of which this code controls or tests. No behaviour change for any document that worked before. A large name or a large document now allocates on the heap and raises NoMemoryError on exhaustion instead of consuming stack. test_large_input.rb covers both, on a Thread. Against the unpatched build both error with SystemStackError; with this change both pass. Full suite: 123 tests, 394 assertions, 0 failures. --- ext/syck/rubyext.c | 23 +++++++++++++++-------- test/test_large_input.rb | 26 ++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 8 deletions(-) create mode 100644 test/test_large_input.rb diff --git a/ext/syck/rubyext.c b/ext/syck/rubyext.c index 4d7f7e2..3c31e74 100644 --- a/ext/syck/rubyext.c +++ b/ext/syck/rubyext.c @@ -148,6 +148,8 @@ rb_syck_compile(VALUE self, VALUE port) SYMID oid; int taint; char *ret; + long blen; + VALUE ret_v; VALUE bc; bytestring_t *sav = NULL; void *data = NULL; @@ -164,14 +166,15 @@ rb_syck_compile(VALUE self, VALUE port) } sav = data; - ret = S_ALLOCA_N( char, strlen( sav->buffer ) + 3 ); - ret[0] = '\0'; - strcat( ret, "D\n" ); - strcat( ret, sav->buffer ); + blen = (long)strlen( sav->buffer ); + ret = ALLOCV_N( char, ret_v, blen + 3 ); + memcpy( ret, "D\n", 2 ); + memcpy( ret + 2, sav->buffer, (size_t)blen + 1 ); syck_free_parser( parser ); - bc = rb_str_new2( ret ); + bc = rb_str_new( ret, blen + 2 ); + ALLOCV_END( ret_v ); if ( taint ) OBJ_TAINT( bc ); return bc; } @@ -1213,13 +1216,17 @@ syck_set_ivars( ) { VALUE ivname = rb_ary_entry( vars, 0 ); + VALUE ivn_v; char *ivn; + long ivn_len; StringValue( ivname ); - ivn = S_ALLOCA_N( char, RSTRING_LEN(ivname) + 2 ); + ivn_len = RSTRING_LEN(ivname); + ivn = ALLOCV_N( char, ivn_v, ivn_len + 2 ); ivn[0] = '@'; - ivn[1] = '\0'; - strncat( ivn, RSTRING_PTR(ivname), RSTRING_LEN(ivname) ); + memcpy( ivn + 1, RSTRING_PTR(ivname), (size_t)ivn_len ); + ivn[ivn_len + 1] = '\0'; rb_iv_set( obj, ivn, rb_ary_entry( vars, 1 ) ); + ALLOCV_END( ivn_v ); return Qnil; } diff --git a/test/test_large_input.rb b/test/test_large_input.rb new file mode 100644 index 0000000..e66f49e --- /dev/null +++ b/test/test_large_input.rb @@ -0,0 +1,26 @@ +require 'helper' + +module Syck + # Both of these sized an `alloca` from the document. They are run on a Thread + # because a Ruby thread's stack is far smaller than the main stack, which is + # the shape a web or job worker actually has. + class TestLargeInput < Test::Unit::TestCase + SIZE = 4 * 1024 * 1024 + + def on_thread + Thread.new { yield }.value + end + + def test_long_ivar_name + doc = "--- !ruby/object:Object\n" + ('n' * SIZE) + ": 1\n" + obj = on_thread { Syck.load(doc) } + assert_equal 1, obj.instance_variable_get("@#{'n' * SIZE}") + end + + def test_long_compile_input + doc = "--- \n" + (0...(SIZE / 20)).map { |i| "k#{i}: v#{i}\n" }.join + bc = on_thread { Syck.compile(doc) } + assert_equal "D\n", bc[0, 2] + end + end +end