Fix three uses of a stale address after GC compaction - #232
Open
jeremy wants to merge 3 commits into
Open
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.