Fix GH-11288 and GH-11289 and GH-11290 and GH-9142: DOMExceptions and segfaults with replaceWith #11299
Conversation
…ception with replaceWith This replaces the implementation of before and after with one following the spec very strictly, instead of trying to figure out the state we're in by looking at the pointers. Also relaxes the condition on text node copying to prevent working on a stale node pointer.
nielsdos
changed the title
|
Ugh Travis fails because the output formatting of saveHTML is different: some newlines are added in the output and some not, but the result is correct... I'll do a postprocess step that just strips newlines I guess :/ EDIT: pushed a commit to fix output formatting due to different libxml versions, hopefully this fixes it otherwise I'll take a look again. |
nielsdos
changed the title
ext/dom/parentnode.c
Outdated
| parentNode = nextsib->parent; | ||
| beforefirstchild = !prevsib; | ||
|
|
||
| /* Spec step 2 appears to be handled by dom_zvals_to_fragment */ |
There was a problem hiding this comment.
I'm confused why this is handled explicitly in the after version but not in the before one. Would be better to be consistent in both functions.
There was a problem hiding this comment.
Sorry, I thought it was fine first because of the document check in dom_zvals_to_fragment. Then I realised it was not and only commented the after version and forgot about this one.
In fact, I believe both before&after should throw HIERARCHY_REQUEST_ERR instead of NO_MODIFICATION_ALLOWED: this is because not having a parent is an invalid hierarchy while NO_MODIFICATION_ALLOWED is used for attempting to modify read-only stuff.
I changed both before and after to throw HIERARCHY_REQUEST_ERR if spec step 2 fails.
There was a problem hiding this comment.
Makes sense for the HIERARCHY_REQUEST_ERR 👍
| @@ -0,0 +1,20 @@ | |||
| --TEST-- | |||
| GH-9142 (DOMChildNode replaceWith() double-free error when replacing elements not separated by any whitespace) | |||
There was a problem hiding this comment.
I'm confused as to how replaceWith is getting affected, as it seems only thebefore and after methods seem to have fixes. Is it because the replace method "call" the two others ones somehow?
There was a problem hiding this comment.
replaceWith calls after, and then remove.
Lines 1254 to 1255 in adb3d52
So spec compliance problem caused problems with replaceWith as well, indirectly.
There was a problem hiding this comment.
Ahhhhhhh right, that makes snese then :)
|
Thanks for the review, George. |
This replaces the implementation of before and after with one following the spec very strictly, instead of trying to figure out the state we're in by looking at the pointers. Also relaxes the condition on text node copying to prevent working on a stale node pointer.