Skip to content

Commit 6fa1af1

Browse files
EdmondDantesLamentXU123
authored andcommitted
Fix leak when the added previous exception is already in the chain (#24177)
zend_exception_set_previous() takes ownership of add_previous. When walking the exception's previous chain reaches add_previous itself, the function returns without releasing that reference. Release it after the loop, as on the other paths that discard the added previous exception. This can happen when an exception thrown in finally already contains the pending exception in its previous chain. Add regression tests covering direct and nested chains, destructors during unwinding, generator and fiber destruction, and GC destructors running in a fiber. Closes #24177
1 parent 303e6aa commit 6fa1af1

7 files changed

Lines changed: 172 additions & 0 deletions

‎NEWS‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,10 @@ PHP NEWS
22
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
33
?? ??? ????, PHP 8.4.28
44

5+
- Core:
6+
. Fixed memory leak when an exception already contains the pending exception
7+
in its previous chain. (Edmond)
8+
59
- DOM:
610
. Fixed bug GH-23352 (UAF reading an attribute value node retained across
711
DOMDocument::adoptNode()). (David Carlier)
Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
--TEST--
2+
Exception thrown in a destructor during unwinding that already has the pending exception in its previous chain
3+
--FILE--
4+
<?php
5+
class ThrowsInDestructor {
6+
public function __construct(private Throwable $pending) {}
7+
8+
public function __destruct() {
9+
throw new LogicException("thrown", 0, $this->pending);
10+
}
11+
}
12+
13+
function f() {
14+
$pending = new RuntimeException("pending");
15+
$object = new ThrowsInDestructor($pending);
16+
throw $pending;
17+
}
18+
19+
try {
20+
f();
21+
} catch (Throwable $t) {
22+
for (; $t !== null; $t = $t->getPrevious()) {
23+
echo $t->getMessage(), "\n";
24+
}
25+
}
26+
?>
27+
--EXPECT--
28+
thrown
29+
pending
Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
--TEST--
2+
Exception thrown by a GC destructor that already has the exception of a previous, suspended destructor in its previous chain
3+
--FILE--
4+
<?php
5+
class Cycle {
6+
public static ?Throwable $pending = null;
7+
public $self;
8+
9+
public function __construct() {
10+
$this->self = $this;
11+
}
12+
13+
public function __destruct() {
14+
if (self::$pending === null) {
15+
try {
16+
Fiber::suspend();
17+
} finally {
18+
self::$pending = new RuntimeException("pending");
19+
throw self::$pending;
20+
}
21+
}
22+
throw new LogicException("thrown", 0, self::$pending);
23+
}
24+
}
25+
26+
$fiber = new Fiber(function () {
27+
new Cycle;
28+
new Cycle;
29+
try {
30+
gc_collect_cycles();
31+
} catch (Throwable $t) {
32+
for (; $t !== null; $t = $t->getPrevious()) {
33+
echo $t->getMessage(), "\n";
34+
}
35+
}
36+
});
37+
$fiber->start();
38+
?>
39+
--EXPECT--
40+
thrown
41+
pending
Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
--TEST--
2+
Exception thrown in finally of a fiber destroyed during unwinding that already has the pending exception in its previous chain
3+
--FILE--
4+
<?php
5+
function f() {
6+
$pending = new RuntimeException("pending");
7+
$fiber = new Fiber(function () use ($pending) {
8+
try {
9+
Fiber::suspend();
10+
} finally {
11+
throw new LogicException("thrown", 0, $pending);
12+
}
13+
});
14+
$fiber->start();
15+
throw $pending;
16+
}
17+
18+
try {
19+
f();
20+
} catch (Throwable $t) {
21+
for (; $t !== null; $t = $t->getPrevious()) {
22+
echo $t->getMessage(), "\n";
23+
}
24+
}
25+
?>
26+
--EXPECT--
27+
thrown
28+
pending
Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
--TEST--
2+
Exception thrown in finally of a generator destroyed during unwinding that already has the pending exception in its previous chain
3+
--FILE--
4+
<?php
5+
function gen(Throwable $pending) {
6+
try {
7+
yield;
8+
} finally {
9+
throw new LogicException("thrown", 0, $pending);
10+
}
11+
}
12+
13+
function f() {
14+
$pending = new RuntimeException("pending");
15+
$generator = gen($pending);
16+
$generator->current();
17+
throw $pending;
18+
}
19+
20+
try {
21+
f();
22+
} catch (Throwable $t) {
23+
for (; $t !== null; $t = $t->getPrevious()) {
24+
echo $t->getMessage(), "\n";
25+
}
26+
}
27+
?>
28+
--EXPECT--
29+
thrown
30+
pending
Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
--TEST--
2+
Exception thrown in finally that already has the pending exception in its previous chain
3+
--FILE--
4+
<?php
5+
function direct() {
6+
$e = new RuntimeException("pending");
7+
try {
8+
throw $e;
9+
} finally {
10+
throw new LogicException("thrown", 0, $e);
11+
}
12+
}
13+
14+
function nested() {
15+
$e = new RuntimeException("pending");
16+
try {
17+
throw $e;
18+
} finally {
19+
throw new LogicException("thrown", 0, new Exception("middle", 0, $e));
20+
}
21+
}
22+
23+
foreach (['direct', 'nested'] as $function) {
24+
try {
25+
$function();
26+
} catch (Throwable $t) {
27+
for (; $t !== null; $t = $t->getPrevious()) {
28+
echo $t->getMessage(), "\n";
29+
}
30+
}
31+
}
32+
?>
33+
--EXPECT--
34+
thrown
35+
pending
36+
thrown
37+
middle
38+
pending

‎Zend/zend_exceptions.c‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,8 @@ void zend_exception_set_previous(zend_object *exception, zend_object *add_previo
135135
}
136136
ex = previous;
137137
} while (Z_OBJ_P(ex) != add_previous);
138+
139+
OBJ_RELEASE(add_previous);
138140
}
139141
/* }}} */
140142

0 commit comments

Comments
 (0)