New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Different usage of _private inside xmlNode resulting in crashes #939

Closed
dbussink opened this Issue Jul 16, 2013 · 6 comments

Comments

Projects
None yet
3 participants
@dbussink
Contributor

dbussink commented Jul 16, 2013

When running the test suite on Rubinius, it results in consistent crashes. From what I've gathered, it seems that assumptions about what _private contains in xmlNode are not always correct.

Here a VALUE is stored inside _private:

https://github.com/sparklemotion/nokogiri/blob/master/ext/nokogiri/xml_node.c#L1441-L1442

The crash happens when calling the mark function for an XML node:

#6  0x00007ffff424636c in mark (node=0x3be03b0) at ../../../../ext/nokogiri/xml_node.c:17

https://github.com/sparklemotion/nokogiri/blob/master/ext/nokogiri/xml_node.c#L17

This uses the DOC_RUBY_OBJECT macro. This macro assumes that _private inside the document for this node is a nokogiriTuplePtr.

https://github.com/sparklemotion/nokogiri/blob/master/ext/nokogiri/xml_document.h#L18

In this case however, node->doc is not a document, but a XML_DOCUMENT_FRAG_NODE, so actually it's an xmlNode that stores a VALUE in _private:

(gdb) p node
$16 = (xmlNodePtr) 0x3be03b0
(gdb) p *node
$17 = {_private = 0x28779a0, type = XML_COMMENT_NODE, name = 0x7fffef8f3704 "comment", children = 0x0, last = 0x0,
  parent = 0x0, next = 0x0, prev = 0x0, doc = 0x2603850, ns = 0x0, content = 0x3be0430 "moo", properties = 0x0,
  nsDef = 0x0, psvi = 0x0, line = 0, extra = 0}
(gdb) p *node->doc
$18 = {_private = 0x2877ca0, type = XML_DOCUMENT_FRAG_NODE, name = 0x0, children = 0x3161fc0, last = 0x3161fc0,
  parent = 0x0, next = 0x0, prev = 0x0, doc = 0x5085590, compression = 0, standalone = 0, intSubset = 0x0,
  extSubset = 0x0, oldNs = 0x0, version = 0x0, encoding = 0x0, ids = 0x41, refs = 0x2603850,
  URL = 0x2603850 "�|\207\002", charset = 39860304, dict = 0x0, psvi = 0x3be04b0, parseFlags = 0, properties = 0}

This can also be seen because the _private pointer of node and node->doc are both very similar and allocated in the memory area where Rubinius allocates the storage behind a VALUE.

I have no idea why this doesn't also crash on MRI, but I suspect it's the conservative garbage collection that doesn't try to GC something inside rb_gc_mark that doesn't look like a valid heap pointer. That way it works as an implementation side effect and actually still is a bug in the implementation.

@dbussink

This comment has been minimized.

Show comment
Hide comment
@dbussink

dbussink Aug 20, 2013

Contributor

Could anyone shed some light on this issue? Is there a way this can be fixed? I think this could crash MRI as well at unexpected moments.

Contributor

dbussink commented Aug 20, 2013

Could anyone shed some light on this issue? Is there a way this can be fixed? I think this could crash MRI as well at unexpected moments.

@dbussink

This comment has been minimized.

Show comment
Hide comment
@dbussink

dbussink Oct 21, 2013

Contributor

Anyone who could help out with this issue? I'd love to help out with a fix, but don't really have an idea whether any of you have good suggestions for what would be a good approach to do so.

Contributor

dbussink commented Oct 21, 2013

Anyone who could help out with this issue? I'd love to help out with a fix, but don't really have an idea whether any of you have good suggestions for what would be a good approach to do so.

@krasnoukhov

This comment has been minimized.

Show comment
Hide comment
@krasnoukhov

krasnoukhov Nov 7, 2013

I'm also seeing a lot of crashes on Rubinius. Any updates?

krasnoukhov commented Nov 7, 2013

I'm also seeing a lot of crashes on Rubinius. Any updates?

@dbussink dbussink referenced this issue Nov 21, 2013

Closed

crash report #2799

@tomquas

This comment has been minimized.

Show comment
Hide comment
@tomquas

tomquas Nov 21, 2013

yeah, no chance to deploy reliable xmpp bots w/o a fix for this one if you sit on adhearsion/blather. my bots crash way too often.

tomquas commented Nov 21, 2013

yeah, no chance to deploy reliable xmpp bots w/o a fix for this one if you sit on adhearsion/blather. my bots crash way too often.

dbussink added a commit to dbussink/nokogiri that referenced this issue Nov 21, 2013

Fix segfault when doing marking and doc is not a document node
This fixes the problem where the doc node can actually also be a
XML_DOCUMENT_FRAG_NODE. In that case it's structured like a node which
means it stores a VALUE inside _private.

This checks the type of the code and uses the correct marking for the
specific cases.

Fixes sparklemotion#939
@dbussink

This comment has been minimized.

Show comment
Hide comment
@dbussink

dbussink Nov 21, 2013

Contributor

I've created a pull request that fixes the problem. This gets all the tests passing on Rubinius as well.

Contributor

dbussink commented Nov 21, 2013

I've created a pull request that fixes the problem. This gets all the tests passing on Rubinius as well.

@krasnoukhov

This comment has been minimized.

Show comment
Hide comment
@krasnoukhov

krasnoukhov Nov 21, 2013

Great job @dbussink, I'll give it a try

krasnoukhov commented Nov 21, 2013

Great job @dbussink, I'll give it a try

dbussink added a commit to dbussink/nokogiri that referenced this issue Nov 21, 2013

Fix segfault when doing marking and doc is not a document node
This fixes the problem where the doc node can actually also be a
XML_DOCUMENT_FRAG_NODE. In that case it's structured like a node which
means it stores a VALUE inside _private.

This checks the type of the code and uses the correct marking for the
specific cases.

Fixes sparklemotion#939

@YorickPeterse YorickPeterse referenced this issue Dec 20, 2013

Closed

GC Crash #2844

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment