From f02e7576efea03a3fac81eb2f4311fbdcf2b38e1 Mon Sep 17 00:00:00 2001 From: Nikita Popov Date: Thu, 1 Aug 2019 12:55:39 +0200 Subject: [PATCH] Fixed bug #72530 --- Zend/tests/bug71818.phpt | 2 +- Zend/tests/bug72530.phpt | 31 ++++++++++ Zend/tests/gc_011.phpt | 2 + Zend/tests/gc_016.phpt | 4 +- Zend/tests/gc_017.phpt | 4 +- Zend/tests/gc_028.phpt | 4 +- Zend/tests/gc_029.phpt | 6 +- Zend/tests/gc_035.phpt | 2 +- Zend/tests/generators/bug76427.phpt | 2 +- Zend/zend_gc.c | 88 +++++++++++++++++------------ 10 files changed, 99 insertions(+), 46 deletions(-) create mode 100644 Zend/tests/bug72530.phpt diff --git a/Zend/tests/bug71818.phpt b/Zend/tests/bug71818.phpt index e09255ddac206..67094c09f7bcd 100644 --- a/Zend/tests/bug71818.phpt +++ b/Zend/tests/bug71818.phpt @@ -19,7 +19,7 @@ class MemoryLeak private $things = []; } -ini_set('memory_limit', '10M'); +ini_set('memory_limit', '20M'); for ($i = 0; $i < 100000; ++$i) { $obj = new MemoryLeak(); diff --git a/Zend/tests/bug72530.phpt b/Zend/tests/bug72530.phpt new file mode 100644 index 0000000000000..b70be0b98fb8f --- /dev/null +++ b/Zend/tests/bug72530.phpt @@ -0,0 +1,31 @@ +--TEST-- +Bug #72530: Use After Free in GC with Certain Destructors +--FILE-- +chtg = $this->ryat; + $this->ryat = 1; + } +} + +$o = new ryat; +$o->ryat = $o; +$x =& $o->chtg; + +unset($o); +gc_collect_cycles(); +var_dump($x); + +?> +--EXPECT-- +object(ryat)#1 (2) { + ["ryat"]=> + int(1) + ["chtg"]=> + *RECURSION* +} diff --git a/Zend/tests/gc_011.phpt b/Zend/tests/gc_011.phpt index 9c4cc2cc0e570..d11d7b6b46e4d 100644 --- a/Zend/tests/gc_011.phpt +++ b/Zend/tests/gc_011.phpt @@ -15,6 +15,7 @@ $a->a = $a; var_dump($a); unset($a); var_dump(gc_collect_cycles()); +var_dump(gc_collect_cycles()); echo "ok\n" ?> --EXPECTF-- @@ -23,5 +24,6 @@ object(Foo)#%d (1) { *RECURSION* } __destruct +int(0) int(1) ok diff --git a/Zend/tests/gc_016.phpt b/Zend/tests/gc_016.phpt index f082d60973e88..211f03a605305 100644 --- a/Zend/tests/gc_016.phpt +++ b/Zend/tests/gc_016.phpt @@ -23,6 +23,6 @@ echo "ok\n" ?> --EXPECT-- -> int(0) -int(1) -int(1) +int(0) +int(2) ok diff --git a/Zend/tests/gc_017.phpt b/Zend/tests/gc_017.phpt index 102c2b6bcbe0f..55f381992e698 100644 --- a/Zend/tests/gc_017.phpt +++ b/Zend/tests/gc_017.phpt @@ -32,11 +32,13 @@ unset($a); unset($b); unset($c); var_dump(gc_collect_cycles()); +var_dump(gc_collect_cycles()); echo "ok\n" ?> --EXPECTF-- string(1) "%s" string(1) "%s" string(1) "%s" -int(4) +int(0) +int(1) ok diff --git a/Zend/tests/gc_028.phpt b/Zend/tests/gc_028.phpt index 8dc70fc39706a..fb2ea92c91330 100644 --- a/Zend/tests/gc_028.phpt +++ b/Zend/tests/gc_028.phpt @@ -28,6 +28,8 @@ $bar->foo = $foo; unset($foo); unset($bar); var_dump(gc_collect_cycles()); +var_dump(gc_collect_cycles()); ?> --EXPECT-- -int(2) +int(0) +int(1) diff --git a/Zend/tests/gc_029.phpt b/Zend/tests/gc_029.phpt index 215d0e0e3bc6f..89c55e5ba76d3 100644 --- a/Zend/tests/gc_029.phpt +++ b/Zend/tests/gc_029.phpt @@ -30,6 +30,8 @@ $bar->foo = $foo; unset($foo); unset($bar); var_dump(gc_collect_cycles()); +var_dump(gc_collect_cycles()); ?> ---EXPECTREGEX-- -int\([23]\) +--EXPECT-- +int(0) +int(1) diff --git a/Zend/tests/gc_035.phpt b/Zend/tests/gc_035.phpt index 177c3101f9f11..187af9108bd78 100644 --- a/Zend/tests/gc_035.phpt +++ b/Zend/tests/gc_035.phpt @@ -22,5 +22,5 @@ var_dump(gc_collect_cycles()); var_dump(gc_collect_cycles()); --EXPECT-- int(0) -int(2) int(0) +int(2) diff --git a/Zend/tests/generators/bug76427.phpt b/Zend/tests/generators/bug76427.phpt index 09ec61a340a6f..53851b0f35fe0 100644 --- a/Zend/tests/generators/bug76427.phpt +++ b/Zend/tests/generators/bug76427.phpt @@ -21,4 +21,4 @@ var_dump(gc_collect_cycles()); ?> --EXPECT-- -int(4) +int(2) diff --git a/Zend/zend_gc.c b/Zend/zend_gc.c index e1c2295d740b6..6bc697bd05034 100644 --- a/Zend/zend_gc.c +++ b/Zend/zend_gc.c @@ -141,6 +141,7 @@ #define GC_ROOT 0x0 /* possible root of circular garbage */ #define GC_UNUSED 0x1 /* part of linked list of unused buffers */ #define GC_GARBAGE 0x2 /* garbage to delete */ +#define GC_DTOR_GARBAGE 0x3 /* garbage on which only the dtor should be invoked */ #define GC_GET_PTR(ptr) \ ((void*)(((uintptr_t)(ptr)) & ~GC_BITS)) @@ -151,9 +152,13 @@ ((((uintptr_t)(ptr)) & GC_BITS) == GC_UNUSED) #define GC_IS_GARBAGE(ptr) \ ((((uintptr_t)(ptr)) & GC_BITS) == GC_GARBAGE) +#define GC_IS_DTOR_GARBAGE(ptr) \ + ((((uintptr_t)(ptr)) & GC_BITS) == GC_DTOR_GARBAGE) #define GC_MAKE_GARBAGE(ptr) \ ((void*)(((uintptr_t)(ptr)) | GC_GARBAGE)) +#define GC_MAKE_DTOR_GARBAGE(ptr) \ + ((void*)(((uintptr_t)(ptr)) | GC_DTOR_GARBAGE)) /* GC address conversion */ #define GC_IDX2PTR(idx) (GC_G(buf) + (idx)) @@ -1328,9 +1333,6 @@ static int gc_remove_nested_data_from_buffer(zend_refcounted *ref, gc_root_buffe tail_call: do { if (root) { - GC_TRACE_REF(ref, "removing from buffer"); - gc_remove_from_roots(root); - GC_REF_SET_INFO(ref, 0); root = NULL; count++; } else if (GC_REF_ADDRESS(ref) != 0 @@ -1461,67 +1463,79 @@ ZEND_API int zend_gc_collect_cycles(void) end = GC_G(first_unused); if (gc_flags & GC_HAS_DESTRUCTORS) { - uint32_t *refcounts; - GC_TRACE("Calling destructors"); - // TODO: may be use emalloc() ??? - refcounts = pemalloc(sizeof(uint32_t) * end, 1); - - /* Remember reference counters before calling destructors */ + /* During a destructor call, new externally references to nested data may + * be introduced. These references can be introduced in a way that does not + * modify any refcounts, so we have no real way to detect this situation + * short of rerunning full GC tracing. What we do instead is to only run + * destructors at this point, and leave the actual freeing of the objects + * until the next GC run. */ + + /* Mark all roots for which a dtor will be invoked as DTOR_GARBAGE. Additionally + * color them purple. This serves a double purpose: First, they should be + * considered new potential roots for the next GC run. Second, it will prevent + * their removal from the root buffer by nested data removal. */ idx = GC_FIRST_ROOT; current = GC_IDX2PTR(GC_FIRST_ROOT); while (idx != end) { if (GC_IS_GARBAGE(current->ref)) { p = GC_GET_PTR(current->ref); - refcounts[idx] = GC_REFCOUNT(p); + if (GC_TYPE(p) == IS_OBJECT && !(OBJ_FLAGS(p) & IS_OBJ_DESTRUCTOR_CALLED)) { + zend_object *obj = (zend_object *) p; + if (obj->handlers->dtor_obj != zend_objects_destroy_object + || obj->ce->destructor) { + current->ref = GC_MAKE_DTOR_GARBAGE(obj); + GC_REF_SET_COLOR(obj, GC_PURPLE); + } else { + GC_ADD_FLAGS(obj, IS_OBJ_DESTRUCTOR_CALLED); + } + } } current++; idx++; } - /* Call destructors - * - * The root buffer might be reallocated during destructors calls, - * make sure to reload pointers as necessary. */ + /* Remove nested data for objects on which a destructor will be called. + * This will not remove the objects themselves, as they have been colored + * purple. */ idx = GC_FIRST_ROOT; + current = GC_IDX2PTR(GC_FIRST_ROOT); while (idx != end) { - current = GC_IDX2PTR(idx); - if (GC_IS_GARBAGE(current->ref)) { + if (GC_IS_DTOR_GARBAGE(current->ref)) { p = GC_GET_PTR(current->ref); - if (GC_TYPE(p) == IS_OBJECT - && !(OBJ_FLAGS(p) & IS_OBJ_DESTRUCTOR_CALLED)) { - zend_object *obj = (zend_object*)p; - - GC_TRACE_REF(obj, "calling destructor"); - GC_ADD_FLAGS(obj, IS_OBJ_DESTRUCTOR_CALLED); - if (obj->handlers->dtor_obj != zend_objects_destroy_object - || obj->ce->destructor) { - GC_ADDREF(obj); - obj->handlers->dtor_obj(obj); - GC_DELREF(obj); - } - } + count -= gc_remove_nested_data_from_buffer(p, current); } + current++; idx++; } - /* Remove values captured in destructors */ + /* Actually call destructors. + * + * The root buffer might be reallocated during destructors calls, + * make sure to reload pointers as necessary. */ idx = GC_FIRST_ROOT; - current = GC_IDX2PTR(GC_FIRST_ROOT); while (idx != end) { - if (GC_IS_GARBAGE(current->ref)) { + current = GC_IDX2PTR(idx); + if (GC_IS_DTOR_GARBAGE(current->ref)) { p = GC_GET_PTR(current->ref); - if (GC_REFCOUNT(p) > refcounts[idx]) { - count -= gc_remove_nested_data_from_buffer(p, current); + /* Mark this is a normal root for the next GC run, + * it's no longer garbage for this run. */ + current->ref = p; + /* Double check that the destructor hasn't been called yet. It could have + * already been invoked indirectly by some other destructor. */ + if (!(OBJ_FLAGS(p) & IS_OBJ_DESTRUCTOR_CALLED)) { + zend_object *obj = (zend_object*)p; + GC_TRACE_REF(obj, "calling destructor"); + GC_ADD_FLAGS(obj, IS_OBJ_DESTRUCTOR_CALLED); + GC_ADDREF(obj); + obj->handlers->dtor_obj(obj); + GC_DELREF(obj); } } - current++; idx++; } - pefree(refcounts, 1); - if (GC_G(gc_protected)) { /* something went wrong */ return 0;