Fix three defects in Psych::Parser#parse

Gemfile

@@ -6,6 +6,6 @@ group :development do
   gem 'rake-compiler', ">= 0.4.1"
   gem 'ruby-maven', :platforms => :jruby
   gem 'test-unit'
-  gem 'test-unit-ruby-core', ">= 1.0.7"
+  gem 'test-unit-ruby-core', ">= 1.0.16"
   gem 'power_assert', '~> 2.0' if RUBY_VERSION < '3.0' # https://github.com/ruby/power_assert/pull/61
 end

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,18 +266,29 @@ 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;
     int parser_encoding = YAML_ANY_ENCODING;
     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);
 
@@ -312,6 +332,10 @@ static VALUE parse(VALUE self, VALUE handler, VALUE yaml, VALUE path)
         event_args[3] = end_line;
         event_args[4] = end_column;
         rb_protect(protected_event_location, (VALUE)event_args, &state);
+        if (state) {
+            yaml_event_delete(&event);
+            rb_jump_tag(state);
+        }
 
         switch(event.type) {
             case YAML_STREAM_START_EVENT:
@@ -496,7 +520,11 @@ static VALUE parse(VALUE self, VALUE handler, VALUE yaml, VALUE path)
             rb_protect(protected_end_mapping, handler, &state);
             break;
           case YAML_NO_EVENT:
+            /* Once libyaml has produced the stream end, every later call
+             * succeeds with a zeroed event and YAML_STREAM_END_EVENT can no
+             * longer be reached.  Stop rather than loop forever. */
             rb_protect(protected_empty, handler, &state);
+            done = 1;
             break;
           case YAML_STREAM_END_EVENT:
             rb_protect(protected_end_stream, handler, &state);
@@ -510,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>
@@ -521,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. */
@@ -348,6 +364,10 @@ static VALUE parse(VALUE self, VALUE handler, VALUE yaml, VALUE path)
         event_args[3] = SIZET2NUM(em ? (size_t)em->line : 0);
         event_args[4] = SIZET2NUM(em ? (size_t)em->column : 0);
         rb_protect(protected_event_location, (VALUE)event_args, &state);
+        if (state) {
+            fy_parser_event_free(parser->fyp, event);
+            rb_jump_tag(state);
+        }
 
         switch (event->type) {
             case FYET_STREAM_START:
@@ -473,7 +493,10 @@ static VALUE parse(VALUE self, VALUE handler, VALUE yaml, VALUE path)
                 rb_protect(protected_end_mapping, handler, &state);
             break;
             case FYET_NONE:
+                /* An event with no type cannot advance the stream, so stop
+                 * rather than loop forever. */
                 rb_protect(protected_empty, handler, &state);
+                done = 1;
             break;
             case FYET_STREAM_END:
                 rb_protect(protected_end_stream, handler, &state);
@@ -489,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 @@ def #{m} *args
       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
@@ -70,6 +103,56 @@ def test_exception_memory_leak
       end
     end
 
+    def test_event_location_exception_is_propagated
+      klass = Class.new(Psych::Handler) do
+        def event_location start_line, start_column, end_line, end_column
+          raise "from event_location"
+        end
+      end
+
+      parser = Psych::Parser.new klass.new
+      2.times do
+        ex = assert_raise(RuntimeError) { parser.parse "--- hello\n" }
+        assert_equal "from event_location", ex.message
+      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'