Skip to content
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.

Sign up for GitHub

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

Correctly handle xmlNs memory in xpath query results #1362

Closed
wants to merge 7 commits into from
Closed

Correctly handle xmlNs memory in xpath query results #1362

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
409379b
Added a test case for #1155.
flavorjones Nov 9, 2014
395557b
Reverting a portion of 2675a75
flavorjones Sep 30, 2015
b46dc93
Reverting a portion of 2675a75
flavorjones Sep 30, 2015
3cd1c04
Nokogiri_wrap_xml_node_set handles NULL
flavorjones Oct 1, 2015
af7ccba
DRY up type checks in xml_node_set.c
flavorjones Oct 1, 2015
56d62e0
normalize how we remove nodes from a node set
flavorjones Oct 1, 2015
8ec407d
Fix memory management of xmlNs xpath results
flavorjones Oct 1, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 49 additions & 13 deletions ext/nokogiri/xml_namespace.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,34 @@

VALUE cNokogiriXmlNamespace ;

static void dealloc_namespace(xmlNsPtr ns)
{
/*
*
* this deallocator is only used for namespace nodes that are part of an xpath
* node set.
*
* see Nokogiri_wrap_xml_namespace() for more details.
*
*/
NOKOGIRI_DEBUG_START(ns) ;
if (ns->href) {
xmlFree((xmlChar *)ns->href);
}
if (ns->prefix) {
xmlFree((xmlChar *)ns->prefix);
}
xmlFree(ns);
NOKOGIRI_DEBUG_END(ns) ;
}


int Nokogiri_namespace_eh(xmlNodePtr node)
{
return (node->type == XML_NAMESPACE_DECL);
}


/*
* call-seq:
* prefix
Expand Down Expand Up @@ -34,21 +62,37 @@ static VALUE href(VALUE self)
return NOKOGIRI_STR_NEW2(ns->href);
}

static int part_of_an_xpath_node_set_eh(xmlNsPtr node)
{
return (node->next && ! Nokogiri_namespace_eh(node->next));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this check really correct? I did not look into the libxml sources, but it looks strange to me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree that it's strange, but that's what libxml2 is doing. Which is why I wrapped it in a function with a semantic name.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See https://github.com/GNOME/libxml2/blob/master/xpath.c#L4190-L4191 for original source. Also check out the comments I made in xml_node_set.c in this series of patches.

For a better explanation, I'd recommend reading the patches in order, I tried to tell a story with them.

}

VALUE Nokogiri_wrap_xml_namespace(xmlDocPtr doc, xmlNsPtr node)
{
VALUE ns, document, node_cache;

assert(doc->type == XML_DOCUMENT_NODE || doc->type == XML_HTML_DOCUMENT_NODE);
assert(doc->_private);

if(node->_private)
if (node->_private) {
return (VALUE)node->_private;

ns = Data_Wrap_Struct(cNokogiriXmlNamespace, 0, 0, node);
}

document = DOC_RUBY_OBJECT(doc);

node_cache = rb_iv_get(document, "@node_cache");
rb_ary_push(node_cache, ns);
if (part_of_an_xpath_node_set_eh(node)) {
/*
* this is a duplicate returned as part of an xpath query node set, and so
* we need to make sure we manage this memory.
*
* see comments in xml_node_set.c for more details.
*/
ns = Data_Wrap_Struct(cNokogiriXmlNamespace, 0, dealloc_namespace, node);
} else {
ns = Data_Wrap_Struct(cNokogiriXmlNamespace, 0, 0, node);
node_cache = rb_iv_get(document, "@node_cache");
rb_ary_push(node_cache, ns);
}

rb_iv_set(ns, "@document", DOC_RUBY_OBJECT(doc));

Expand All @@ -57,14 +101,6 @@ VALUE Nokogiri_wrap_xml_namespace(xmlDocPtr doc, xmlNsPtr node)
return ns;
}

VALUE Nokogiri_wrap_xml_namespace2(VALUE document, xmlNsPtr node)
{
xmlDocPtr doc;
Data_Get_Struct(document, xmlDoc, doc) ;
return Nokogiri_wrap_xml_namespace(doc, node);
}


void init_xml_namespace()
{
VALUE nokogiri = rb_define_module("Nokogiri");
Expand Down
Loading