Skip to content

Commit 60a7e60

Browse files
nikicnikic
committed
Fixed bug #72530
For objects with destructors, we will now only call the destructor in the initial GC run, and remove any nested data. The object is marked purple so it will be considered a root for the next GC run, at which point it will be fully destroyed, if possible. GC counts change on a number of tests, as the objects now get destroyed later.
1 parent fdfc7ea commit 60a7e60

File tree

11 files changed

+100
-46
lines changed

11 files changed

+100
-46
lines changed

‎NEWS

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ PHP NEWS
77
(Nikita)
88
. Fixed bug #78406 (Broken file includes with user-defined stream filters).
99
(Nikita)
10+
. Fixed bug #72530 (Use After Free in GC with Certain Destructors). (Nikita)
1011

1112
- Date:
1213
. Fixed bug #78383 (Casting a DateTime to array no longer returns its

‎Zend/tests/bug71818.phpt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ class MemoryLeak
1919
private $things = [];
2020
}
2121

22-
ini_set('memory_limit', '10M');
22+
ini_set('memory_limit', '20M');
2323

2424
for ($i = 0; $i < 100000; ++$i) {
2525
$obj = new MemoryLeak();

‎Zend/tests/bug72530.phpt

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
--TEST--
2+
Bug #72530: Use After Free in GC with Certain Destructors
3+
--FILE--
4+
<?php
5+
6+
class ryat {
7+
var $ryat;
8+
var $chtg;
9+
10+
function __destruct() {
11+
$this->chtg = $this->ryat;
12+
$this->ryat = 1;
13+
}
14+
}
15+
16+
$o = new ryat;
17+
$o->ryat = $o;
18+
$x =& $o->chtg;
19+
20+
unset($o);
21+
gc_collect_cycles();
22+
var_dump($x);
23+
24+
?>
25+
--EXPECT--
26+
object(ryat)#1 (2) {
27+
["ryat"]=>
28+
int(1)
29+
["chtg"]=>
30+
*RECURSION*
31+
}

‎Zend/tests/gc_011.phpt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ $a->a = $a;
1515
var_dump($a);
1616
unset($a);
1717
var_dump(gc_collect_cycles());
18+
var_dump(gc_collect_cycles());
1819
echo "ok\n"
1920
?>
2021
--EXPECTF--
@@ -23,5 +24,6 @@ object(Foo)#%d (1) {
2324
*RECURSION*
2425
}
2526
__destruct
27+
int(0)
2628
int(1)
2729
ok

‎Zend/tests/gc_016.phpt

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,6 @@ echo "ok\n"
2323
?>
2424
--EXPECT--
2525
-> int(0)
26-
int(1)
27-
int(1)
26+
int(0)
27+
int(2)
2828
ok

‎Zend/tests/gc_017.phpt

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,11 +32,13 @@ unset($a);
3232
unset($b);
3333
unset($c);
3434
var_dump(gc_collect_cycles());
35+
var_dump(gc_collect_cycles());
3536
echo "ok\n"
3637
?>
3738
--EXPECTF--
3839
string(1) "%s"
3940
string(1) "%s"
4041
string(1) "%s"
41-
int(4)
42+
int(0)
43+
int(1)
4244
ok

‎Zend/tests/gc_028.phpt

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,8 @@ $bar->foo = $foo;
2828
unset($foo);
2929
unset($bar);
3030
var_dump(gc_collect_cycles());
31+
var_dump(gc_collect_cycles());
3132
?>
3233
--EXPECT--
33-
int(2)
34+
int(0)
35+
int(1)

‎Zend/tests/gc_029.phpt

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,8 @@ $bar->foo = $foo;
3030
unset($foo);
3131
unset($bar);
3232
var_dump(gc_collect_cycles());
33+
var_dump(gc_collect_cycles());
3334
?>
34-
--EXPECTREGEX--
35-
int\([23]\)
35+
--EXPECT--
36+
int(0)
37+
int(1)

‎Zend/tests/gc_035.phpt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,5 +22,5 @@ var_dump(gc_collect_cycles());
2222
var_dump(gc_collect_cycles());
2323
--EXPECT--
2424
int(0)
25-
int(2)
2625
int(0)
26+
int(2)

‎Zend/tests/generators/bug76427.phpt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,4 +21,4 @@ var_dump(gc_collect_cycles());
2121

2222
?>
2323
--EXPECT--
24-
int(4)
24+
int(2)

‎Zend/zend_gc.c

Lines changed: 51 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,7 @@
141141
#define GC_ROOT 0x0 /* possible root of circular garbage */
142142
#define GC_UNUSED 0x1 /* part of linked list of unused buffers */
143143
#define GC_GARBAGE 0x2 /* garbage to delete */
144+
#define GC_DTOR_GARBAGE 0x3 /* garbage on which only the dtor should be invoked */
144145

145146
#define GC_GET_PTR(ptr) \
146147
((void*)(((uintptr_t)(ptr)) & ~GC_BITS))
@@ -151,9 +152,13 @@
151152
((((uintptr_t)(ptr)) & GC_BITS) == GC_UNUSED)
152153
#define GC_IS_GARBAGE(ptr) \
153154
((((uintptr_t)(ptr)) & GC_BITS) == GC_GARBAGE)
155+
#define GC_IS_DTOR_GARBAGE(ptr) \
156+
((((uintptr_t)(ptr)) & GC_BITS) == GC_DTOR_GARBAGE)
154157

155158
#define GC_MAKE_GARBAGE(ptr) \
156159
((void*)(((uintptr_t)(ptr)) | GC_GARBAGE))
160+
#define GC_MAKE_DTOR_GARBAGE(ptr) \
161+
((void*)(((uintptr_t)(ptr)) | GC_DTOR_GARBAGE))
157162

158163
/* GC address conversion */
159164
#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
13281333
tail_call:
13291334
do {
13301335
if (root) {
1331-
GC_TRACE_REF(ref, "removing from buffer");
1332-
gc_remove_from_roots(root);
1333-
GC_REF_SET_INFO(ref, 0);
13341336
root = NULL;
13351337
count++;
13361338
} else if (GC_REF_ADDRESS(ref) != 0
@@ -1461,67 +1463,79 @@ ZEND_API int zend_gc_collect_cycles(void)
14611463
end = GC_G(first_unused);
14621464

14631465
if (gc_flags & GC_HAS_DESTRUCTORS) {
1464-
uint32_t *refcounts;
1465-
14661466
GC_TRACE("Calling destructors");
14671467

1468-
// TODO: may be use emalloc() ???
1469-
refcounts = pemalloc(sizeof(uint32_t) * end, 1);
1470-
1471-
/* Remember reference counters before calling destructors */
1468+
/* During a destructor call, new externally visible references to nested data may
1469+
* be introduced. These references can be introduced in a way that does not
1470+
* modify any refcounts, so we have no real way to detect this situation
1471+
* short of rerunning full GC tracing. What we do instead is to only run
1472+
* destructors at this point, and leave the actual freeing of the objects
1473+
* until the next GC run. */
1474+
1475+
/* Mark all roots for which a dtor will be invoked as DTOR_GARBAGE. Additionally
1476+
* color them purple. This serves a double purpose: First, they should be
1477+
* considered new potential roots for the next GC run. Second, it will prevent
1478+
* their removal from the root buffer by nested data removal. */
14721479
idx = GC_FIRST_ROOT;
14731480
current = GC_IDX2PTR(GC_FIRST_ROOT);
14741481
while (idx != end) {
14751482
if (GC_IS_GARBAGE(current->ref)) {
14761483
p = GC_GET_PTR(current->ref);
1477-
refcounts[idx] = GC_REFCOUNT(p);
1484+
if (GC_TYPE(p) == IS_OBJECT && !(OBJ_FLAGS(p) & IS_OBJ_DESTRUCTOR_CALLED)) {
1485+
zend_object *obj = (zend_object *) p;
1486+
if (obj->handlers->dtor_obj != zend_objects_destroy_object
1487+
|| obj->ce->destructor) {
1488+
current->ref = GC_MAKE_DTOR_GARBAGE(obj);
1489+
GC_REF_SET_COLOR(obj, GC_PURPLE);
1490+
} else {
1491+
GC_ADD_FLAGS(obj, IS_OBJ_DESTRUCTOR_CALLED);
1492+
}
1493+
}
14781494
}
14791495
current++;
14801496
idx++;
14811497
}
14821498

1483-
/* Call destructors
1484-
*
1485-
* The root buffer might be reallocated during destructors calls,
1486-
* make sure to reload pointers as necessary. */
1499+
/* Remove nested data for objects on which a destructor will be called.
1500+
* This will not remove the objects themselves, as they have been colored
1501+
* purple. */
14871502
idx = GC_FIRST_ROOT;
1503+
current = GC_IDX2PTR(GC_FIRST_ROOT);
14881504
while (idx != end) {
1489-
current = GC_IDX2PTR(idx);
1490-
if (GC_IS_GARBAGE(current->ref)) {
1505+
if (GC_IS_DTOR_GARBAGE(current->ref)) {
14911506
p = GC_GET_PTR(current->ref);
1492-
if (GC_TYPE(p) == IS_OBJECT
1493-
&& !(OBJ_FLAGS(p) & IS_OBJ_DESTRUCTOR_CALLED)) {
1494-
zend_object *obj = (zend_object*)p;
1495-
1496-
GC_TRACE_REF(obj, "calling destructor");
1497-
GC_ADD_FLAGS(obj, IS_OBJ_DESTRUCTOR_CALLED);
1498-
if (obj->handlers->dtor_obj != zend_objects_destroy_object
1499-
|| obj->ce->destructor) {
1500-
GC_ADDREF(obj);
1501-
obj->handlers->dtor_obj(obj);
1502-
GC_DELREF(obj);
1503-
}
1504-
}
1507+
count -= gc_remove_nested_data_from_buffer(p, current);
15051508
}
1509+
current++;
15061510
idx++;
15071511
}
15081512

1509-
/* Remove values captured in destructors */
1513+
/* Actually call destructors.
1514+
*
1515+
* The root buffer might be reallocated during destructors calls,
1516+
* make sure to reload pointers as necessary. */
15101517
idx = GC_FIRST_ROOT;
1511-
current = GC_IDX2PTR(GC_FIRST_ROOT);
15121518
while (idx != end) {
1513-
if (GC_IS_GARBAGE(current->ref)) {
1519+
current = GC_IDX2PTR(idx);
1520+
if (GC_IS_DTOR_GARBAGE(current->ref)) {
15141521
p = GC_GET_PTR(current->ref);
1515-
if (GC_REFCOUNT(p) > refcounts[idx]) {
1516-
count -= gc_remove_nested_data_from_buffer(p, current);
1522+
/* Mark this is as a normal root for the next GC run,
1523+
* it's no longer garbage for this run. */
1524+
current->ref = p;
1525+
/* Double check that the destructor hasn't been called yet. It could have
1526+
* already been invoked indirectly by some other destructor. */
1527+
if (!(OBJ_FLAGS(p) & IS_OBJ_DESTRUCTOR_CALLED)) {
1528+
zend_object *obj = (zend_object*)p;
1529+
GC_TRACE_REF(obj, "calling destructor");
1530+
GC_ADD_FLAGS(obj, IS_OBJ_DESTRUCTOR_CALLED);
1531+
GC_ADDREF(obj);
1532+
obj->handlers->dtor_obj(obj);
1533+
GC_DELREF(obj);
15171534
}
15181535
}
1519-
current++;
15201536
idx++;
15211537
}
15221538

1523-
pefree(refcounts, 1);
1524-
15251539
if (GC_G(gc_protected)) {
15261540
/* something went wrong */
15271541
return 0;

0 commit comments

Comments
 (0)