Fix #80927: Removing documentElement after creating attribute node: p… #11892
Conversation
…ossible use-after-free
|
I don't see another way to solve this issue ATM. The main problem is that libxml2 doesn't have a reference count on the namespace declaration. I understand why this is, it's non-trivial to keep track of (e.g. clones, imports, adoptions, ...) and normally it doesn't matter. We don't actually need to save the namespace declaration if we know the subtree it belongs to has no references from userland. However, we can't know that without traversing the whole subtree (=> slow), or without adding some subtree metadata (=> also slow). So we have to assume we need to save everything. However, namespace declarations are quite rare in comparison to other node types. Most node types are either elements, text or attributes. And you only need one namespace declaration per namespace (in principle). So I expect the number of namespace declarations to be low for an average XML document. In the worst possible case we have to save all namespace declarations when we for example remove the whole document. But given the above reasoning this likely won't be a lot of declarations even in the worst case. I'm going to mark this as ready, and update the comment in the code to include some of this text for future reference. |
ext/dom/tests/bug80927.phpt
Outdated
| var_dump($a->prefix); | ||
| } | ||
|
|
||
| function test2($variation) { |
There was a problem hiding this comment.
You could use an Enum and type against this for the variation arg
ext/libxml/libxml.c
Outdated
| php_libxml_set_old_ns_list(node->doc, ns, last); | ||
| node->nsDef = NULL; | ||
| } | ||
| ZEND_FALLTHROUGH; |
There was a problem hiding this comment.
I get that this how it was previously written, and the default case is very small and clear. But I wonder if it just doesn't make more sense to keep it self-contained and have:
xmlFreeNode(node);
break;here too.
…ossible use-after-free Closes phpGH-11892.
…ossible use-after-free Closes phpGH-11892.
…ossible use-after-free
Not 100% convinced that this solution is optimal. Also targeting master only because this is a non-trivial change in how the destruction works.
Marking as draft for now until I convince myself more that there's no better alternative.