Fix an exception from the SAX input IO coming back as a TypeError

CHANGELOG.md

@@ -36,6 +36,10 @@ All changes to the Ox gem are documented here. Releases follow semantic versioni
 
 ### Fixed
 
+- Fix Ox.sax_parse replacing an exception raised by the input IO's read
+  or readpartial method with a TypeError. The original error now reaches
+  the caller with its class and message, and a TypeError raised inside
+  read is no longer swallowed as end of input.
 - Fix an element or attribute name going out unchecked. A name can now
   end itself and start something else with nothing raised. See PR #470.
 - Fix a comment, CDATA, DOCTYPE or instruction value going out

ext/ox/ox.c

@@ -58,6 +58,7 @@ ID ox_end_element_id;
 ID ox_end_id;
 ID ox_end_instruct_id;
 ID ox_error_id;
+ID ox_exception_id;
 ID ox_excl_id;
 ID ox_external_encoding_id;
 ID ox_fileno_id;
@@ -1565,6 +1566,7 @@ void Init_ox(void) {
     ox_end_id               = rb_intern("@end");
     ox_end_instruct_id      = rb_intern("end_instruct");
     ox_error_id             = rb_intern("error");
+    ox_exception_id         = rb_intern("exception");
     ox_excl_id              = rb_intern("@excl");
     ox_external_encoding_id = rb_intern("external_encoding");
     ox_fileno_id            = rb_intern("fileno");

ext/ox/ox.h

@@ -190,6 +190,7 @@ extern ID ox_end_element_id;
 extern ID ox_end_id;
 extern ID ox_end_instruct_id;
 extern ID ox_error_id;
+extern ID ox_exception_id;
 extern ID ox_excl_id;
 extern ID ox_external_encoding_id;
 extern ID ox_fileno_id;

ext/ox/sax_buf.c

@@ -119,16 +119,35 @@ int ox_sax_buf_read(Buf buf) {
     return err;
 }
 
+// rb_rescue hands the handler the exception instance, not its class. Passing
+// that instance to rb_raise(), which expects a class, made Ruby raise a
+// TypeError about the argument type while building the new exception, so the
+// caller saw that TypeError instead of the error its IO raised. Re-raise the
+// original error with the position appended, keeping its class.
+//
+// EOFError still ends the parse: that is how readpartial() signals end of
+// input. TypeError is no longer swallowed here; read() returning nil at EOF is
+// recognized in io_cb() rather than through the TypeError StringValuePtr()
+// raised on it.
+//
+// Raising here is safe: ox_sax_drive_cleanup() runs after the rb_protect() in
+// sax_parse() catches this.
 static VALUE rescue_cb(VALUE rbuf, VALUE err) {
-    VALUE err_class = rb_obj_class(err);
+    Buf   buf = (Buf)rbuf;
+    VALUE msg;
 
-    if (err_class != rb_eTypeError && err_class != rb_eEOFError) {
-        Buf buf = (Buf)rbuf;
-
-        // ox_sax_drive_cleanup(buf->dr); called after exiting protect
-        rb_raise(err, "at line %ld, column %ld\n", (long)buf->line, (long)buf->col);
+    if (rb_obj_is_kind_of(err, rb_eEOFError)) {
+        return Qfalse;
     }
-    return Qfalse;
+    msg = rb_sprintf("%" PRIsVALUE " at line %ld, column %ld",
+                     rb_funcall(err, ox_message_id, 0),
+                     (long)buf->line,
+                     (long)buf->col);
+    // Exception#exception clones the receiver and swaps in the new message, so
+    // the class survives whatever initialize() it happens to define.
+    rb_exc_raise(rb_funcall(err, ox_exception_id, 1, msg));
+
+    return Qfalse;  // not reached
 }
 
 static VALUE partial_io_cb(VALUE rbuf) {
@@ -141,7 +160,10 @@ static VALUE partial_io_cb(VALUE rbuf) {
 
     args[0] = ULONG2NUM(max);
     rstr    = rb_funcall2(buf->in.io, ox_readpartial_id, 1, args);
-    str     = StringValuePtr(rstr);
+    if (NIL_P(rstr)) {  // end of input
+        return Qfalse;
+    }
+    str = StringValuePtr(rstr);
     // Clamp to the space actually requested. A misbehaving IO can return more
     // than max bytes; copying it unclamped overflows the buffer (which starts
     // on the C stack). Use the real byte length, not strlen(), so an embedded
@@ -167,7 +189,13 @@ static VALUE io_cb(VALUE rbuf) {
 
     args[0] = ULONG2NUM(max);
     rstr    = rb_funcall2(buf->in.io, ox_read_id, 1, args);
-    str     = StringValuePtr(rstr);
+    // IO#read(len) returns nil at EOF. That used to reach StringValuePtr(),
+    // whose TypeError rescue_cb() read as end of input; say so directly so
+    // rescue_cb() no longer has to treat every TypeError as EOF.
+    if (NIL_P(rstr)) {
+        return Qfalse;
+    }
+    str = StringValuePtr(rstr);
     // See partial_io_cb: clamp to the requested size to prevent overflow.
     cnt = (size_t)RSTRING_LEN(rstr);
     if (cnt > max) {

test/sax_io_error_test.rb

@@ -0,0 +1,192 @@
+#!/usr/bin/env ruby
+# frozen_string_literal: true
+
+# Regression test: exceptions raised by the input IO's read method were mangled.
+#
+# rescue_cb() in sax_buf.c is the rb_rescue handler for the read()/readpartial()
+# calls. rb_rescue passes it the exception instance, but it handed that instance
+# to rb_raise(), which expects a class, so Ruby raised a TypeError about the
+# argument type and the original error was lost. On top of that the handler
+# treated TypeError and EOFError as end of input, so a TypeError raised inside
+# read() for any other reason ended the parse silently.
+#
+# A stream wrapper enforcing a size limit, or an IO failing mid-read, now
+# reports through Ox instead of having to stash the error and re-raise after
+# Ox.sax_parse returns.
+
+$: << File.join(File.dirname(__FILE__), '../lib')
+$: << File.join(File.dirname(__FILE__), '../ext')
+
+require 'test/unit'
+require 'stringio'
+require 'ox'
+
+class SaxIoErrorTest < ::Test::Unit::TestCase
+  class NullSax < ::Ox::Sax
+    def start_element(_n); end
+    def end_element(_n); end
+    def text(_t); end
+    def attr(_n, _v); end
+    def error(_m, _l, _c); end
+  end
+
+  class SizeLimit < StandardError; end
+
+  # An initialize that does not accept a message: rebuilding the exception with
+  # Class.new(msg) would fail on it, cloning the instance does not.
+  class NoArgError < StandardError
+    def initialize
+      super('no arg error')
+    end
+  end
+
+  # Fails partway through the document rather than on the first read, so the
+  # parser has consumed some input and line/column are meaningful.
+  #
+  # read() and readpartial() live in separate classes on purpose: sax_buf_init
+  # picks the readpartial path for anything that responds to readpartial, so an
+  # object carrying both never exercises read().
+  class FailingIO
+    def initialize(body, error)
+      @io = StringIO.new(body)
+      @error = error
+      @reads = 0
+    end
+
+    def read(max)
+      @reads += 1
+      raise @error if @reads > 1
+
+      @io.read(max)
+    end
+  end
+
+  class PartialFailingIO < FailingIO
+    def readpartial(max)
+      read(max)
+    end
+  end
+
+  # Bigger than the parser's 4096 byte buffer so a second read is needed.
+  BODY = "<top>#{'x' * 10_000}</top>".freeze
+
+  def parse(io)
+    Ox.sax_parse(NullSax.new, io)
+  end
+
+  def test_custom_error_from_read_reaches_the_caller
+    e = assert_raise(SizeLimit) do
+      parse(FailingIO.new(BODY, SizeLimit.new('response too large')))
+    end
+    assert_match(/response too large/, e.message)
+  end
+
+  def test_the_message_carries_the_position
+    e = assert_raise(SizeLimit) do
+      parse(FailingIO.new(BODY, SizeLimit.new('response too large')))
+    end
+    assert_match(/at line \d+, column \d+/, e.message)
+  end
+
+  def test_custom_error_from_readpartial_reaches_the_caller
+    e = assert_raise(SizeLimit) do
+      parse(PartialFailingIO.new(BODY, SizeLimit.new('response too large')))
+    end
+    assert_match(/response too large/, e.message)
+  end
+
+  def test_error_class_with_a_zero_argument_initialize_survives
+    e = assert_raise(NoArgError) { parse(FailingIO.new(BODY, NoArgError.new)) }
+    assert_match(/no arg error/, e.message)
+  end
+
+  # TypeError used to be swallowed as end of input, so the parse just stopped.
+  def test_type_error_from_read_is_no_longer_swallowed
+    assert_raise(TypeError) do
+      parse(FailingIO.new(BODY, TypeError.new('not a string')))
+    end
+  end
+
+  def test_io_error_from_read_reaches_the_caller
+    assert_raise(IOError) { parse(FailingIO.new(BODY, IOError.new('closed stream'))) }
+  end
+
+  # EOFError stays end of input: that is how readpartial signals it.
+  def test_eof_error_still_ends_the_parse
+    assert_nothing_raised do
+      parse(PartialFailingIO.new(BODY, EOFError.new))
+    end
+  end
+
+  def test_eof_error_from_read_still_ends_the_parse
+    assert_nothing_raised { parse(FailingIO.new(BODY, EOFError.new)) }
+  end
+
+  # read returning nil is the other end of input signal, and it used to work
+  # only because StringValuePtr raised a TypeError on nil.
+  class NilAtEofIO
+    def initialize(body)
+      @body = body
+      @sent = false
+    end
+
+    def read(_max)
+      return nil if @sent
+
+      @sent = true
+      @body
+    end
+  end
+
+  def test_nil_from_read_still_ends_the_parse
+    handler = Collect.new
+    Ox.sax_parse(handler, NilAtEofIO.new('<top>x</top>'))
+    assert_equal(['top'], handler.seen)
+  end
+
+  # An IO that never fails still parses through both IO paths.
+  class WorkingIO
+    def initialize(body)
+      @io = StringIO.new(body)
+    end
+
+    def read(max)
+      @io.read(max)
+    end
+  end
+
+  class WorkingPartialIO < WorkingIO
+    def readpartial(max)
+      raise EOFError if @io.eof?
+
+      @io.readpartial(max)
+    end
+  end
+
+  class Collect < ::Ox::Sax
+    attr_reader :seen
+
+    def initialize
+      super
+      @seen = []
+    end
+
+    def start_element(name)
+      @seen << name.to_s
+    end
+
+    def error(_m, _l, _c); end
+  end
+
+  def test_a_complete_document_still_parses_through_read
+    handler = Collect.new
+    Ox.sax_parse(handler, WorkingIO.new(BODY.dup))
+    assert_equal(['top'], handler.seen)
+  end
+
+  def test_a_complete_document_still_parses_through_readpartial
+    handler = Collect.new
+    Ox.sax_parse(handler, WorkingPartialIO.new(BODY.dup))
+    assert_equal(['top'], handler.seen)
+  end
+end