Memory unsafety with `HashTable`s treated as weak map

[TimWolla]

Description

Apply the following patch to zend_test:

diff --git i/ext/zend_test/php_test.h w/ext/zend_test/php_test.h
index 09391d8137a..7ec6f543123 100644
--- i/ext/zend_test/php_test.h
+++ w/ext/zend_test/php_test.h
@@ -50,7 +50,7 @@ ZEND_BEGIN_MODULE_GLOBALS(zend_test)
        int observer_fiber_switch;
        int observer_fiber_destroy;
        int observer_execute_internal;
-       HashTable global_weakmap;
+       HashTable *global_weakmap;
        int replace_zend_execute_ex;
        int register_passes;
        bool print_stderr_mshutdown;
diff --git i/ext/zend_test/test.c w/ext/zend_test/test.c
index af60dffb4bf..1ca94983465 100644
--- i/ext/zend_test/test.c
+++ w/ext/zend_test/test.c
@@ -381,7 +381,7 @@ static ZEND_FUNCTION(zend_weakmap_attach)
                        Z_PARAM_ZVAL(value)
        ZEND_PARSE_PARAMETERS_END();
 
-       if (zend_weakrefs_hash_add(&ZT_G(global_weakmap), obj, value)) {
+       if (zend_weakrefs_hash_add(ZT_G(global_weakmap), obj, value)) {
                Z_TRY_ADDREF_P(value);
                RETURN_TRUE;
        }
@@ -396,13 +396,13 @@ static ZEND_FUNCTION(zend_weakmap_remove)
                        Z_PARAM_OBJ(obj)
        ZEND_PARSE_PARAMETERS_END();
 
-       RETURN_BOOL(zend_weakrefs_hash_del(&ZT_G(global_weakmap), obj) == SUCCESS);
+       RETURN_BOOL(zend_weakrefs_hash_del(ZT_G(global_weakmap), obj) == SUCCESS);
 }
 
 static ZEND_FUNCTION(zend_weakmap_dump)
 {
        ZEND_PARSE_PARAMETERS_NONE();
-       RETURN_ARR(zend_array_dup(&ZT_G(global_weakmap)));
+       RETURN_ARR(zend_array_dup(ZT_G(global_weakmap)));
 }
 
 static ZEND_FUNCTION(zend_get_current_func_name)
@@ -1424,7 +1424,8 @@ PHP_MSHUTDOWN_FUNCTION(zend_test)
 
 PHP_RINIT_FUNCTION(zend_test)
 {
-       zend_hash_init(&ZT_G(global_weakmap), 8, NULL, ZVAL_PTR_DTOR, 0);
+       ZT_G(global_weakmap) = emalloc(sizeof(HashTable));
+       zend_hash_init(ZT_G(global_weakmap), 8, NULL, ZVAL_PTR_DTOR, 0);
        ZT_G(observer_nesting_depth) = 0;
        zend_test_mm_custom_handlers_rinit();
        return SUCCESS;
@@ -1432,7 +1433,8 @@ PHP_RINIT_FUNCTION(zend_test)
 
 PHP_RSHUTDOWN_FUNCTION(zend_test)
 {
-       zend_weakrefs_hash_destroy(&ZT_G(global_weakmap));
+       zend_weakrefs_hash_destroy(ZT_G(global_weakmap));
+       efree(ZT_G(global_weakmap));
 
        if (ZT_G(zend_test_heap))  {
                free(ZT_G(zend_test_heap));

The following code:

<?php
$e = new Exception();
$a = new stdClass();
zend_weakmap_attach($e, $a);
unset($a);
gc_collect_cycles();

Resulted in this output:

==884545== Memcheck, a memory error detector
==884545== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==884545== Using Valgrind-3.22.0 and LibVEX; rerun with -h for copyright info
==884545== Command: sapi/cli/php weakmap.php
==884545== 
==884545== Invalid read of size 4
==884545==    at 0xB77ACE: gc_scan_black (zend_gc.c:868)
==884545==    by 0xB78DD9: gc_scan (zend_gc.c:1301)
==884545==    by 0xB79419: gc_scan_roots (zend_gc.c:1448)
==884545==    by 0xB7A90C: zend_gc_collect_cycles (zend_gc.c:2009)
==884545==    by 0xA8C145: zif_gc_collect_cycles (zend_builtin_functions.c:170)
==884545==    by 0xAE99CF: ZEND_DO_ICALL_SPEC_RETVAL_UNUSED_HANDLER (zend_vm_execute.h:1344)
==884545==    by 0xB6B1CF: execute_ex (zend_vm_execute.h:58930)
==884545==    by 0xB70C35: zend_execute (zend_vm_execute.h:64398)
==884545==    by 0xC14DCB: zend_execute_script (zend.c:1976)
==884545==    by 0x9B3931: php_execute_script_ex (main.c:2647)
==884545==    by 0x9B3AB7: php_execute_script (main.c:2687)
==884545==    by 0xC174E5: do_cli (php_cli.c:952)
==884545==  Address 0x7bc488c is 4 bytes after a block of size 56 alloc'd
==884545==    at 0x4846828: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==884545==    by 0xA65AC0: __zend_malloc (zend_alloc.c:3561)
==884545==    by 0xA63CAB: _emalloc (zend_alloc.c:2798)
==884545==    by 0x9A925E: zm_activate_zend_test (test.c:1427)
==884545==    by 0xA7392E: zend_activate_modules (zend_API.c:3396)
==884545==    by 0x9B2853: php_request_startup (main.c:1946)
==884545==    by 0xC1734A: do_cli (php_cli.c:920)
==884545==    by 0xC18544: main (php_cli.c:1363)
==884545== 
==884545== Invalid read of size 4
==884545==    at 0xB77B23: gc_scan_black (zend_gc.c:882)
==884545==    by 0xB78DD9: gc_scan (zend_gc.c:1301)
==884545==    by 0xB79419: gc_scan_roots (zend_gc.c:1448)
==884545==    by 0xB7A90C: zend_gc_collect_cycles (zend_gc.c:2009)
==884545==    by 0xA8C145: zif_gc_collect_cycles (zend_builtin_functions.c:170)
==884545==    by 0xAE99CF: ZEND_DO_ICALL_SPEC_RETVAL_UNUSED_HANDLER (zend_vm_execute.h:1344)
==884545==    by 0xB6B1CF: execute_ex (zend_vm_execute.h:58930)
==884545==    by 0xB70C35: zend_execute (zend_vm_execute.h:64398)
==884545==    by 0xC14DCB: zend_execute_script (zend.c:1976)
==884545==    by 0x9B3931: php_execute_script_ex (main.c:2647)
==884545==    by 0x9B3AB7: php_execute_script (main.c:2687)
==884545==    by 0xC174E5: do_cli (php_cli.c:952)
==884545==  Address 0x7bc488c is 4 bytes after a block of size 56 alloc'd
==884545==    at 0x4846828: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==884545==    by 0xA65AC0: __zend_malloc (zend_alloc.c:3561)
==884545==    by 0xA63CAB: _emalloc (zend_alloc.c:2798)
==884545==    by 0x9A925E: zm_activate_zend_test (test.c:1427)
==884545==    by 0xA7392E: zend_activate_modules (zend_API.c:3396)
==884545==    by 0x9B2853: php_request_startup (main.c:1946)
==884545==    by 0xC1734A: do_cli (php_cli.c:920)
==884545==    by 0xC18544: main (php_cli.c:1363)
==884545== 
==884545== 
==884545== HEAP SUMMARY:
==884545==     in use at exit: 0 bytes in 0 blocks
==884545==   total heap usage: 18,859 allocs, 18,859 frees, 3,708,637 bytes allocated
==884545== 
==884545== All heap blocks were freed -- no leaks are possible
==884545== 
==884545== For lists of detected and suppressed errors, rerun with: -s
==884545== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)

But I expected this output instead:

No memory unsafety.

---

The internal weak map API for bare HashTables was added by @bwoebi in 471102e. I've shortly checked with him and he indicated that this got broken in cbf67e4 by @arnaud-lb, because that commit treats anything passed to zend_weakrefs_hash_add() as an object of zend_ce_weakref (i.e. a zend_weakmap struct rather than a bare HashTable).

PHP Version

git master

Operating System

No response

[arnaud-lb]

Indeed. I wasn't aware that using bare HashTables as WeakMaps was supported. In cbf67e4, the GC needs to find all weak references to an IS_OBJ_WEAKLY_REFERENCED object, and doing so treats all ZEND_WEAKREF_TAG_MAP references as WeakMap objects.

I need to figure how to differentiate bare HashTable references from WeakMap references. I guess we can introduce a new tag, e.g. ZEND_WEAKREF_TAG_BARE_HT.

One caveat is that bare HashTables can not generally be considered collectable. E.g. the one in zend_test is not.

[bwoebi]

I think it's fine to just exclude those from collection. If collection is desired, users of the API shall just use a full-fledged weakmap object.

[TimWolla]

users of the API shall just use a full-fledged weakmap object.

It should be noted there, that neither struct _zend_weakmap, nor zend_ce_weakmap are currently exported, which makes it inconvenient to use a full-fledged WeakMap object internally (other than by pulling the CE from the class_table and then calling the object handlers directly; or hardcoding the struct layout of course).

[TimWolla]

One caveat is that bare HashTables can not generally be considered collectable. E.g. the one in zend_test is not.

Collectable referring to “safe to pass into pefree()”? Then I agree with Bob that folks should just not do this.

For my use case, I know that the HashTable that is used as a weak map always has exactly 1 reference and cannot be part of a cycle itself.