From 6553637de69a7a12f9f95b813b89e2b09f865fa4 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Thu, 6 Aug 2026 18:26:17 -0700 Subject: [PATCH 1/3] Update the pointer registry after GC compaction The bindings map an xmlDocPtr or an xmlNodePtr to its Ruby wrapper in a global st_table. The table holds each wrapper as a raw machine address. GC compaction moves the wrapper but does not change the address in the table. The table then holds a dead address. Mark functions read the table. rxml_node_mark and rxml_dtd_mark and rxml_reader_mark send the address to rb_gc_mark. The garbage collector then aborts the process: [BUG] try to mark T_NONE object (obj: 0x... T_NONE/, parent: 0x... LibXML::XML::Node) [BUG] Segmentation fault at 0x000000000000001c The data pointer that dcompact receives is the key of the table entry. So each wrapper type can repair its own entry. Add rxml_registry_update for this. It looks up the entry, calls rb_gc_location on the stored address, and stores the result. Install a dcompact function on the Document type and on the managed Node type. These are the two types that add entries to the table. Add regression tests for both types. The tests keep the wrapper in an Array, not in a local variable. The machine stack scan pins a local variable, so a wrapper in a local variable never moves. --- ext/libxml/ruby_xml_document.c | 12 +++++++++++- ext/libxml/ruby_xml_node.c | 13 ++++++++++++- ext/libxml/ruby_xml_registry.c | 7 +++++++ ext/libxml/ruby_xml_registry.h | 9 ++++++++- test/test_document.rb | 16 ++++++++++++++++ test/test_helper.rb | 18 ++++++++++++++++++ test/test_node.rb | 19 +++++++++++++++++++ 7 files changed, 91 insertions(+), 3 deletions(-) diff --git a/ext/libxml/ruby_xml_document.c b/ext/libxml/ruby_xml_document.c index d51511b2..5a9083d6 100644 --- a/ext/libxml/ruby_xml_document.c +++ b/ext/libxml/ruby_xml_document.c @@ -68,9 +68,19 @@ void rxml_document_free(void* data) xmlFreeDoc(xdoc); } +/* GC compaction moves the Ruby wrapper but does not update the address that + the registry holds for this document. The document pointer is the registry + key, so ask the registry to rewrite this entry with the new address. */ +static void rxml_document_compact(void* data) +{ + xmlDocPtr xdoc = (xmlDocPtr)data; + if (xdoc) + rxml_registry_update(xdoc); +} + const rb_data_type_t rxml_document_data_type = { .wrap_struct_name = "LibXML::XML::Document", - .function = { .dmark = NULL, .dfree = rxml_document_free }, + .function = { .dmark = NULL, .dfree = rxml_document_free, .dcompact = rxml_document_compact }, .flags = RUBY_TYPED_FREE_IMMEDIATELY, }; diff --git a/ext/libxml/ruby_xml_node.c b/ext/libxml/ruby_xml_node.c index e8710ae7..8f53f94d 100644 --- a/ext/libxml/ruby_xml_node.c +++ b/ext/libxml/ruby_xml_node.c @@ -96,9 +96,20 @@ const rb_data_type_t rxml_node_unmanaged_data_type = { .flags = RUBY_TYPED_FREE_IMMEDIATELY, }; +/* GC compaction moves the Ruby wrapper but does not update the address that + the registry holds for this node. The node pointer is the registry key, so + ask the registry to rewrite this entry with the new address. Only managed + nodes are in the registry, so only this type needs the function. */ +static void rxml_node_compact(void* data) +{ + xmlNodePtr xnode = (xmlNodePtr)data; + if (xnode) + rxml_registry_update(xnode); +} + static const rb_data_type_t rxml_node_managed_data_type = { .wrap_struct_name = "LibXML::XML::Node (managed)", - .function = { .dmark = (RUBY_DATA_FUNC)rxml_node_mark, .dfree = rxml_node_free }, + .function = { .dmark = (RUBY_DATA_FUNC)rxml_node_mark, .dfree = rxml_node_free, .dcompact = rxml_node_compact }, .parent = &rxml_node_data_type, .flags = RUBY_TYPED_FREE_IMMEDIATELY, }; diff --git a/ext/libxml/ruby_xml_registry.c b/ext/libxml/ruby_xml_registry.c index 7f70e45e..8f81c3ed 100644 --- a/ext/libxml/ruby_xml_registry.c +++ b/ext/libxml/ruby_xml_registry.c @@ -29,3 +29,10 @@ VALUE rxml_registry_lookup(void *ptr) return (VALUE)val; return Qnil; } + +void rxml_registry_update(void *ptr) +{ + st_data_t val; + if (st_lookup(rxml_registry, (st_data_t)ptr, &val)) + st_insert(rxml_registry, (st_data_t)ptr, (st_data_t)rb_gc_location((VALUE)val)); +} diff --git a/ext/libxml/ruby_xml_registry.h b/ext/libxml/ruby_xml_registry.h index 9cfeb68e..58a4c06d 100644 --- a/ext/libxml/ruby_xml_registry.h +++ b/ext/libxml/ruby_xml_registry.h @@ -12,11 +12,18 @@ of reading _private). Registered pointers MUST be unregistered before the underlying C struct is - freed, typically in the wrapper's dfree function. */ + freed, typically in the wrapper's dfree function. + + The stored VALUEs are plain machine addresses, so GC compaction invalidates + them when it moves a wrapper. Each wrapper type that registers itself MUST + also install a dcompact function that calls rxml_registry_update with its + own data pointer. The data pointer is the registry key, so the entry for + the object that moved is the entry that dcompact repairs. */ void rxml_init_registry(void); void rxml_registry_register(void *ptr, VALUE obj); void rxml_registry_unregister(void *ptr); VALUE rxml_registry_lookup(void *ptr); /* Qnil on miss */ +void rxml_registry_update(void *ptr); /* rewrite entry after compaction */ #endif diff --git a/test/test_document.rb b/test/test_document.rb index 231a7d0d..c0aa2fbd 100644 --- a/test/test_document.rb +++ b/test/test_document.rb @@ -135,4 +135,20 @@ def test_io end end + # The bindings map an xmlDocPtr to its Ruby wrapper in a registry. GC + # compaction moves the wrapper. The registry must learn the new address. + # The document stays in a heap container so that compaction can move it. + def test_gc_compaction_updates_document_registry + holder = [LibXML::XML::Document.string('')] + + compact_heap + + assert_equal('root', holder[0].root.doc.root.name) + + # A node wrapper marks its document through the registry. A stale entry + # sends a dead address to the garbage collector. + holder[0].root + GC.start(full_mark: true, immediate_sweep: true) + end + end diff --git a/test/test_helper.rb b/test/test_helper.rb index 1a56fa7a..0e834874 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -14,6 +14,24 @@ def windows? !(RbConfig::CONFIG['host_os'] =~ /mswin|mingw/).nil? end +# Moves every movable object on the heap. +# +# Tests that use this method must keep the object under test in a heap +# container, for example an Array. An object in a live local variable is +# pinned by the conservative machine stack scan, so it never moves and the +# test cannot detect a stale address. +def compact_heap + if GC.respond_to?(:verify_compaction_references) + begin + GC.verify_compaction_references(expand_heap: true, toward: :empty) + rescue NotImplementedError => e + skip("GC compaction is not available: #{e.message}") + end + else + skip('GC compaction is not available') + end +end + STDOUT.write "\nlibxml2: #{LibXML::XML::LIBXML_VERSION}\n#{RUBY_DESCRIPTION}\n\n" require 'minitest/autorun' diff --git a/test/test_node.rb b/test/test_node.rb index 1e23f329..8dd296d8 100644 --- a/test/test_node.rb +++ b/test/test_node.rb @@ -242,6 +242,25 @@ def test_document_node_marks_document GC.stress = false end + # A standalone node owns its libxml tree, so the bindings map the xmlNodePtr + # to its Ruby wrapper in a registry. GC compaction moves the wrapper. The + # registry must learn the new address. The wrapper stays in a heap container + # so that compaction can move it. + def test_gc_compaction_updates_node_registry + holder = [LibXML::XML::Node.new('root')] + holder[0] << LibXML::XML::Node.new('child') + + compact_heap + + # A child node wrapper marks the root wrapper through the registry. A + # stale entry sends a dead address to the garbage collector. + child = holder[0].child + GC.start(full_mark: true, immediate_sweep: true) + + assert_equal('child', child.name) + assert_equal('root', holder[0].name) + end + private def create_document_child From fc5225e9aa0b0866947450b3c815c1d64948dd39 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Thu, 6 Aug 2026 18:31:03 -0700 Subject: [PATCH 2/3] Give libxml2 a stable context for reads from a Ruby IO object XML::Parser::Context.io and XML::HTMLParser::Context.io and XML::Reader.io give libxml2 a raw VALUE as the read context. libxml2 keeps that value and calls rxml_read_callback much later. GC compaction moves the IO object in the meantime, so the callback reads a dead address: [BUG] Segmentation fault at 0x0000000000000010 The public methods XML::Parser.io, XML::Document.io, XML::SaxParser.io, XML::HTMLParser.io and XML::Reader.io all use one of these three methods. XML::Writer already shows the correct pattern. It gives libxml2 a struct that holds the VALUE, and it marks that VALUE from its dmark function. rb_gc_mark pins the object, so the address in the struct stays correct. Add rxml_io_context for this pattern: - The two parser contexts store the struct in ctxt->_private. libxml2 never touches that field. The mark function marks the io object and the free function releases the struct. - An xmlTextReader is opaque, so XML::Reader now wraps a new rxml_reader_object struct that holds the reader and the io context. Remove the @io instance variable from all three classes. The mark function now keeps the io object alive. In the two parser contexts the variable never worked, because the code assigned the result of ID2SYM to an ID and then passed it to rb_ivar_set. Add regression tests for XML::Parser.io, XML::HTMLParser.io and XML::Reader.io. Each test keeps the parser or the reader in an Array, not in a local variable. The machine stack scan pins a local variable, so an object in a local variable never moves. --- ext/libxml/ruby_xml_html_parser_context.c | 38 +++++++--- ext/libxml/ruby_xml_io.c | 26 ++++++- ext/libxml/ruby_xml_io.h | 21 ++++++ ext/libxml/ruby_xml_parser_context.c | 40 ++++++++--- ext/libxml/ruby_xml_reader.c | 84 ++++++++++++++++------- test/test_html_parser.rb | 12 ++++ test/test_parser.rb | 12 ++++ test/test_reader.rb | 12 ++++ 8 files changed, 200 insertions(+), 45 deletions(-) diff --git a/ext/libxml/ruby_xml_html_parser_context.c b/ext/libxml/ruby_xml_html_parser_context.c index 28bbed1a..23c9ff02 100644 --- a/ext/libxml/ruby_xml_html_parser_context.c +++ b/ext/libxml/ruby_xml_html_parser_context.c @@ -13,7 +13,6 @@ */ VALUE cXMLHtmlParserContext; -static ID IO_ATTR; /* OS X 10.5 ships with libxml2 version 2.6.16 which does not expose the htmlNewParserCtxt (or htmlInitParserCtxt which it uses) method. htmlNewParserCtxt @@ -129,15 +128,27 @@ static htmlParserCtxtPtr htmlNewParserCtxt(void) } #endif +/* XML::HTMLParser::Context.io stores the read context in the _private field + of the libxml parser context. libxml2 never touches that field. */ static void rxml_html_parser_context_free(void* data) { htmlParserCtxtPtr ctxt = (htmlParserCtxtPtr)data; + if (!ctxt) return; + rxml_io_context_free((rxml_io_context*)ctxt->_private); + ctxt->_private = NULL; htmlFreeParserCtxt(ctxt); } +static void rxml_html_parser_context_mark(void* data) +{ + htmlParserCtxtPtr ctxt = (htmlParserCtxtPtr)data; + if (!ctxt) return; + rxml_io_context_mark((rxml_io_context*)ctxt->_private); +} + const rb_data_type_t rxml_html_parser_context_type = { "LibXML::XML::HTMLParser::Context", - {NULL, rxml_html_parser_context_free, NULL}, + {rxml_html_parser_context_mark, rxml_html_parser_context_free, NULL}, &rxml_parser_context_type, NULL, 0 }; @@ -197,13 +208,22 @@ static VALUE rxml_html_parser_context_io(int argc, VALUE* argv, VALUE klass) if (NIL_P(io)) rb_raise(rb_eTypeError, "Must pass in an IO object"); - input = xmlParserInputBufferCreateIO((xmlInputReadCallback) rxml_read_callback, NULL, - (void*)io, XML_CHAR_ENCODING_NONE); - ctxt = htmlNewParserCtxt(); if (!ctxt) + rxml_raise(xmlGetLastError()); + + /* libxml2 keeps the context pointer and calls the read callback much later. + So give libxml2 a stable address instead of the io object itself. The + mark function marks the io object, which keeps it alive and stops GC + compaction from moving it. */ + ctxt->_private = rxml_io_context_new(io); + + input = xmlParserInputBufferCreateIO((xmlInputReadCallback) rxml_read_callback, NULL, + ctxt->_private, XML_CHAR_ENCODING_NONE); + + if (!input) { - xmlFreeParserInputBuffer(input); + rxml_html_parser_context_free(ctxt); rxml_raise(xmlGetLastError()); } @@ -218,14 +238,13 @@ static VALUE rxml_html_parser_context_io(int argc, VALUE* argv, VALUE klass) if (!stream) { xmlFreeParserInputBuffer(input); - xmlFreeParserCtxt(ctxt); + rxml_html_parser_context_free(ctxt); rxml_raise(xmlGetLastError()); } inputPush(ctxt, stream); result = rxml_html_parser_context_wrap(ctxt); - /* Attach io object to parser so it won't get freed.*/ - rb_ivar_set(result, IO_ATTR, io); + RB_GC_GUARD(io); return result; } @@ -351,7 +370,6 @@ static VALUE rxml_html_parser_context_alloc(VALUE klass) void rxml_init_html_parser_context(void) { - IO_ATTR = ID2SYM(rb_intern("@io")); cXMLHtmlParserContext = rb_define_class_under(cXMLHtmlParser, "Context", cXMLParserContext); rb_define_alloc_func(cXMLHtmlParserContext, rxml_html_parser_context_alloc); diff --git a/ext/libxml/ruby_xml_io.c b/ext/libxml/ruby_xml_io.c index 727453e7..272294e8 100644 --- a/ext/libxml/ruby_xml_io.c +++ b/ext/libxml/ruby_xml_io.c @@ -6,12 +6,36 @@ static ID READ_METHOD; static ID WRITE_METHOD; +rxml_io_context* rxml_io_context_new(VALUE io) +{ + rxml_io_context* context = ALLOC(rxml_io_context); + context->io = io; + return context; +} + +void rxml_io_context_init(rxml_io_context* context, VALUE io) +{ + context->io = io; +} + +void rxml_io_context_free(rxml_io_context* context) +{ + if (context) + xfree(context); +} + +void rxml_io_context_mark(rxml_io_context* context) +{ + if (context && !NIL_P(context->io)) + rb_gc_mark(context->io); +} + /* This method is called by libxml when it wants to read more data from a stream. We go with the duck typing solution to support StringIO objects. */ int rxml_read_callback(void *context, char *buffer, int len) { - VALUE io = (VALUE) context; + VALUE io = ((rxml_io_context*) context)->io; VALUE string = rb_funcall(io, READ_METHOD, 1, INT2NUM(len)); size_t size; diff --git a/ext/libxml/ruby_xml_io.h b/ext/libxml/ruby_xml_io.h index cafd95ce..d64ab62f 100644 --- a/ext/libxml/ruby_xml_io.h +++ b/ext/libxml/ruby_xml_io.h @@ -3,6 +3,27 @@ #ifndef __RXML_IO__ #define __RXML_IO__ +/* Context that libxml2 receives for reads from a Ruby IO object. + + libxml2 keeps the context pointer and calls rxml_read_callback much later. + A raw VALUE is not a valid context, because GC compaction moves the IO + object and the VALUE then holds a dead address. So give libxml2 the + address of this struct instead. + + The owner of the struct MUST call rxml_io_context_mark from its dmark + function. rb_gc_mark pins the IO object, so the VALUE in the struct stays + correct. The owner MUST also call rxml_io_context_free from its dfree + function. */ +typedef struct +{ + VALUE io; +} rxml_io_context; + +rxml_io_context* rxml_io_context_new(VALUE io); +void rxml_io_context_init(rxml_io_context* context, VALUE io); +void rxml_io_context_free(rxml_io_context* context); +void rxml_io_context_mark(rxml_io_context* context); + int rxml_read_callback(void *context, char *buffer, int len); int rxml_write_callback(VALUE io, const char *buffer, int len); void rxml_init_io(void); diff --git a/ext/libxml/ruby_xml_parser_context.c b/ext/libxml/ruby_xml_parser_context.c index 610a88d8..97ccefff 100644 --- a/ext/libxml/ruby_xml_parser_context.c +++ b/ext/libxml/ruby_xml_parser_context.c @@ -6,7 +6,6 @@ #include VALUE cXMLParserContext; -static ID IO_ATTR; /* * Document-class: LibXML::XML::Parser::Context @@ -15,15 +14,27 @@ static ID IO_ATTR; * a document is parsed. */ +/* XML::Parser::Context.io stores the read context in the _private field of + the libxml parser context. libxml2 never touches that field. */ static void rxml_parser_context_free(void* data) { xmlParserCtxtPtr ctxt = (xmlParserCtxtPtr)data; + if (!ctxt) return; + rxml_io_context_free((rxml_io_context*)ctxt->_private); + ctxt->_private = NULL; xmlFreeParserCtxt(ctxt); } +static void rxml_parser_context_mark(void* data) +{ + xmlParserCtxtPtr ctxt = (xmlParserCtxtPtr)data; + if (!ctxt) return; + rxml_io_context_mark((rxml_io_context*)ctxt->_private); +} + const rb_data_type_t rxml_parser_context_type = { "LibXML::XML::Parser::Context", - {NULL, rxml_parser_context_free, NULL}, + {rxml_parser_context_mark, rxml_parser_context_free, NULL}, NULL, NULL, 0 }; @@ -158,14 +169,23 @@ static VALUE rxml_parser_context_io(int argc, VALUE* argv, VALUE klass) if (NIL_P(io)) rb_raise(rb_eTypeError, "Must pass in an IO object"); - xmlParserInputBufferPtr input = xmlParserInputBufferCreateIO((xmlInputReadCallback) rxml_read_callback, NULL, - (void*)io, XML_CHAR_ENCODING_NONE); - xmlParserCtxtPtr ctxt = xmlNewParserCtxt(); if (!ctxt) + rxml_raise(xmlGetLastError()); + + /* libxml2 keeps the context pointer and calls the read callback much later. + So give libxml2 a stable address instead of the io object itself. The + mark function marks the io object, which keeps it alive and stops GC + compaction from moving it. */ + ctxt->_private = rxml_io_context_new(io); + + xmlParserInputBufferPtr input = xmlParserInputBufferCreateIO((xmlInputReadCallback) rxml_read_callback, NULL, + ctxt->_private, XML_CHAR_ENCODING_NONE); + + if (!input) { - xmlFreeParserInputBuffer(input); + rxml_parser_context_free(ctxt); rxml_raise(xmlGetLastError()); } @@ -180,14 +200,14 @@ static VALUE rxml_parser_context_io(int argc, VALUE* argv, VALUE klass) if (!stream) { xmlFreeParserInputBuffer(input); - xmlFreeParserCtxt(ctxt); + rxml_parser_context_free(ctxt); rxml_raise(xmlGetLastError()); } inputPush(ctxt, stream); + VALUE result = rxml_parser_context_wrap(ctxt); - /* Attach io object to parser so it won't get freed.*/ - rb_ivar_set(result, IO_ATTR, io); + RB_GC_GUARD(io); return result; } @@ -960,8 +980,6 @@ static VALUE rxml_parser_context_well_formed_q(VALUE self) void rxml_init_parser_context(void) { - IO_ATTR = ID2SYM(rb_intern("@io")); - cXMLParserContext = rb_define_class_under(cXMLParser, "Context", rb_cObject); rb_define_alloc_func(cXMLParserContext, rxml_parser_context_alloc); diff --git a/ext/libxml/ruby_xml_reader.c b/ext/libxml/ruby_xml_reader.c index 7475b0e0..4ac12898 100644 --- a/ext/libxml/ruby_xml_reader.c +++ b/ext/libxml/ruby_xml_reader.c @@ -57,24 +57,52 @@ VALUE cXMLReader; static ID BASE_URI_SYMBOL; static ID ENCODING_SYMBOL; -static ID IO_ATTR; static ID OPTIONS_SYMBOL; +/* An xmlTextReader is opaque, so there is no field in it for Ruby data. The + Ruby object therefore wraps this struct instead of the reader itself. + + XML::Reader.io gives libxml2 the address of io_context. libxml2 keeps that + address and calls the read callback much later. The mark function marks + io_context.io, which keeps the io object alive and stops GC compaction + from moving it. */ +typedef struct +{ + xmlTextReaderPtr xreader; + rxml_io_context io_context; +} rxml_reader_object; + +static rxml_reader_object* rxml_reader_object_alloc(void) +{ + rxml_reader_object* rro = ALLOC(rxml_reader_object); + rro->xreader = NULL; + rxml_io_context_init(&rro->io_context, Qnil); + return rro; +} + static void rxml_reader_free(void* data) { - xmlTextReaderPtr xreader = (xmlTextReaderPtr)data; - xmlFreeTextReader(xreader); + rxml_reader_object* rro = (rxml_reader_object*)data; + if (rro->xreader) + xmlFreeTextReader(rro->xreader); + xfree(rro); } static void rxml_reader_mark(void* data) { - xmlTextReaderPtr xreader = (xmlTextReaderPtr)data; - xmlDocPtr xdoc = xmlTextReaderCurrentDoc(xreader); - if (xdoc) + rxml_reader_object* rro = (rxml_reader_object*)data; + + rxml_io_context_mark(&rro->io_context); + + if (rro->xreader) { - VALUE doc = rxml_registry_lookup(xdoc); - if (!NIL_P(doc)) - rb_gc_mark(doc); + xmlDocPtr xdoc = xmlTextReaderCurrentDoc(rro->xreader); + if (xdoc) + { + VALUE doc = rxml_registry_lookup(xdoc); + if (!NIL_P(doc)) + rb_gc_mark(doc); + } } } @@ -84,17 +112,24 @@ static const rb_data_type_t rxml_reader_data_type = { .flags = RUBY_TYPED_FREE_IMMEDIATELY, }; -static VALUE rxml_reader_wrap(xmlTextReaderPtr xreader) +static VALUE rxml_reader_wrap(rxml_reader_object* rro) { - return TypedData_Wrap_Struct(cXMLReader, &rxml_reader_data_type, xreader); + return TypedData_Wrap_Struct(cXMLReader, &rxml_reader_data_type, rro); } +/* Wraps a reader that does not read from a Ruby object. */ +static VALUE rxml_reader_wrap_xreader(xmlTextReaderPtr xreader) +{ + rxml_reader_object* rro = rxml_reader_object_alloc(); + rro->xreader = xreader; + return rxml_reader_wrap(rro); +} static xmlTextReaderPtr rxml_text_reader_get(VALUE obj) { - xmlTextReaderPtr xreader; - TypedData_Get_Struct(obj, xmlTextReader, &rxml_reader_data_type, xreader); - return xreader; + rxml_reader_object* rro; + TypedData_Get_Struct(obj, rxml_reader_object, &rxml_reader_data_type, rro); + return rro->xreader; } /* @@ -115,7 +150,7 @@ VALUE rxml_reader_document(VALUE klass, VALUE doc) if (xreader == NULL) rxml_raise(xmlGetLastError()); - return rxml_reader_wrap(xreader); + return rxml_reader_wrap_xreader(xreader); } /* call-seq: @@ -163,7 +198,7 @@ static VALUE rxml_reader_file(int argc, VALUE *argv, VALUE klass) if (xreader == NULL) rb_syserr_fail(ENOENT, StringValueCStr(path)); - return rxml_reader_wrap(xreader); + return rxml_reader_wrap_xreader(xreader); } /* call-seq: @@ -214,17 +249,21 @@ static VALUE rxml_reader_io(int argc, VALUE *argv, VALUE klass) xoptions = NIL_P(parserOptions) ? 0 : NUM2INT(parserOptions); } + rxml_reader_object* rro = rxml_reader_object_alloc(); + rxml_io_context_init(&rro->io_context, io); + xreader = xmlReaderForIO((xmlInputReadCallback) rxml_read_callback, NULL, - (void *) io, + &rro->io_context, xbaseurl, xencoding, xoptions); if (xreader == NULL) + { + xfree(rro); rxml_raise(xmlGetLastError()); + } - result = rxml_reader_wrap(xreader); - - /* Attach io object to parser so it won't get freed.*/ - rb_ivar_set(result, IO_ATTR, io); + rro->xreader = xreader; + result = rxml_reader_wrap(rro); return result; } @@ -283,7 +322,7 @@ static VALUE rxml_reader_string(int argc, VALUE *argv, VALUE klass) if (xreader == NULL) rxml_raise(xmlGetLastError()); - return rxml_reader_wrap(xreader); + return rxml_reader_wrap_xreader(xreader); } /* @@ -1143,7 +1182,6 @@ void rxml_init_reader(void) { BASE_URI_SYMBOL = ID2SYM(rb_intern("base_uri")); ENCODING_SYMBOL = ID2SYM(rb_intern("encoding")); - IO_ATTR = rb_intern("@io"); OPTIONS_SYMBOL = ID2SYM(rb_intern("options")); cXMLReader = rb_define_class_under(mXML, "Reader", rb_cObject); diff --git a/test/test_html_parser.rb b/test/test_html_parser.rb index e2662c5d..4e901675 100644 --- a/test/test_html_parser.rb +++ b/test/test_html_parser.rb @@ -52,6 +52,18 @@ def test_io_gc assert(parser.parse) end + # libxml2 keeps the read context and calls the read callback at parse time. + # GC compaction moves the io object, so the context must not be a raw + # address. The parser stays in a heap container, not in a local variable, + # so that compaction can move it. + def test_io_gc_compaction + holder = [LibXML::XML::HTMLParser.io(StringIO.new(File.read(html_file)))] + + compact_heap + + assert_instance_of(LibXML::XML::Document, holder[0].parse) + end + def test_nil_io error = assert_raises(TypeError) do LibXML::XML::HTMLParser.io(nil) diff --git a/test/test_parser.rb b/test/test_parser.rb index f94e54fd..9c584e04 100644 --- a/test/test_parser.rb +++ b/test/test_parser.rb @@ -103,6 +103,18 @@ def test_io_gc assert(parser.parse) end + # libxml2 keeps the read context and calls the read callback at parse time. + # GC compaction moves the io object, so the context must not be a raw + # address. The parser stays in a heap container, not in a local variable, + # so that compaction can move it. + def test_io_gc_compaction + holder = [LibXML::XML::Parser.io(StringIO.new(''))] + + compact_heap + + assert_equal('root', holder[0].parse.root.name) + end + def test_nil_io error = assert_raises(TypeError) do LibXML::XML::Parser.io(nil) diff --git a/test/test_reader.rb b/test/test_reader.rb index 4cd93788..0f86e2f0 100644 --- a/test/test_reader.rb +++ b/test/test_reader.rb @@ -88,6 +88,18 @@ def test_io_gc assert(reader.read) end + # libxml2 keeps the read context and calls the read callback on each read. + # GC compaction moves the io object, so the context must not be a raw + # address. The reader stays in a heap container, not in a local variable, + # so that compaction can move it. + def test_io_gc_compaction + holder = [LibXML::XML::Reader.io(StringIO.new(File.read(XML_FILE)))] + + compact_heap + + verify_simple(holder[0]) + end + def test_string_io data = File.read(XML_FILE) string_io = StringIO.new(data) From 64c0aa80f04b8e9b77feaf6908dae78a74ce51d5 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Thu, 6 Aug 2026 18:34:56 -0700 Subject: [PATCH 3/3] Keep the string that XML::Reader.string reads from XML::Reader.string calls xmlReaderForMemory with the address of the buffer of the Ruby String. xmlReaderForMemory does not copy the buffer, and libxml2 reads from the buffer on each call to XML::Reader#read. The reader does not keep the String, so the garbage collector frees the String and the reader then reads free memory: LibXML::XML::Error: Fatal error: Couldn't find end of Start Tag roo at :1. The corrupted name is other data in the reused memory. The reader now keeps a frozen copy of the String and marks it with rb_gc_mark. A frozen copy shares the buffer of the original String, so this does not copy the data of a large document. The copy also protects the reader if the program changes the original String. rb_gc_mark pins the copy, so GC compaction cannot move a short String that holds its bytes inside the object. Both constructors now wrap the struct in the Ruby object before they store a VALUE in it. TypedData_Wrap_Struct allocates, so it can start a garbage collection. A VALUE that only malloc memory holds is not visible to the garbage collector at that moment. Add a regression test. The test builds the string at run time and makes the garbage collector reuse the memory. A short literal string stays in place, so a test with a literal passes for the wrong reason. --- ext/libxml/ruby_xml_reader.c | 40 +++++++++++++++++++++++++++++------- test/test_reader.rb | 26 +++++++++++++++++++++++ 2 files changed, 59 insertions(+), 7 deletions(-) diff --git a/ext/libxml/ruby_xml_reader.c b/ext/libxml/ruby_xml_reader.c index 4ac12898..d0e80bba 100644 --- a/ext/libxml/ruby_xml_reader.c +++ b/ext/libxml/ruby_xml_reader.c @@ -65,17 +65,25 @@ static ID OPTIONS_SYMBOL; XML::Reader.io gives libxml2 the address of io_context. libxml2 keeps that address and calls the read callback much later. The mark function marks io_context.io, which keeps the io object alive and stops GC compaction - from moving it. */ + from moving it. + + XML::Reader.string gives libxml2 the address of the buffer of a String. + xmlReaderForMemory does not copy the buffer, and libxml2 reads from it on + each call to XML::Reader#read. So the reader keeps the String in the + string field. The mark function marks that String with rb_gc_mark, which + both keeps the String alive and stops GC compaction from moving it. */ typedef struct { xmlTextReaderPtr xreader; rxml_io_context io_context; + VALUE string; } rxml_reader_object; static rxml_reader_object* rxml_reader_object_alloc(void) { rxml_reader_object* rro = ALLOC(rxml_reader_object); rro->xreader = NULL; + rro->string = Qnil; rxml_io_context_init(&rro->io_context, Qnil); return rro; } @@ -94,6 +102,9 @@ static void rxml_reader_mark(void* data) rxml_io_context_mark(&rro->io_context); + if (!NIL_P(rro->string)) + rb_gc_mark(rro->string); + if (rro->xreader) { xmlDocPtr xdoc = xmlTextReaderCurrentDoc(rro->xreader); @@ -249,7 +260,10 @@ static VALUE rxml_reader_io(int argc, VALUE *argv, VALUE klass) xoptions = NIL_P(parserOptions) ? 0 : NUM2INT(parserOptions); } + /* Wrap the struct before it holds any VALUE. The garbage collector can + then see the io object through the mark function. */ rxml_reader_object* rro = rxml_reader_object_alloc(); + result = rxml_reader_wrap(rro); rxml_io_context_init(&rro->io_context, io); xreader = xmlReaderForIO((xmlInputReadCallback) rxml_read_callback, NULL, @@ -257,13 +271,9 @@ static VALUE rxml_reader_io(int argc, VALUE *argv, VALUE klass) xbaseurl, xencoding, xoptions); if (xreader == NULL) - { - xfree(rro); rxml_raise(xmlGetLastError()); - } rro->xreader = xreader; - result = rxml_reader_wrap(rro); return result; } @@ -316,13 +326,29 @@ static VALUE rxml_reader_string(int argc, VALUE *argv, VALUE klass) xoptions = NIL_P(parserOptions) ? 0 : NUM2INT(parserOptions); } - xreader = xmlReaderForMemory(StringValueCStr(string), (int)RSTRING_LEN(string), + /* Reject a string that contains a null character. */ + StringValueCStr(string); + + /* Wrap the struct before it holds any VALUE. The garbage collector can then + see the string through the mark function. */ + rxml_reader_object* rro = rxml_reader_object_alloc(); + VALUE result = rxml_reader_wrap(rro); + + /* Take a frozen copy of the string. The copy shares the buffer of the + original string, so this does not copy the data of a large document. The + copy also protects the reader if the program changes the original + string. */ + rro->string = rb_str_new_frozen(string); + + xreader = xmlReaderForMemory(RSTRING_PTR(rro->string), (int)RSTRING_LEN(rro->string), xbaseurl, xencoding, xoptions); if (xreader == NULL) rxml_raise(xmlGetLastError()); - return rxml_reader_wrap_xreader(xreader); + rro->xreader = xreader; + + return result; } /* diff --git a/test/test_reader.rb b/test/test_reader.rb index 0f86e2f0..e4d39ada 100644 --- a/test/test_reader.rb +++ b/test/test_reader.rb @@ -413,8 +413,34 @@ def test_expand_gc_after_advance GC.stress = false end + # xmlReaderForMemory does not copy the string, and libxml2 reads from the + # buffer of the string on each call to #read. So the reader must keep the + # string. A short literal string does not show the defect, because the + # literal stays in place. The test therefore builds a large string at run + # time and then makes the garbage collector reuse the memory. + def test_string_gc_compaction + holder = [LibXML::XML::Reader.string(build_large_xml)] + + compact_heap + 20_000.times { |i| +"churn #{i}" } + + elements = 0 + while holder[0].read + elements += 1 if holder[0].node_type == LibXML::XML::Reader::TYPE_ELEMENT + end + + assert_equal(201, elements) + end + private + def build_large_xml + xml = +'' + 200.times { xml << "#{'x' * 50}" } + xml << '' + xml + end + def expand_and_advance reader = LibXML::XML::Reader.string("") reader.read # root