Skip to content

DOMChildNode replaceWith() double-free error when replacing elements not separated by any whitespace #9142

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

Closed
bakerkretzmar opened this issue Jul 25, 2022 · 6 comments

Comments

@bakerkretzmar
Copy link

bakerkretzmar commented Jul 25, 2022

Description

Two XML/HTML elements that are immediately following each other, without any space in between them, cause a 'double free' error if you try to call replaceWith() on them.

$document = '<var>One</var><var>Two</var>';

($dom = new DOMDocument('1.0', 'UTF-8'))->loadHTML($document);

foreach ((new DOMXPath($dom))->query('//var') as $var) {
    $var->replaceWith($dom->createElement('p', $var->nodeValue));
}

var_dump($dom->saveHTML());

From 3v4l.org (note that the first element is successfully replaced):

string(158) "<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.0 Transitional//EN" "http://www.w3.org/TR/REC-html40/loose.dtd">
<html><body><p>One</p><var>Two</var></body></html>
"
free(): double free detected in tcache 2

Process exited with code 134.

Locally (macOS, inside a PHPUnit test, also exits with code 134):

php(1575,0x1016a8580) malloc: *** error for object 0x600002a642f0: pointer being freed was not allocated
php(1575,0x1016a8580) malloc: *** set a breakpoint in malloc_error_break to debug

Expected (both <var>s replaced and no error):

string(158) "<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.0 Transitional//EN" "http://www.w3.org/TR/REC-html40/loose.dtd">
<html><body><p>One</p><p>Two</p></body></html>
"

Note:

  • There's no error if the two <var> elements are separated by a line break or space.
  • This happens with any elements, not just custom ones (e.g. <span>, <p>).

https://3v4l.org/Y2vJc

PHP Version

8.0 and 8.1

Operating System

Whatever 3v4l.org is using, and macOS

@NathanFreeman
Copy link
Contributor

I can not reproduce it on my centos server but I detect memory leak.
It maybe cause by memory leak.

@hwde
Copy link
Contributor

hwde commented Jul 27, 2022

I could reproduce it locally, here is a bt:

#0  __pthread_kill_implementation (threadid=281474839537280, signo=signo@entry=6, no_tid=no_tid@entry=0) at ./nptl/pthread_kill.c:44
#1  0x0000fffff799f254 in __pthread_kill_internal (signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:78
#2  0x0000fffff795a67c in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
#3  0x0000fffff7947130 in __GI_abort () at ./stdlib/abort.c:79
#4  0x0000fffff7993308 in __libc_message (action=action@entry=do_abort, fmt=fmt@entry=0xfffff7a75238 "%s\n") at ../sysdeps/posix/libc_fatal.c:155
#5  0x0000fffff79a957c in malloc_printerr (str=str@entry=0xfffff7a70598 "free(): double free detected in tcache 2") at ./malloc/malloc.c:5664
#6  0x0000fffff79ab8b4 in _int_free (av=0xfffff7abcb10 <main_arena>, p=0xaaaaabe59f00, have_lock=0) at ./malloc/malloc.c:4473
#7  0x0000fffff79adc84 in __GI___libc_free (mem=<optimized out>) at ./malloc/malloc.c:3391
#8  0x0000fffff7d93b34 in xmlFreeNodeList () from /lib/aarch64-linux-gnu/libxml2.so.2
#9  0x0000fffff7d93e44 in xmlFreeDoc () from /lib/aarch64-linux-gnu/libxml2.so.2
#10 0x0000aaaaaac4dce0 in php_libxml_decrement_doc_ref (object=0xfffff4675040) at /home/heiko/php-src/ext/libxml/libxml.c:1301
#11 0x0000aaaaaad14f24 in dom_objects_free_storage (object=0xfffff4675058) at /home/heiko/php-src/ext/dom/php_dom.c:988
#12 0x0000aaaaab34c23c in zend_objects_store_del (object=0xfffff4675058) at /home/heiko/php-src/Zend/zend_objects_API.c:193
#13 0x0000aaaaab17d180 in rc_dtor_func (p=0xfffff4675058) at /home/heiko/php-src/Zend/zend_variables.c:57
#14 0x0000aaaaab17d368 in i_zval_ptr_dtor (zval_ptr=0xffffffffcea8) at /home/heiko/php-src/Zend/zend_variables.h:44
#15 zval_ptr_dtor (zval_ptr=0xffffffffcea8) at /home/heiko/php-src/Zend/zend_variables.c:84
#16 0x0000aaaaab1aac30 in _zend_hash_del_el_ex (prev=0x0, p=0xfffff4657300, idx=8, ht=0xaaaaabcab050 <executor_globals+304>) at /home/heiko/php-src/Zend/zend_hash.c:1330
#17 _zend_hash_del_el (p=0xfffff4657300, idx=8, ht=0xaaaaabcab050 <executor_globals+304>) at /home/heiko/php-src/Zend/zend_hash.c:1353
#18 zend_hash_reverse_apply (ht=0xaaaaabcab050 <executor_globals+304>, apply_func=0xaaaaab15d9ec <zval_call_destructor>) at /home/heiko/php-src/Zend/zend_hash.c:1924
#19 0x0000aaaaab15dd74 in shutdown_destructors () at /home/heiko/php-src/Zend/zend_execute_API.c:246
#20 0x0000aaaaab181e38 in zend_call_destructors () at /home/heiko/php-src/Zend/zend.c:1224
#21 0x0000aaaaab0b3eec in php_request_shutdown (dummy=0x0) at /home/heiko/php-src/main/main.c:1803
#22 0x0000aaaaab35fbe0 in do_cli (argc=2, argv=0xaaaaabcc41d0) at /home/heiko/php-src/sapi/cli/php_cli.c:1111
#23 0x0000aaaaab360340 in main (argc=2, argv=0xaaaaabcc41d0) at /home/heiko/php-src/sapi/cli/php_cli.c:1337

Here is another example which shows the same error:

$document = '<var>One</var><var>Two</var>';

($dom = new DOMDocument('1.0', 'UTF-8'))->loadHTML($document);

foreach ((new DOMXPath($dom))->query('//var') as $var) {
	$var->after($dom->createElement('p', $var->nodeValue));
	$var->remove();
}

var_dump($dom->saveHTML());

NathanFreeman added a commit to NathanFreeman/php-src that referenced this issue Jul 27, 2022
@NathanFreeman
Copy link
Contributor

NathanFreeman commented Jul 27, 2022

@bakerkretzmar @hwde @cmb69 @beberlei
Hi, I have made a pr for this bug.
It would be greatly appreciated if you could review it.

@bakerkretzmar
Copy link
Author

@NathanFreeman thanks! where is the PR?

@NathanFreeman
Copy link
Contributor

NathanFreeman commented Jul 27, 2022

@bakerkretzmar
#8729
Thanks!

@hwde
Copy link
Contributor

hwde commented Jul 27, 2022

Works for me.

nielsdos added a commit to nielsdos/php-src that referenced this issue May 22, 2023
…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 added a commit that referenced this issue May 25, 2023
… segfaults 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.

Closes GH-11299.
nielsdos added a commit that referenced this issue May 25, 2023
* PHP-8.1:
  Fix GH-11288 and GH-11289 and GH-11290 and GH-9142: DOMExceptions and segfaults with replaceWith
nielsdos added a commit that referenced this issue May 25, 2023
* PHP-8.2:
  Fix GH-11288 and GH-11289 and GH-11290 and GH-9142: DOMExceptions and segfaults with replaceWith
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
@hwde @cmb69 @nielsdos @bakerkretzmar @NathanFreeman and others