Skip to content

Commit 1c307c4

Browse files
LamentXU123LamentXU123
committed
feedback
1 parent e4bbd5b commit 1c307c4

3 files changed

Lines changed: 93 additions & 37 deletions

File tree

‎ext/zip/php_zip.c‎

Lines changed: 30 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1081,23 +1081,23 @@ static HashTable *php_zip_get_properties(zend_object *object)/* {{{ */
10811081
#ifdef HAVE_PROGRESS_CALLBACK
10821082
static void _php_zip_progress_callback_free(void *ptr)
10831083
{
1084-
ze_zip_object *obj = ptr;
1084+
php_zip_archive *archive = ptr;
10851085

1086-
if (!Z_ISUNDEF(obj->progress_callback)) {
1087-
zval_ptr_dtor(&obj->progress_callback);
1088-
ZVAL_UNDEF(&obj->progress_callback);
1086+
if (!Z_ISUNDEF(archive->progress_callback)) {
1087+
zval_ptr_dtor(&archive->progress_callback);
1088+
ZVAL_UNDEF(&archive->progress_callback);
10891089
}
10901090
}
10911091
#endif
10921092

10931093
#ifdef HAVE_CANCEL_CALLBACK
10941094
static void _php_zip_cancel_callback_free(void *ptr)
10951095
{
1096-
ze_zip_object *obj = ptr;
1096+
php_zip_archive *archive = ptr;
10971097

1098-
if (!Z_ISUNDEF(obj->cancel_callback)) {
1099-
zval_ptr_dtor(&obj->cancel_callback);
1100-
ZVAL_UNDEF(&obj->cancel_callback);
1098+
if (!Z_ISUNDEF(archive->cancel_callback)) {
1099+
zval_ptr_dtor(&archive->cancel_callback);
1100+
ZVAL_UNDEF(&archive->cancel_callback);
11011101
}
11021102
}
11031103
#endif
@@ -1131,6 +1131,16 @@ void php_zip_archive_release(php_zip_archive *archive)
11311131
}
11321132
}
11331133

1134+
#ifdef HAVE_PROGRESS_CALLBACK
1135+
/* In case libzip did not invoke the callback state destructor. */
1136+
_php_zip_progress_callback_free(archive);
1137+
#endif
1138+
1139+
#ifdef HAVE_CANCEL_CALLBACK
1140+
/* In case libzip did not invoke the callback state destructor. */
1141+
_php_zip_cancel_callback_free(archive);
1142+
#endif
1143+
11341144
if (archive->buffers) {
11351145
for (int i = 0; i < archive->buffers_cnt; i++) {
11361146
efree(archive->buffers[i]);
@@ -1156,17 +1166,6 @@ static void php_zip_object_free_storage(zend_object *object) /* {{{ */
11561166
php_zip_archive_release(intern->archive);
11571167
intern->archive = NULL;
11581168
}
1159-
1160-
#ifdef HAVE_PROGRESS_CALLBACK
1161-
/* if not properly called by libzip */
1162-
_php_zip_progress_callback_free(intern);
1163-
#endif
1164-
1165-
#ifdef HAVE_CANCEL_CALLBACK
1166-
/* if not properly called by libzip */
1167-
_php_zip_cancel_callback_free(intern);
1168-
#endif
1169-
11701169
zend_object_std_dtor(&intern->zo);
11711170

11721171
if (intern->filename) {
@@ -3168,10 +3167,10 @@ static void _php_zip_progress_callback(zip_t *arch, double state, void *ptr)
31683167
{
31693168
zval cb_args[1];
31703169
zval cb_retval;
3171-
ze_zip_object *obj = ptr;
3170+
php_zip_archive *archive = ptr;
31723171

31733172
ZVAL_DOUBLE(&cb_args[0], state);
3174-
if (call_user_function(EG(function_table), NULL, &obj->progress_callback, &cb_retval, 1, cb_args) == SUCCESS && !Z_ISUNDEF(cb_retval)) {
3173+
if (call_user_function(EG(function_table), NULL, &archive->progress_callback, &cb_retval, 1, cb_args) == SUCCESS && !Z_ISUNDEF(cb_retval)) {
31753174
zval_ptr_dtor(&cb_retval);
31763175
}
31773176
}
@@ -3184,21 +3183,21 @@ PHP_METHOD(ZipArchive, registerProgressCallback)
31843183
double rate;
31853184
zend_fcall_info fci;
31863185
zend_fcall_info_cache fcc;
3187-
ze_zip_object *obj;
3186+
php_zip_archive *archive;
31883187

31893188
if (zend_parse_parameters(ZEND_NUM_ARGS(), "df", &rate, &fci, &fcc) == FAILURE) {
31903189
RETURN_THROWS();
31913190
}
31923191

31933192
ZIP_FROM_OBJECT(intern, self);
31943193

3195-
obj = Z_ZIP_P(self);
3194+
archive = Z_ZIP_P(self)->archive;
31963195

31973196
/* register */
3198-
if (zip_register_progress_callback_with_state(intern, rate, _php_zip_progress_callback, _php_zip_progress_callback_free, obj)) {
3197+
if (zip_register_progress_callback_with_state(intern, rate, _php_zip_progress_callback, _php_zip_progress_callback_free, archive)) {
31993198
RETURN_FALSE;
32003199
}
3201-
ZVAL_COPY(&obj->progress_callback, &fci.function_name);
3200+
ZVAL_COPY(&archive->progress_callback, &fci.function_name);
32023201

32033202
RETURN_TRUE;
32043203
}
@@ -3210,9 +3209,9 @@ static int _php_zip_cancel_callback(zip_t *arch, void *ptr)
32103209
{
32113210
zval cb_retval;
32123211
int retval = 0;
3213-
ze_zip_object *obj = ptr;
3212+
php_zip_archive *archive = ptr;
32143213

3215-
if (call_user_function(EG(function_table), NULL, &obj->cancel_callback, &cb_retval, 0, NULL) == SUCCESS && !Z_ISUNDEF(cb_retval)) {
3214+
if (call_user_function(EG(function_table), NULL, &archive->cancel_callback, &cb_retval, 0, NULL) == SUCCESS && !Z_ISUNDEF(cb_retval)) {
32163215
retval = zval_get_long(&cb_retval);
32173216
zval_ptr_dtor(&cb_retval);
32183217
}
@@ -3227,20 +3226,20 @@ PHP_METHOD(ZipArchive, registerCancelCallback)
32273226
zval *self = ZEND_THIS;
32283227
zend_fcall_info fci;
32293228
zend_fcall_info_cache fcc;
3230-
ze_zip_object *obj;
3229+
php_zip_archive *archive;
32313230
if (zend_parse_parameters(ZEND_NUM_ARGS(), "f", &fci, &fcc) == FAILURE) {
32323231
RETURN_THROWS();
32333232
}
32343233

32353234
ZIP_FROM_OBJECT(intern, self);
32363235

3237-
obj = Z_ZIP_P(self);
3236+
archive = Z_ZIP_P(self)->archive;
32383237

32393238
/* register */
3240-
if (zip_register_cancel_callback_with_state(intern, _php_zip_cancel_callback, _php_zip_cancel_callback_free, obj)) {
3239+
if (zip_register_cancel_callback_with_state(intern, _php_zip_cancel_callback, _php_zip_cancel_callback_free, archive)) {
32413240
RETURN_FALSE;
32423241
}
3243-
ZVAL_COPY(&obj->cancel_callback, &fci.function_name);
3242+
ZVAL_COPY(&archive->cancel_callback, &fci.function_name);
32443243

32453244
RETURN_TRUE;
32463245
}

‎ext/zip/php_zip.h‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,12 @@ typedef struct _php_zip_archive {
7373
/* libzip reads buffers until the archive is closed, can outlive the object. */
7474
char **buffers;
7575
int buffers_cnt;
76+
#ifdef HAVE_PROGRESS_CALLBACK
77+
zval progress_callback;
78+
#endif
79+
#ifdef HAVE_CANCEL_CALLBACK
80+
zval cancel_callback;
81+
#endif
7682
} php_zip_archive;
7783

7884
/* Extends zend object */
@@ -86,12 +92,6 @@ typedef struct _ze_zip_object {
8692
zip_int64_t last_id;
8793
int err_zip;
8894
int err_sys;
89-
#ifdef HAVE_PROGRESS_CALLBACK
90-
zval progress_callback;
91-
#endif
92-
#ifdef HAVE_CANCEL_CALLBACK
93-
zval cancel_callback;
94-
#endif
9595
zend_object zo;
9696
} ze_zip_object;
9797

‎ext/zip/tests/gh23276_callbacks.phpt‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
--TEST--
2+
GH-23276 (ZipArchive callbacks outlive the object while a stream holds the archive)
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+
if (!method_exists(ZipArchive::class, 'registerCancelCallback')) {
11+
die('skip cancel callbacks are not supported');
12+
}
13+
?>
14+
--FILE--
15+
<?php
16+
$filename = __DIR__ . '/gh23276_callbacks.zip';
17+
18+
$zip = new ZipArchive;
19+
$zip->open($filename, ZipArchive::CREATE | ZipArchive::OVERWRITE);
20+
$zip->addFromString('entry.txt', 'contents');
21+
$zip->close();
22+
23+
function testCallback(string $filename, string $type): void {
24+
$zip = new ZipArchive;
25+
$zip->open($filename);
26+
if ($type === 'progress') {
27+
var_dump($zip->registerProgressCallback(0.5, static function (float $rate): void {}));
28+
} else {
29+
var_dump($zip->registerCancelCallback(static function (): int { return 0; }));
30+
}
31+
$zip->addFromString("$type.txt", 'late');
32+
$stream = $zip->getStream('entry.txt');
33+
$weakRef = WeakReference::create($zip);
34+
unset($zip);
35+
36+
var_dump($weakRef->get());
37+
var_dump(stream_get_contents($stream));
38+
fclose($stream);
39+
echo "$type done\n";
40+
}
41+
42+
testCallback($filename, 'progress');
43+
testCallback($filename, 'cancel');
44+
?>
45+
--CLEAN--
46+
<?php
47+
@unlink(__DIR__ . '/gh23276_callbacks.zip');
48+
?>
49+
--EXPECT--
50+
bool(true)
51+
NULL
52+
string(8) "contents"
53+
progress done
54+
bool(true)
55+
NULL
56+
string(8) "contents"
57+
cancel done

0 commit comments

Comments
 (0)