Skip to content

Commit b527e1b

Browse files
LamentXU123LamentXU123
committed
Merge branch 'PHP-8.6'
* PHP-8.6: ext/zip: Fix use-after-free in the archive destructor path (#23779)
2 parents 6fc4017 + e0a1098 commit b527e1b

3 files changed

Lines changed: 125 additions & 2 deletions

File tree

‎ext/zip/php_zip.c‎

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -659,7 +659,6 @@ static bool php_zipobj_close(ze_zip_object *obj, zend_string **out_str) /* {{{ *
659659
if (intern) {
660660
archive->close = true;
661661
int err = zip_close(intern);
662-
archive->close = false;
663662
if (err) {
664663
php_error_docref(NULL, E_WARNING, "%s", zip_strerror(intern));
665664
/* Save error for property reader */
@@ -698,6 +697,7 @@ static bool php_zipobj_close(ze_zip_object *obj, zend_string **out_str) /* {{{ *
698697

699698
if (archive) {
700699
archive->za = NULL;
700+
archive->close = false;
701701
bailout = archive->bailout_callback;
702702
archive->bailout_callback = false;
703703
obj->archive = NULL;
@@ -1128,6 +1128,10 @@ static void php_zip_progress_callback_free(void *ptr)
11281128
{
11291129
php_zip_archive *archive = ptr;
11301130

1131+
if (UNEXPECTED(!EG(active) || archive->bailout_callback)) {
1132+
return;
1133+
}
1134+
11311135
if (ZEND_FCC_INITIALIZED(archive->progress_callback)) {
11321136
zend_fcc_dtor(&archive->progress_callback);
11331137
}
@@ -1139,6 +1143,10 @@ static void php_zip_cancel_callback_free(void *ptr)
11391143
{
11401144
php_zip_archive *archive = ptr;
11411145

1146+
if (UNEXPECTED(!EG(active) || archive->bailout_callback)) {
1147+
return;
1148+
}
1149+
11421150
if (ZEND_FCC_INITIALIZED(archive->cancel_callback)) {
11431151
zend_fcc_dtor(&archive->cancel_callback);
11441152
}
@@ -1169,7 +1177,12 @@ bool php_zip_archive_release(php_zip_archive *archive)
11691177
}
11701178

11711179
if (archive->za) {
1172-
if (zip_close(archive->za) != 0) {
1180+
/* Guard against a re-entrant close() or open() from a progress/cancel
1181+
* callback fired during zip_close(), which would run a nested zip_close()
1182+
* on the same archive (see php_zipobj_close()). */
1183+
archive->close = true;
1184+
int err = zip_close(archive->za);
1185+
if (err != 0) {
11731186
if (!archive->bailout_callback) {
11741187
php_error_docref(NULL, E_WARNING, "Cannot destroy the zip context: %s", zip_strerror(archive->za));
11751188
}
@@ -3131,6 +3144,10 @@ static void php_zip_get_stream(INTERNAL_FUNCTION_PARAMETERS, int type, bool acce
31313144

31323145
ZIP_FROM_OBJECT(intern, self);
31333146

3147+
if (php_zipobj_closing(Z_ZIP_P(self))) {
3148+
RETURN_THROWS();
3149+
}
3150+
31343151
if (type) {
31353152
PHP_ZIP_STAT_PATH(intern, ZSTR_VAL(filename), ZSTR_LEN(filename), flags, sb);
31363153
} else {

‎ext/zip/tests/gh23747_close_error.phpt‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
--TEST--
2+
GH-23747 (Re-entrant close from a ZipArchive close warning is rejected)
3+
--EXTENSIONS--
4+
zip
5+
--FILE--
6+
<?php
7+
$filename = __DIR__ . '/gh23747_close_error.zip';
8+
$source = __DIR__ . '/gh23747_close_error.txt';
9+
10+
foreach (['close', 'destruct'] as $operation) {
11+
echo $operation, ":\n";
12+
file_put_contents($source, 'contents');
13+
$zip = new ZipArchive();
14+
$zip->open($filename, ZipArchive::CREATE | ZipArchive::OVERWRITE);
15+
$zip->addFile($source, 'file.txt');
16+
unlink($source);
17+
18+
$weak = WeakReference::create($zip);
19+
set_error_handler(static function (int $errno, string $message) use ($weak): bool {
20+
try {
21+
$weak->get()->close();
22+
} catch (Error $error) {
23+
echo $error::class, ': ', $error->getMessage(), "\n";
24+
}
25+
return true;
26+
});
27+
28+
if ($operation === 'close') {
29+
var_dump($zip->close());
30+
} else {
31+
unset($zip);
32+
echo "destroyed\n";
33+
}
34+
restore_error_handler();
35+
}
36+
?>
37+
--CLEAN--
38+
<?php
39+
@unlink(__DIR__ . '/gh23747_close_error.zip');
40+
@unlink(__DIR__ . '/gh23747_close_error.txt');
41+
?>
42+
--EXPECT--
43+
close:
44+
Error: Already being closed
45+
bool(false)
46+
destruct:
47+
Error: Already being closed
48+
destroyed

‎ext/zip/tests/gh23747_dtor.phpt‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
--TEST--
2+
GH-23747 (Re-entrant operations during ZipArchive destruction are rejected)
3+
--EXTENSIONS--
4+
zip
5+
--SKIPIF--
6+
<?php
7+
if (!method_exists(ZipArchive::class, 'registerProgressCallback')) {
8+
die('skip progress callbacks are not supported');
9+
}
10+
?>
11+
--FILE--
12+
<?php
13+
$filename = __DIR__ . '/gh23747_dtor.zip';
14+
15+
foreach ([
16+
['close', []],
17+
['getStream', ['f0.txt']],
18+
['getStreamName', ['f0.txt']],
19+
['getStreamIndex', [0]],
20+
] as [$method, $arguments]) {
21+
$zip = new ZipArchive();
22+
$zip->open($filename, ZipArchive::CREATE | ZipArchive::OVERWRITE);
23+
for ($index = 0; $index < 64; $index++) {
24+
$zip->addFromString("f$index.txt", str_repeat('x', 2000));
25+
}
26+
27+
$weak = WeakReference::create($zip);
28+
$callback = static function (float $rate) use ($weak, $method, $arguments): void {
29+
static $done = false;
30+
if ($done) {
31+
return;
32+
}
33+
$done = true;
34+
35+
try {
36+
$weak->get()->$method(...$arguments);
37+
} catch (Error $error) {
38+
echo $method, ': ', $error->getMessage(), "\n";
39+
}
40+
};
41+
$zip->registerProgressCallback(0.0, $callback);
42+
unset($zip);
43+
echo "destroyed\n";
44+
}
45+
?>
46+
--CLEAN--
47+
<?php
48+
@unlink(__DIR__ . '/gh23747_dtor.zip');
49+
?>
50+
--EXPECT--
51+
close: Already being closed
52+
destroyed
53+
getStream: Already being closed
54+
destroyed
55+
getStreamName: Already being closed
56+
destroyed
57+
getStreamIndex: Already being closed
58+
destroyed

0 commit comments

Comments
 (0)