Fix three uses of a stale address after GC compaction - #232
Conversation
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.
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.
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<garbage> 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.
There was a problem hiding this comment.
Verified on Windows / Ruby 4.0.1 (MSVC 14.51 + mingw gcc 15.2) / libxml2 2.15: full suite 406 runs, 0 failures, also clean under GC.auto_compact = true and RUBY_FREE_AT_EXIT=1; no new compiler warnings; no leak from the new allocations. On master, 5 of the 6 new tests crash with the signatures you quoted:
| Test | On master |
|---|---|
test_document.rb#test_gc_compaction_updates_document_registry |
[BUG] Segmentation fault |
test_node.rb#test_gc_compaction_updates_node_registry |
[BUG] try to mark T_NONE object (… parent: … LibXML::XML::Node) |
test_parser.rb#test_io_gc_compaction |
[BUG] Segmentation fault |
test_html_parser.rb#test_io_gc_compaction |
[BUG] Segmentation fault |
test_reader.rb#test_io_gc_compaction |
[BUG] Segmentation fault |
test_reader.rb#test_string_gc_compaction |
passes — see inline comment |
The registry dcompact design is right, and for the reason you give: the data pointer is the key. I audited every rxml_registry_register site — Document + managed Node is indeed the complete set. Line-level notes are inline.
Your questions:
- Yes,
rb_gc_mark_movableis now sound — the node stores no documentVALUE, it re-looks it up from the registry on every mark, so there is nothing for a node-sidedcompactto repair. Please do it as a separate change though. - Keep
ctxt->_private. In 2.15 the field is still public and notXML_DEPRECATED_MEMBER(no warning), unlikerecovery/lastErrornearby. Its doc comment does point atxmlCtxtGetPrivate()/xmlCtxtSetPrivate(), but those would need a version guard given theLIBXML_VERSION >= 20605guards still in this tree. Follow-up. - Fine, with one correction: the
ID2SYM-assigned-to-an-IDbug did not stop the parser contexts from retaining the io.rb_ivar_setstill stored it, just under a key that isn't a valid instance-variable id — invisible toinstance_variables, but alive. Confirmed onlyReaderexposed@io, so that part is right; worth a CHANGELOG line since it changes observably.
One thing not in the diff: ruby_xml_sax_parser.c:82 puts a raw VALUE in ctxt->userData, read back in ruby_xml_sax2_handler.c — same bug class as #2. I couldn't trigger it (the value stays pinned via the live rxml_sax_parser_parse frame), so it's latent. Worth a follow-up issue.
cfis
left a comment
cfis
left a comment
There was a problem hiding this comment.
Inline notes for the minor items from my earlier review.
| 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)); | ||
| } |
There was a problem hiding this comment.
Guarding on st_lookup is what makes this GC-safe: st_insert on an existing key only rewrites the value, so nothing allocates inside the reference-update phase. Worth stating here, since an unconditional insert could grow the table.
| 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. */ |
There was a problem hiding this comment.
Worth adding the other half of the invariant: a registering type also needs RUBY_TYPED_FREE_IMMEDIATELY, so its dfree (and the unregister) runs during sweep, before gc_update_references calls dcompact. Both current types have it; without it rxml_registry_update could be handed a freed slot.
| 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. */ |
There was a problem hiding this comment.
XML::Reader embeds this struct by value and must not call rxml_io_context_free, so "MUST" is too strong as written. Suggest splitting the contract: _new/_free for the heap case (the two parser contexts), _init for the embedded case.
|
|
||
| xreader = xmlReaderForMemory(StringValueCStr(string), (int)RSTRING_LEN(string), | ||
| /* Reject a string that contains a null character. */ | ||
| StringValueCStr(string); |
There was a problem hiding this comment.
Since xmlReaderForMemory takes an explicit length, this has nothing to do with NUL-termination any more — it's purely to preserve the old ArgumentError on an embedded NUL. Worth saying so, otherwise it reads like a requirement of the C call.
| # 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)] |
There was a problem hiding this comment.
Heads-up: this test can't fail on newer libxml2. On 2.15.2 xmlReaderForMemory copies the buffer, so it passes on unpatched master — I confirmed by mutating the source string in place after constructing the reader (via []= and via tr!); the reader was unaffected both ways. Keep the fix, but the comment should say the test only detects the defect on older libxml2.
| holder = [LibXML::XML::Reader.string(build_large_xml)] | ||
|
|
||
| compact_heap | ||
| 20_000.times { |i| +"churn #{i}" } |
There was a problem hiding this comment.
The unary + is a no-op here (an interpolated string is already mutable) and the value is discarded, so this is just 20_000.times { |i| "churn #{i}" }.
|
|
||
| # A node wrapper marks its document through the registry. A stale entry | ||
| # sends a dead address to the garbage collector. | ||
| holder[0].root |
There was a problem hiding this comment.
This wrapper is discarded, so it may be collected before GC.start and the mark path may never run. Bind it, as test_node.rb does with child. (The assert_equal above is what actually segfaults on master, so the test still does its job — this line just isn't pulling its weight.)
Summary
This pull request fixes issue #231. Three parts of the extension keep an address
into memory that the garbage collector controls. GC compaction moves the target.
The extension then uses the old address.
One commit fixes one defect. Each commit adds a regression test.
6553637VALUEas the read contextfc5225eXML::Reader.stringdoes not keep the String64c0aa8Test environment
Each test fails on the code before the commit. Each test passes after the commit.
Each test result is the same in 3 runs.
1. The pointer registry keeps raw wrapper addresses
ruby_xml_registry.cmaps anxmlDocPtror anxmlNodePtrto its Ruby wrapper.It keeps each wrapper as a raw machine address. GC compaction moves the wrapper,
but the address in the registry stays the same. The address is then dead.
Mark functions read the registry.
rxml_node_mark,rxml_dtd_markandrxml_reader_marksend the address torb_gc_mark. The garbage collector thenstops the process:
The report says that
dcompactcannot repair the registry, becausedcompactreceives the data pointer and not the
VALUE. That statement is not correct.The data pointer is the key of the registry entry. So each type repairs its
own entry.
The new function
rxml_registry_updatedoes this. It finds the entry, callsrb_gc_locationon the stored address, and stores the result.Two types add entries to the registry, so two types get a
dcompactfunction:the Document type and the managed Node type.
2. libxml2 receives a raw
VALUEas the read contextXML::Parser::Context.io,XML::HTMLParser::Context.ioandXML::Reader.iogive libxml2 a raw
VALUEas the read context. libxml2 keeps the context andcalls
rxml_read_callbackmuch later. GC compaction moves the IO object in thattime. The callback then uses a dead address:
The public methods
XML::Parser.io,XML::Document.io,XML::SaxParser.io,XML::HTMLParser.ioandXML::Reader.ioall use one of these three methods.XML::Writeralready shows the correct pattern. It gives libxml2 a struct thatholds the
VALUE, and it marks thatVALUEfrom itsdmarkfunction.rb_gc_markpins the object, so the address in the struct stays correct.This pull request adds
rxml_io_contextfor the same pattern:ctxt->_private. libxml2 neveruses that field.
xmlTextReaderis opaque. SoXML::Readernow wraps a newrxml_reader_objectstruct. The struct holds the reader and the read context.The commit also removes the
@ioinstance variable from the three classes. Themark function now keeps the IO object alive. Please note that in the two parser
contexts the instance variable never worked. The code assigned the result of
ID2SYMto anID, and then passed it torb_ivar_set. The result is thatXML::Parser::Context.io(io).instance_variablesis empty.3.
XML::Reader.stringdoes not keep the StringXML::Reader.stringcallsxmlReaderForMemorywith the address of the buffer ofthe Ruby String.
xmlReaderForMemorydoes not copy the buffer. libxml2 readsfrom the buffer on each call to
XML::Reader#read. The reader does not keep theString, so the garbage collector frees the String. The reader then reads free
memory:
The corrupt 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 a large document costs
no more memory. The copy also protects the reader if the program changes the
original String.
rb_gc_markpins the copy. So GC compaction cannot move a shortString that holds its bytes inside the object.
Test method
Each regression test keeps the object under test in an
Array. A local variableis not correct for this test. The machine stack scan pins a local variable, so an
object in a local variable never moves.
The test for
XML::Reader.stringbuilds the String at run time and then makes thegarbage collector reuse the memory. A short literal String stays in place, so a
test with a literal String passes for the wrong reason.
test/test_helper.rbgets a newcompact_heapmethod. The method skips the testif the ruby build does not support compaction.
Results
ef4b8eb(before)The suite also passes with
RUBY_FREE_AT_EXIT=1.Questions
rxml_node_markstill usesrb_gc_markfor the document. That call pins thedocument. With the registry repaired,
rb_gc_mark_movableis now correct andlets the document move. Do you want that change?
ctxt->_private. Recent libxml2 versions make thestructures opaque. Do you prefer a wrapper struct for these two classes also?
@ioinstance variable acceptable? No code in therepository reads it. Please note one difference between the three classes. In
the two parser contexts the instance variable never existed, because of the
ID2SYMdefect above. InXML::Readerit did exist. So the removal is avisible change for
XML::Readeronly. I can keep the instance variable thereif you prefer.