Reject a reentrant Psych::Parser#parse on JRuby - #821
Open
youdie006 wants to merge 1 commit into
Open
Conversation
ruby#817 added this guard to both C backends but not to the Java one, where the parser is an instance field: a handler calling parse again replaces the ParserImpl the outer loop is still draining, so the outer document is truncated and the inner document's events are spliced into it. Re-enables the two reentrancy tests that ruby#817 marked pending on JRuby.
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.
#817 fixed
Psych::Parser#parsereentrancy inext/psych/psych_parser.candext/psych/psych_parser_fy.c.ext/java/org/jruby/ext/psych/PsychParser.javais the third implementation of the same method and did not get the guard, which is why that PR marked both of its new testspend "Failing on JRuby".parseris an instance field (PsychParser.java:568), assigned at:241and driven by the loop at:248. A handler callingparseagain replaces theParserImplthe outer loop is still draining:delivers
["a", "inner"]with no error, where the C backends deliver["a", "b", "c"]and raisePsych::Exceptionout of the inner call.The change mirrors the C fix — same flag, same exception class, same message string,
finallystanding in forrb_ensure. No new test: #817 already wrote them, so this only deletes the twopendlines. The other fourpend "Failing on JRuby"lines in that file are a separatePsych::Parser#markgap and are untouched.Verification
Built in
jruby:9.4-jdk21against a compile of this repo's Java sources. Every row asserts whichPsychParserwas loaded, because with the classpath unset this silently tests JRuby's bundledpsych.jar.mastertest_parser.rb:128)masterfinally { parsing = false; }test_parser.rb:135That last row is the useful one: without the
finallythe reentrancy check still passes, and only the "reusable afterwards" assertion — the one #817's own comment describes — catches the poisoned flag.test/psychtest/psych/test_parser.rbThe removed
pends areRUBY_PLATFORM =~ /java/-guarded, so they were already no-ops on CRuby and the C sources are untouched; the same reasoning covers the truffleruby rows andlibfyaml.yml, which I did not execute. I compiled against snakeyaml-engine 2.9 while the repo pins 2.10; the patch touches no snakeyaml API.Disclosure: I used Claude (an AI assistant) while preparing this change. Every result above I ran and verified myself.