diff --git a/Gemfile b/Gemfile index 19673236..e8ccc2af 100644 --- a/Gemfile +++ b/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 diff --git a/ext/psych/psych_parser.c b/ext/psych/psych_parser.c index 27292737..2b50fcae 100644 --- a/ext/psych/psych_parser.c +++ b/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); @@ -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 # => # @@ -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); } diff --git a/ext/psych/psych_parser_fy.c b/ext/psych/psych_parser_fy.c index 96aa0fe6..ccb05776 100644 --- a/ext/psych/psych_parser_fy.c +++ b/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 # => # diff --git a/test/psych/test_parser.rb b/test/psych/test_parser.rb index 786cf016..aeb74244 100644 --- a/test/psych/test_parser.rb +++ b/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'