[ruby/psych] Reject a reentrant Psych::Parser#parse

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.

https://github.com/ruby/psych/commit/5f2e37dc1c

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

ext/psych/psych_parser.c

@@ -52,19 +52,28 @@ static int io_reader(void * data, unsigned char *buf, size_t size, size_t *read)
     return 1;
 }
 
+/* The parser calls back into Ruby for every event, so a handler can call
+ * Psych::Parser#parse again on the same object.  parse() reinitialises the
+ * parser it is handed, which would pull the input out from under the loop
+ * still driving it, so keep a flag to reject a reentrant call. */
+typedef struct {
+    yaml_parser_t yaml_parser;
+    int parsing;
+} psych_parser_t;
+
 static void dealloc(void * ptr)
 {
-    yaml_parser_t * parser;
+    psych_parser_t * parser;
 
-    parser = (yaml_parser_t *)ptr;
-    yaml_parser_delete(parser);
+    parser = (psych_parser_t *)ptr;
+    yaml_parser_delete(&parser->yaml_parser);
     xfree(parser);
 }
 
 #if 0
 static size_t memsize(const void *ptr)
 {
-    const yaml_parser_t *parser = ptr;
+    const psych_parser_t *parser = ptr;
     /* TODO: calculate parser's size */
     return 0;
 }
@@ -81,10 +90,10 @@ static const rb_data_type_t psych_parser_type = {
 
 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;
 }
@@ -257,9 +266,22 @@ static VALUE protected_event_location(VALUE pointer)
     return rb_funcall3(args[0], id_event_location, 4, args + 1);
 }
 
-static VALUE parse(VALUE self, VALUE handler, VALUE yaml, VALUE path)
+struct parse_args {
+    psych_parser_t * psych_parser;
+    VALUE self;
+    VALUE handler;
+    VALUE yaml;
+    VALUE path;
+};
+
+static VALUE parse_body(VALUE ptr)
 {
-    yaml_parser_t * parser;
+    struct parse_args * pargs = (struct parse_args *)ptr;
+    yaml_parser_t * parser = &pargs->psych_parser->yaml_parser;
+    VALUE self = pargs->self;
+    VALUE handler = pargs->handler;
+    VALUE yaml = pargs->yaml;
+    VALUE path = pargs->path;
     yaml_event_t event;
     int done = 0;
     int state = 0;
@@ -267,8 +289,6 @@ static VALUE parse(VALUE self, VALUE handler, VALUE yaml, VALUE path)
     int encoding = rb_utf8_encindex();
     rb_encoding * internal_enc = rb_default_internal_encoding();
 
-    TypedData_Get_Struct(self, yaml_parser_t, &psych_parser_type, parser);
-
     yaml_parser_delete(parser);
     yaml_parser_initialize(parser);
 
@@ -518,6 +538,37 @@ static VALUE parse(VALUE self, VALUE handler, VALUE yaml, VALUE path)
     return self;
 }
 
+static VALUE parse_ensure(VALUE ptr)
+{
+    psych_parser_t * parser = (psych_parser_t *)ptr;
+
+    parser->parsing = 0;
+
+    return Qnil;
+}
+
+static VALUE parse(VALUE self, VALUE handler, VALUE yaml, VALUE path)
+{
+    psych_parser_t * parser;
+    struct parse_args pargs;
+
+    TypedData_Get_Struct(self, psych_parser_t, &psych_parser_type, parser);
+
+    if (parser->parsing) {
+        rb_raise(rb_const_get(mPsych, rb_intern("Exception")),
+                "parser is already parsing, it cannot be reused from a handler callback");
+    }
+    parser->parsing = 1;
+
+    pargs.psych_parser = parser;
+    pargs.self         = self;
+    pargs.handler      = handler;
+    pargs.yaml         = yaml;
+    pargs.path         = path;
+
+    return rb_ensure(parse_body, (VALUE)&pargs, parse_ensure, (VALUE)parser);
+}
+
 /*
  * call-seq:
  *    parser.mark # => #<Psych::Parser::Mark>
@@ -529,13 +580,13 @@ static VALUE mark(VALUE self)
 {
     VALUE mark_klass;
     VALUE args[3];
-    yaml_parser_t * parser;
+    psych_parser_t * parser;
 
-    TypedData_Get_Struct(self, yaml_parser_t, &psych_parser_type, parser);
+    TypedData_Get_Struct(self, psych_parser_t, &psych_parser_type, parser);
     mark_klass = rb_const_get_at(cPsychParser, rb_intern("Mark"));
-    args[0] = SIZET2NUM(parser->mark.index);
-    args[1] = SIZET2NUM(parser->mark.line);
-    args[2] = SIZET2NUM(parser->mark.column);
+    args[0] = SIZET2NUM(parser->yaml_parser.mark.index);
+    args[1] = SIZET2NUM(parser->yaml_parser.mark.line);
+    args[2] = SIZET2NUM(parser->yaml_parser.mark.column);
 
     return rb_class_new_instance(3, args, mark_klass);
 }

ext/psych/psych_parser_fy.c

@@ -41,6 +41,11 @@ typedef struct {
     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;
 
 static const struct fy_parse_cfg psych_parse_cfg = {
@@ -256,17 +261,28 @@ static VALUE token_to_str(struct fy_token *tok, int encoding, rb_encoding *inter
     return str;
 }
 
-static VALUE parse(VALUE self, VALUE handler, VALUE yaml, VALUE path)
-{
+struct parse_args {
     psych_fy_parser_t *parser;
+    VALUE self;
+    VALUE handler;
+    VALUE yaml;
+    VALUE path;
+};
+
+static VALUE parse_body(VALUE ptr)
+{
+    struct parse_args *pargs = (struct parse_args *)ptr;
+    psych_fy_parser_t *parser = pargs->parser;
+    VALUE self = pargs->self;
+    VALUE handler = pargs->handler;
+    VALUE yaml = pargs->yaml;
+    VALUE path = pargs->path;
     struct fy_event *event;
     int done = 0;
     int state = 0;
     int encoding = rb_utf8_encindex();
     rb_encoding *internal_enc = rb_default_internal_encoding();
 
-    TypedData_Get_Struct(self, psych_fy_parser_t, &psych_parser_type, parser);
-
     /* Use a pristine parser for each parse, like fy-tool does.  Reusing a
      * parser across documents via fy_parser_reset() left the default tag
      * handles unset for bare (no "---") tag-led documents. */
@@ -496,6 +512,37 @@ static VALUE parse(VALUE self, VALUE handler, VALUE yaml, VALUE path)
     return self;
 }
 
+static VALUE parse_ensure(VALUE ptr)
+{
+    psych_fy_parser_t *parser = (psych_fy_parser_t *)ptr;
+
+    parser->parsing = 0;
+
+    return Qnil;
+}
+
+static VALUE parse(VALUE self, VALUE handler, VALUE yaml, VALUE path)
+{
+    psych_fy_parser_t *parser;
+    struct parse_args pargs;
+
+    TypedData_Get_Struct(self, psych_fy_parser_t, &psych_parser_type, parser);
+
+    if (parser->parsing) {
+        rb_raise(rb_const_get(mPsych, rb_intern("Exception")),
+                "parser is already parsing, it cannot be reused from a handler callback");
+    }
+    parser->parsing = 1;
+
+    pargs.parser  = parser;
+    pargs.self    = self;
+    pargs.handler = handler;
+    pargs.yaml    = yaml;
+    pargs.path    = path;
+
+    return rb_ensure(parse_body, (VALUE)&pargs, parse_ensure, (VALUE)parser);
+}
+
 /*
  * call-seq:
  *    parser.mark # => #<Psych::Parser::Mark>

test/psych/test_parser.rb

@@ -26,6 +26,39 @@ module Psych
       end
     end
 
+    # Calls Parser#parse again, once, from inside a callback of the parse it is
+    # already handling.
+    class ReentrantHandler < Handler
+      attr_accessor :parser, :inner_yaml
+      attr_reader :inner_error, :scalars, :empty_calls
+
+      def initialize
+        @parser      = nil
+        @inner_yaml  = nil
+        @inner_error = nil
+        @scalars     = []
+        @empty_calls = 0
+      end
+
+      def empty
+        @empty_calls += 1
+        raise "handler#empty keeps being called, the parse loop is not terminating" if @empty_calls > 1000
+      end
+
+      def scalar value, anchor, tag, plain, quoted, style
+        @scalars << value
+
+        inner, @inner_yaml = @inner_yaml, nil
+        return unless inner
+
+        begin
+          @parser.parse inner
+        rescue => e
+          @inner_error = e
+        end
+      end
+    end
+
     def setup
       super
       @handler        = EventCatcher.new
@@ -84,6 +117,42 @@ module Psych
       end
     end
 
+    def test_parse_is_not_reentrant
+      pend "Failing on JRuby" if RUBY_PLATFORM =~ /java/
+
+      handler = ReentrantHandler.new
+      handler.inner_yaml = "--- inner\n"
+      parser = Psych::Parser.new handler
+      handler.parser = parser
+
+      parser.parse "--- outer\n"
+
+      assert_kind_of Psych::Exception, handler.inner_error
+      assert_equal ['outer'], handler.scalars
+      assert_equal 0, handler.empty_calls
+
+      # The in-use flag is cleared when the parse finishes, so the same parser
+      # can be used again afterwards.
+      handler.scalars.clear
+      parser.parse "--- second\n"
+      assert_equal ['second'], handler.scalars
+    end
+
+    def test_parse_is_not_reentrant_with_invalid_inner_document
+      pend "Failing on JRuby" if RUBY_PLATFORM =~ /java/
+
+      handler = ReentrantHandler.new
+      handler.inner_yaml = "--- \x00bad\n"
+      parser = Psych::Parser.new handler
+      handler.parser = parser
+
+      parser.parse "--- outer\n"
+
+      assert_kind_of Psych::Exception, handler.inner_error
+      assert_equal ['outer'], handler.scalars
+      assert_equal 0, handler.empty_calls
+    end
+
     def test_multiparse
       3.times do
         @parser.parse '--- foo'