Bug #79820 [Opn]: double-free causing heap corruption

Edit report at https://bugs.php.net/bug.php?id=79820&edit=1 ID: 79820 Updated by: nikic@php.net Reported by: christopher dot broadbent at zencontrol dot com Summary: double-free causing heap corruption Status: Open Type: Bug Package: Reproducible crash Operating System: linux debian buster PHP Version: 7.4.7 Block user comment: N Private report: N New Comment: Right you are! Minimal reproducer: <?php class Test { public stdClass $prop; } $rp = new ReflectionProperty(Test::class, 'prop'); $test = new Test; $test->prop = new stdClass; var_dump($rp->getType()->getName()); For PHP 8 this has already been fixed by https://github.com/php/php-src/commit/0e3045ae69d1b80c21b2779df721a4ad8bcda401. Previous Comments: ------------------------------------------------------------------------ [2020-07-15 05:44:51] chris-broadbent at zencontrol dot com The following patch has been added/updated: Patch Name: 0001-Add-refs-to-prop-names-to-avoid-use-after-free Revision: 1594791891 URL: https://bugs.php.net/patch-display.php?bug=79820&patch=0001-Add-refs-to-prop-names-to-avoid-use-after-free&revision=1594791891 ------------------------------------------------------------------------ [2020-07-14 23:53:50] christopher dot broadbent at zencontrol dot com I've made a change to php_reflecation.c lines 5281 to 5302, on 7.4.8 release, inside reflection_property____construct reference = (property_reference*) emalloc(sizeof(property_reference)); if (dynam_prop) { reference->prop.flags = ZEND_ACC_PUBLIC; reference->prop.name = name; reference->prop.doc_comment = NULL; reference->prop.ce = ce; reference->dynamic = 1; } else { reference->prop = *property_info; reference->dynamic = 0; + if (ZEND_TYPE_IS_NAME(reference->prop.type)) { + zend_string_addref(ZEND_TYPE_NAME(reference->prop.type)); + } } reference->unmangled_name = zend_string_copy(name); intern->ptr = reference; intern->ref_type = REF_TYPE_PROPERTY; intern->ce = ce; intern->ignore_visibility = 0; } This fixes the segfault in all the reproduction cases we've got, but I'm not familiar enough with the rest of the code to know if this is correct or causes leaks. ------------------------------------------------------------------------ [2020-07-14 23:27:12] christopher dot broadbent at zencontrol dot com Sorry, I meant to say bump the ref counters on reference->prop.type ------------------------------------------------------------------------ [2020-07-14 23:22:18] christopher dot broadbent at zencontrol dot com This is probably my inexperience with the code-base, but shouldn't the implementation for ZEND_METHOD(reflection_property, __construct) by increment the ref-count for the value copied by reference->prop = *property_info; when dynam_prop is false? It seems to be the thing keeping a reference around to the deallocated string, causing the use-after-free, and I can't see anywhere in the code path where it bumps the reference count. ------------------------------------------------------------------------ [2020-07-14 07:48:04] ondrej@php.net > On which version/commit of PHP were the gdb backtraces gathered? I can't find > any > version of PHP 7.4 that has a zend_string_release call in php_reflection.c:225. The valgrind trace > looks more plausible. If you look carefully, the backtrace matches the function with it's location and zend_string_release is always inlined, so the php_reflection.c:225 is just a red herring as it just got inlined (and the line number marks the location of the affected block where it is used). The packages don't patch php_reflection.c at all. ------------------------------------------------------------------------ The remainder of the comments for this report are too long. To view the rest of the comments, please view the bug report online at https://bugs.php.net/bug.php?id=79820 -- Edit this bug report at https://bugs.php.net/bug.php?id=79820&edit=1