Fix three defects in Psych::Parser#parse - #817
Merged
Merged
Conversation
Every case of the event switch overwrote the rb_protect state, so only the second result was ever tested and the state from the event_location call was dropped. The parse then returned normally with the exception silently discarded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Once libyaml has set stream_end_produced, every further yaml_parser_parse succeeds with a zeroed event, so YAML_STREAM_END_EVENT can no longer be reached and the loop calls handler#empty forever at full CPU. A parser left in that state should terminate instead. No test covers this directly because the loop only reaches it through a reentrant parse, which the parser now rejects outright. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The parser calls back into Ruby for every event, so a handler can call #parse again on the same object. That reinitialised the parser and re-pointed it at new input while the outer loop was still driving it, which left the outer loop reading the inner document's freed buffer, or resuming on a parser with no input at all and tripping libyaml's read_handler assertion. Psych::Exception is used so the caller can rescue it, and the in-use flag is cleared with rb_ensure so a parser stays reusable afterwards. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens Psych::Parser#parse against three low-level correctness defects in both the libyaml and libfyaml backends: (1) exceptions raised from Handler#event_location not reliably propagating, (2) unsafe reentrant parse calls from within callbacks corrupting parser state, and (3) a potential infinite loop when the backend returns an event with no type.
Changes:
- Ensure exceptions from
event_locationare immediately surfaced (and associated event memory is released before raising). - Add a per-parser “in use” guard to reject reentrant
parsecalls, and clear it viarb_ensureto keep parsers reusable. - Stop the parse loop when the backend returns a “no event” type to avoid spinning indefinitely.
- Add tests for exception propagation and reentrancy behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
test/psych/test_parser.rb |
Adds regression tests covering event_location exception propagation and reentrant parse rejection/reuse. |
ext/psych/psych_parser.c |
Wraps yaml_parser_t in a struct with an in-use flag; fixes exception propagation and loop termination edge cases. |
ext/psych/psych_parser_fy.c |
Mirrors the same correctness fixes for the libfyaml backend (event_location propagation, non-reentrancy, loop termination). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
91
to
98
| static VALUE allocate(VALUE klass) | ||
| { | ||
| yaml_parser_t * parser; | ||
| VALUE obj = TypedData_Make_Struct(klass, yaml_parser_t, &psych_parser_type, parser); | ||
| psych_parser_t * parser; | ||
| VALUE obj = TypedData_Make_Struct(klass, psych_parser_t, &psych_parser_type, parser); | ||
|
|
||
| yaml_parser_initialize(parser); | ||
| yaml_parser_initialize(&parser->yaml_parser); | ||
|
|
||
| return obj; |
Comment on lines
39
to
49
| typedef struct { | ||
| struct fy_parser *fyp; | ||
| size_t mark_line; | ||
| size_t mark_column; | ||
| size_t mark_index; | ||
| /* The parser calls back into Ruby for every event, so a handler can call | ||
| * Psych::Parser#parse again on the same object. parse() destroys and | ||
| * recreates fyp, which would pull the parser out from under the loop still | ||
| * driving it, so keep a flag to reject a reentrant call. */ | ||
| int parsing; | ||
| } psych_fy_parser_t; |
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.
An exception raised in
Psych::Handler#event_locationnever reaches the caller. Every branch of the event switch overwrites therb_protectstate, so only the second result is tested.Calling
Psych::Parser#parsefrom inside a handler callback reinitialises the parser the outer loop is still driving. The outer loop then reads the inner document's freed buffer, or resumes with no input and trips libyaml'sread_handlerassertion, aborting the process.parsenow raisesPsych::Exceptionwhen the parser is already parsing, andrb_ensureclears the flag so the parser stays reusable.The loop also spun forever on an event with no type, which libyaml keeps returning once
stream_end_producedis set.None of this is reachable through
Psych.loadand friends, which build a fresh parser each time. The libfyaml backend shared all three and is fixed alongside.