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.

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

Fix segfaults in destructor using xmlDeregisterNodeDefault callback #321

Closed
wants to merge 26 commits into from

Conversation

rchipka
Copy link
Member

@rchipka rchipka commented Jul 14, 2015

Libxml has a few functions that accept callbacks. One of those functions is xmlDeregisterNodeDefault. This function sets the default callback function that is called when any node is "deregistered" (i.e. before freeing). The callback is also called for all of the node's children.

We can use this function to prevent bad access related segmentation faults by setting a flag saying that the xmlNode has already been freed. The XmlNode destructor code will then check the flag and return if it has already been freed.

In libxmljs.cc:

// set up our callback function that will set the flag

void flagNode(xmlNode* node) {
  if (node->_private) {
    (static_cast<XmlNode*>(node->_private))->isFree = true;
  }
  return;
}

// set the callback function to be called

LibXMLJS::LibXMLJS()
{
    xmlDeregisterNodeDefault(flagNode);

....

}

In xml_node.h:

class XmlNode : public node::ObjectWrap {
public:
    bool isFree;

In xml_node.cc:

// The constructor will set the default value to false

XmlNode::XmlNode(xmlNode* node) : xml_obj(node) {
    this->isFree = false;

...
}


// The destructor will check the value

XmlNode::~XmlNode() {
    if (this->isFree) {
      return;
    }

...
}

Now every time an xmlNode (or any of its children) is freed, the callback will set a flag on the libxmljs Node object saying that the xmlNode has already been freed. Since the destructor will have this information it will not attempt to free the node or access properties of the node causing a segfault. This solution works without any additional ref counting code or memory leaks.

polotek and others added 26 commits October 30, 2014 22:27
This problem was surfaced by issue #268. We weren't doing a proper check for failed attribute creation in the attr method. This still fails and segaults, but the reason is now obvious.
HTML documents have lax parsing and can be created even without a root node. This results in errors when the document defers methods to the root node. This patch catches that circumstance and throws more descriptive errors to let you know what went wrong.
HTML document throws better errors when there is no root node
add assertion to catch failed attribute creation
update nan to 1.5.1 and fix issues with the latest v8
Added support for RELAX NG schema validation
package.json: Add license attribute
Bindings for list of features from xmlHasFeature
@rchipka rchipka closed this Jul 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants