Do not use alloca for a buffer that the document sizes - #63
Open
jeremy wants to merge 1 commit into
Open
Conversation
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.
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.
Two functions call
S_ALLOCA_Nwith a size that comes from the document.S_ALLOCA_Nisalloca(syck.h:52). The size has no limit.syck_set_ivars— the size is an instance variable name.Syck.loadcalls this function one time for each key.rb_syck_compile— the size is the full compiled buffer.Syck.compilecalls this function.Change
Both functions now use
ALLOCV_N.ALLOCV_Nusesallocafor a small size. It uses a Ruby heap buffer for a large size. It frees the buffer if the function raises.rb_iv_setandrb_str_newcan raise, so this behavior is necessary.A document that was correct before is still correct.
Severity
This is a robustness change, not a security fix. On Ruby 3.4 and 4.0, the stack limit stops the large
allocafirst. The result is aSystemStackError, and you can rescue it.SystemStackErrorSyck.compileinputSystemStackErrorI could not cause a hard crash on syck 1.6.1, on arm64-darwin or x86_64-linux, from 1 MiB to 64 MiB. But the protection is from Ruby, not from syck. An
allocawith an attacker-controlled size is a stack-clash pattern. The result depends on the platform and the memory layout.Test
test_large_input.rbruns both functions on aThread. Each test fails withSystemStackErroron the current code. Each test passes with this change. Full suite: 123 tests, 0 failures.