Fix three uses of a stale address after GC compaction

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,
 };
 

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);
 

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;
 

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);

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,
 };

ext/libxml/ruby_xml_parser_context.c

@@ -6,7 +6,6 @@
 #include <libxml/parserInternals.h>
 
 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);