diff --git a/NEWS b/NEWS index 022d48973602..497293d89e33 100644 --- a/NEWS +++ b/NEWS @@ -75,6 +75,10 @@ PHP NEWS - Readline: . Fixed class constant completion in the interactive shell. (Weilin Du) +- Zip: + . Fixed bug GH-23276 (ZipArchive subclass storing its own stream cannot be + garbage collected). (Weilin Du, ndossche) + - SAPI: . Fixed fuzzer targets failing to build in isolation. (Mrmaxmeier) . Fixed returns uninitialized value on LiteSpeed lsapi SAPI (Go Kudo) diff --git a/ext/zip/php_zip.c b/ext/zip/php_zip.c index f7a294425e6d..a7a3e340ecf1 100644 --- a/ext/zip/php_zip.c +++ b/ext/zip/php_zip.c @@ -339,6 +339,7 @@ static zend_result php_zip_add_file(ze_zip_object *obj, const char *filename, si zip_flags_t flags ) /* {{{ */ { + struct zip *za = php_zip_object_za(obj); struct zip_source *zs; char resolved_path[MAXPATHLEN]; php_stream_statbuf ssb; @@ -364,33 +365,33 @@ static zend_result php_zip_add_file(ze_zip_object *obj, const char *filename, si return FAILURE; } flags ^= ZIP_FL_OPEN_FILE_NOW; - zs = zip_source_filep(obj->za, fd, offset_start, offset_len); + zs = zip_source_filep(za, fd, offset_start, offset_len); if (!zs) { fclose(fd); return FAILURE; } } else { - zs = zip_source_file(obj->za, resolved_path, offset_start, offset_len); + zs = zip_source_file(za, resolved_path, offset_start, offset_len); if (!zs) { return FAILURE; } } /* Replace */ if (replace >= 0) { - if (zip_file_replace(obj->za, replace, zs, flags) < 0) { + if (zip_file_replace(za, replace, zs, flags) < 0) { zip_source_free(zs); return FAILURE; } - zip_error_clear(obj->za); + zip_error_clear(za); return SUCCESS; } /* Add */ - obj->last_id = zip_file_add(obj->za, entry_name, zs, flags); + obj->last_id = zip_file_add(za, entry_name, zs, flags); if (obj->last_id < 0) { zip_source_free(zs); return FAILURE; } - zip_error_clear(obj->za); + zip_error_clear(za); return SUCCESS; } /* }}} */ @@ -526,7 +527,7 @@ static zend_result php_zip_parse_options(HashTable *options, zip_options *opts) #define ZIP_FROM_OBJECT(intern, object) \ { \ ze_zip_object *obj = Z_ZIP_P(object); \ - intern = obj->za; \ + intern = php_zip_object_za(obj); \ if (!intern) { \ zend_value_error("Invalid or uninitialized Zip object"); \ RETURN_THROWS(); \ @@ -565,12 +566,13 @@ static zend_result php_zip_parse_options(HashTable *options, zip_options *opts) static zend_long php_zip_status(ze_zip_object *obj) /* {{{ */ { + struct zip *za = php_zip_object_za(obj); zend_long zep = (zend_long)obj->err_zip; /* saved err if closed */ - if (obj->za) { + if (za) { zip_error_t *err; - err = zip_get_error(obj->za); + err = zip_get_error(za); zep = (zend_long)zip_error_code_zip(err); zip_error_fini(err); } @@ -586,12 +588,13 @@ static zend_long php_zip_last_id(ze_zip_object *obj) /* {{{ */ static zend_long php_zip_status_sys(ze_zip_object *obj) /* {{{ */ { + struct zip *za = php_zip_object_za(obj); zend_long syp = (zend_long)obj->err_sys; /* saved err if closed */ - if (obj->za) { + if (za) { zip_error_t *err; - err = zip_get_error(obj->za); + err = zip_get_error(za); syp = (zend_long)zip_error_code_system(err); zip_error_fini(err); } @@ -601,8 +604,10 @@ static zend_long php_zip_status_sys(ze_zip_object *obj) /* {{{ */ static zend_long php_zip_get_num_files(ze_zip_object *obj) /* {{{ */ { - if (obj->za) { - zip_int64_t num = zip_get_num_entries(obj->za, 0); + struct zip *za = php_zip_object_za(obj); + + if (za) { + zip_int64_t num = zip_get_num_entries(za, 0); return MIN(num, ZEND_LONG_MAX); } return 0; @@ -621,8 +626,10 @@ static char * php_zipobj_get_filename(ze_zip_object *obj, int *len) /* {{{ */ static char * php_zipobj_get_zip_comment(ze_zip_object *obj, int *len) /* {{{ */ { - if (obj->za) { - return (char *)zip_get_archive_comment(obj->za, len, 0); + struct zip *za = php_zip_object_za(obj); + + if (za) { + return (char *)zip_get_archive_comment(za, len, 0); } return NULL; } @@ -635,7 +642,9 @@ static char * php_zipobj_get_zip_comment(ze_zip_object *obj, int *len) /* {{{ */ * If out_str is NULL, the final string contents, if any, will be discarded. */ static bool php_zipobj_close(ze_zip_object *obj, zend_string **out_str) /* {{{ */ { - struct zip *intern = obj->za; + php_zip_archive *archive = obj->archive; + struct zip *intern = archive ? archive->za : NULL; + bool bailout = false; bool success = false; if (intern) { @@ -665,22 +674,26 @@ static bool php_zipobj_close(ze_zip_object *obj, zend_string **out_str) /* {{{ * obj->filename_len = 0; } - if (obj->out_str) { + if (archive && archive->out_str) { if (out_str) { - *out_str = obj->out_str; + *out_str = archive->out_str; } else { - zend_string_release(obj->out_str); + zend_string_release(archive->out_str); } - obj->out_str = NULL; + archive->out_str = NULL; } else { ZEND_ASSERT(!out_str); } - obj->za = NULL; - obj->from_string = false; + if (archive) { + archive->za = NULL; + bailout = archive->bailout_callback; + archive->bailout_callback = false; + obj->archive = NULL; + bailout |= php_zip_archive_release(archive); + } - if (obj->bailout_callback) { - obj->bailout_callback = false; + if (bailout) { zend_bailout(); } @@ -1102,10 +1115,10 @@ static HashTable *php_zip_get_properties(zend_object *object)/* {{{ */ #ifdef HAVE_PROGRESS_CALLBACK static void php_zip_progress_callback_free(void *ptr) { - ze_zip_object *obj = ptr; + php_zip_archive *archive = ptr; - if (ZEND_FCC_INITIALIZED(obj->progress_callback)) { - zend_fcc_dtor(&obj->progress_callback); + if (ZEND_FCC_INITIALIZED(archive->progress_callback)) { + zend_fcc_dtor(&archive->progress_callback); } } #endif @@ -1113,30 +1126,76 @@ static void php_zip_progress_callback_free(void *ptr) #ifdef HAVE_CANCEL_CALLBACK static void php_zip_cancel_callback_free(void *ptr) { - ze_zip_object *obj = ptr; + php_zip_archive *archive = ptr; - if (ZEND_FCC_INITIALIZED(obj->cancel_callback)) { - zend_fcc_dtor(&obj->cancel_callback); + if (ZEND_FCC_INITIALIZED(archive->cancel_callback)) { + zend_fcc_dtor(&archive->cancel_callback); } } #endif +static php_zip_archive *php_zip_archive_create(struct zip *za) +{ + php_zip_archive *archive = ecalloc(1, sizeof(php_zip_archive)); + + archive->za = za; + archive->refcount = 1; + + return archive; +} + +void php_zip_archive_addref(php_zip_archive *archive) +{ + ZEND_ASSERT(archive->refcount > 0); + archive->refcount++; +} + +bool php_zip_archive_release(php_zip_archive *archive) +{ + ZEND_ASSERT(archive->refcount > 0); + if (--archive->refcount != 0) { + return false; + } + + if (archive->za) { + if (zip_close(archive->za) != 0) { + if (!archive->bailout_callback) { + php_error_docref(NULL, E_WARNING, "Cannot destroy the zip context: %s", zip_strerror(archive->za)); + } + zip_discard(archive->za); + } + archive->za = NULL; + } + +#ifdef HAVE_PROGRESS_CALLBACK + /* In case libzip did not invoke the callback state destructor. */ + php_zip_progress_callback_free(archive); +#endif + +#ifdef HAVE_CANCEL_CALLBACK + /* In case libzip did not invoke the callback state destructor. */ + php_zip_cancel_callback_free(archive); +#endif + + if (archive->out_str) { + zend_string_release(archive->out_str); + } + + bool bailout = archive->bailout_callback; + efree(archive); + return bailout; +} + static void php_zip_object_dtor(zend_object *object) { zend_objects_destroy_object(object); ze_zip_object *intern = php_zip_fetch_object(object); - if (intern->za) { - if (zip_close(intern->za) != 0) { - if (!intern->bailout_callback) { - php_error_docref(NULL, E_WARNING, "Cannot destroy the zip context: %s", zip_strerror(intern->za)); - } - zip_discard(intern->za); - } - intern->za = NULL; - if (intern->bailout_callback) { - intern->bailout_callback = false; + if (intern->archive) { + bool bailout = php_zip_archive_release(intern->archive); + intern->archive = NULL; + if (bailout) { zend_bailout(); } } @@ -1147,17 +1206,6 @@ static void php_zip_object_free_storage(zend_object *object) /* {{{ */ ze_zip_object * intern = php_zip_fetch_object(object); php_zipobj_close(intern, NULL); - -#ifdef HAVE_PROGRESS_CALLBACK - /* if not properly called by libzip */ - php_zip_progress_callback_free(intern); -#endif - -#ifdef HAVE_CANCEL_CALLBACK - /* if not properly called by libzip */ - php_zip_cancel_callback_free(intern); -#endif - zend_object_std_dtor(&intern->zo); } /* }}} */ @@ -1576,8 +1624,7 @@ PHP_METHOD(ZipArchive, open) } ze_obj->filename = resolved_path; ze_obj->filename_len = strlen(resolved_path); - ze_obj->za = intern; - ze_obj->from_string = false; + ze_obj->archive = php_zip_archive_create(intern); RETURN_TRUE; } /* }}} */ @@ -1598,18 +1645,21 @@ PHP_METHOD(ZipArchive, openString) } ze_zip_object *ze_obj = Z_ZIP_P(self); + php_zip_archive *archive; php_zipobj_close(ze_obj, NULL); zip_error_t err; zip_error_init(&err); - zip_source_t * zip_source = php_zip_create_string_source(buffer, &ze_obj->out_str, &err); + archive = php_zip_archive_create(NULL); + zip_source_t * zip_source = php_zip_create_string_source(buffer, &archive->out_str, &err); if (!zip_source) { ze_obj->err_zip = zip_error_code_zip(&err); ze_obj->err_sys = zip_error_code_system(&err); zip_error_fini(&err); + php_zip_archive_release(archive); RETURN_LONG(ze_obj->err_zip); } @@ -1619,11 +1669,13 @@ PHP_METHOD(ZipArchive, openString) ze_obj->err_sys = zip_error_code_system(&err); zip_error_fini(&err); zip_source_free(zip_source); + php_zip_archive_release(archive); RETURN_LONG(ze_obj->err_zip); } - ze_obj->from_string = true; - ze_obj->za = intern; + archive->za = intern; + archive->from_string = true; + ze_obj->archive = archive; zip_error_fini(&err); RETURN_TRUE; } @@ -1675,7 +1727,7 @@ PHP_METHOD(ZipArchive, closeString) ZIP_FROM_OBJECT(intern, self); - if (!Z_ZIP_P(self)->from_string) { + if (!Z_ZIP_P(self)->archive->from_string) { zend_throw_error(NULL, "ZipArchive::closeString can only be called on " "an archive opened with ZipArchive::openString"); RETURN_THROWS(); @@ -1737,12 +1789,14 @@ PHP_METHOD(ZipArchive, clearError) { zval *self = ZEND_THIS; ze_zip_object *ze_obj; + struct zip *za; ZEND_PARSE_PARAMETERS_NONE(); ze_obj = Z_ZIP_P(self); /* not ZIP_FROM_OBJECT as we can use saved error after close */ - if (ze_obj->za) { - zip_error_clear(ze_obj->za); + za = php_zip_object_za(ze_obj); + if (za) { + zip_error_clear(za); } else { ze_obj->err_zip = 0; ze_obj->err_sys = 0; @@ -1755,15 +1809,16 @@ PHP_METHOD(ZipArchive, getStatusString) { zval *self = ZEND_THIS; ze_zip_object *ze_obj; + struct zip *za; ZEND_PARSE_PARAMETERS_NONE(); ze_obj = Z_ZIP_P(self); /* not ZIP_FROM_OBJECT as we can use saved error after close */ - - if (ze_obj->za) { + za = php_zip_object_za(ze_obj); + if (za) { zip_error_t *err; - err = zip_get_error(ze_obj->za); + err = zip_get_error(za); RETVAL_STRING(zip_error_strerror(err)); zip_error_fini(err); } else { @@ -1858,6 +1913,7 @@ static void php_zip_add_from_pattern(INTERNAL_FUNCTION_PARAMETERS, int type) /* if (found > 0) { zval *zval_file; ze_zip_object *ze_obj = Z_ZIP_P(self); + struct zip *za = php_zip_object_za(ze_obj); for (int i = 0; i < found; i++) { zend_string *basename = NULL; @@ -1920,14 +1976,14 @@ static void php_zip_add_from_pattern(INTERNAL_FUNCTION_PARAMETERS, int type) /* RETURN_FALSE; } if (opts.comp_method >= 0) { - if (zip_set_file_compression(ze_obj->za, ze_obj->last_id, opts.comp_method, opts.comp_flags)) { + if (zip_set_file_compression(za, ze_obj->last_id, opts.comp_method, opts.comp_flags)) { zend_array_destroy(Z_ARR_P(return_value)); RETURN_FALSE; } } #ifdef HAVE_ENCRYPTION if (opts.enc_method >= 0) { - if (!php_zip_file_set_encryption(ze_obj->za, ze_obj->last_id, opts.enc_method, opts.enc_password)) { + if (!php_zip_file_set_encryption(za, ze_obj->last_id, opts.enc_method, opts.enc_password)) { zend_array_destroy(Z_ARR_P(return_value)); RETURN_FALSE; } @@ -3084,9 +3140,9 @@ PHP_METHOD(ZipArchive, getStream) #ifdef HAVE_PROGRESS_CALLBACK static void php_zip_progress_callback(zip_t *arch, double state, void *ptr) { - ze_zip_object *obj = ptr; + php_zip_archive *archive = ptr; - if (UNEXPECTED(!EG(active) || obj->bailout_callback)) { + if (UNEXPECTED(!EG(active) || archive->bailout_callback)) { return; } @@ -3095,9 +3151,9 @@ static void php_zip_progress_callback(zip_t *arch, double state, void *ptr) ZVAL_DOUBLE(&cb_args[0], state); zend_try { - zend_call_known_fcc(&obj->progress_callback, NULL, 1, cb_args, NULL); + zend_call_known_fcc(&archive->progress_callback, NULL, 1, cb_args, NULL); } zend_catch { - obj->bailout_callback = true; + archive->bailout_callback = true; } zend_end_try(); } @@ -3108,27 +3164,28 @@ PHP_METHOD(ZipArchive, registerProgressCallback) double rate; zend_fcall_info dummy_fci; zend_fcall_info_cache fcc; + php_zip_archive *archive; ze_zip_object *obj; if (zend_parse_parameters(ZEND_NUM_ARGS(), "dF", &rate, &dummy_fci, &fcc) == FAILURE) { RETURN_THROWS(); } - /* Inline ZIP_FROM_OBJECT(intern, self); */ obj = Z_ZIP_P(ZEND_THIS); - intern = obj->za; - if (!intern) { \ + intern = php_zip_object_za(obj); + if (!intern) { zend_value_error("Invalid or uninitialized Zip object"); zend_release_fcall_info_cache(&fcc); RETURN_THROWS(); } + archive = obj->archive; /* register */ - if (zip_register_progress_callback_with_state(intern, rate, php_zip_progress_callback, php_zip_progress_callback_free, obj)) { + if (zip_register_progress_callback_with_state(intern, rate, php_zip_progress_callback, php_zip_progress_callback_free, archive)) { zend_release_fcall_info_cache(&fcc); RETURN_FALSE; } - zend_fcc_dup(&obj->progress_callback, &fcc); + zend_fcc_dup(&archive->progress_callback, &fcc); RETURN_TRUE; } @@ -3139,16 +3196,16 @@ PHP_METHOD(ZipArchive, registerProgressCallback) static int php_zip_cancel_callback(zip_t *arch, void *ptr) { zval cb_retval; - ze_zip_object *obj = ptr; + php_zip_archive *archive = ptr; - if (UNEXPECTED(!EG(active) || obj->bailout_callback)) { + if (UNEXPECTED(!EG(active) || archive->bailout_callback)) { return 0; } zend_try { - zend_call_known_fcc(&obj->cancel_callback, &cb_retval, 0, NULL, NULL); + zend_call_known_fcc(&archive->cancel_callback, &cb_retval, 0, NULL, NULL); } zend_catch { - obj->bailout_callback = true; + archive->bailout_callback = true; /* Cancel if a bailout occurs to allow cleanup to happen */ return -1; } zend_end_try(); @@ -3176,6 +3233,7 @@ PHP_METHOD(ZipArchive, registerCancelCallback) struct zip *intern; zend_fcall_info dummy_fci; zend_fcall_info_cache fcc; + php_zip_archive *archive; ze_zip_object *obj; if (zend_parse_parameters(ZEND_NUM_ARGS(), "F", &dummy_fci, &fcc) == FAILURE) { RETURN_THROWS(); @@ -3183,19 +3241,20 @@ PHP_METHOD(ZipArchive, registerCancelCallback) /* Inline ZIP_FROM_OBJECT(intern, self); */ obj = Z_ZIP_P(ZEND_THIS); - intern = obj->za; - if (!intern) { \ + intern = php_zip_object_za(obj); + if (!intern) { zend_value_error("Invalid or uninitialized Zip object"); zend_release_fcall_info_cache(&fcc); RETURN_THROWS(); } + archive = obj->archive; /* register */ - if (zip_register_cancel_callback_with_state(intern, php_zip_cancel_callback, php_zip_cancel_callback_free, obj)) { + if (zip_register_cancel_callback_with_state(intern, php_zip_cancel_callback, php_zip_cancel_callback_free, archive)) { zend_release_fcall_info_cache(&fcc); RETURN_FALSE; } - zend_fcc_dup(&obj->cancel_callback, &fcc); + zend_fcc_dup(&archive->cancel_callback, &fcc); RETURN_TRUE; } diff --git a/ext/zip/php_zip.h b/ext/zip/php_zip.h index a10b1910f2ad..a67d2042d7ca 100644 --- a/ext/zip/php_zip.h +++ b/ext/zip/php_zip.h @@ -64,17 +64,13 @@ typedef struct _ze_zip_read_rsrc { zend_long zip_rsrc_handle; } zip_read_rsrc; -/* Extends zend object */ -typedef struct _ze_zip_object { +/* Refcounted holder for the native archive state. + * Owned by a ZipArchive object and streams opened from it. */ +typedef struct _php_zip_archive { struct zip *za; - HashTable *prop_handler; - char *filename; - size_t filename_len; + uint32_t refcount; zend_string *out_str; bool from_string; - zip_int64_t last_id; - int err_zip; - int err_sys; bool bailout_callback; #ifdef HAVE_PROGRESS_CALLBACK zend_fcall_info_cache progress_callback; @@ -82,16 +78,37 @@ typedef struct _ze_zip_object { #ifdef HAVE_CANCEL_CALLBACK zend_fcall_info_cache cancel_callback; #endif +} php_zip_archive; + +/* Extends zend object */ +typedef struct _ze_zip_object { + /* NULL when there is no open archive, non-NULL otherwise. + * Owns one ref to the struct. */ + php_zip_archive *archive; + HashTable *prop_handler; + char *filename; + size_t filename_len; + zip_int64_t last_id; + int err_zip; + int err_sys; zend_object zo; } ze_zip_object; #define php_zip_fetch_object(obj) ZEND_CONTAINER_OF(obj, ze_zip_object, zo) +/* The archive an object currently has open, or NULL. */ +static zend_always_inline struct zip *php_zip_object_za(const ze_zip_object *obj) { + return obj->archive ? obj->archive->za : NULL; +} + #define Z_ZIP_P(zv) php_zip_fetch_object(Z_OBJ_P((zv))) php_stream *php_stream_zip_opener(php_stream_wrapper *wrapper, const char *path, const char *mode, int options, zend_string **opened_path, php_stream_context *context STREAMS_DC); php_stream *php_stream_zip_open(ze_zip_object *obj, struct zip_stat *sb, const char *mode, zip_flags_t flags STREAMS_DC); +void php_zip_archive_addref(php_zip_archive *archive); +bool php_zip_archive_release(php_zip_archive *archive); + extern const php_stream_wrapper php_stream_zip_wrapper; zip_source_t * php_zip_create_string_source(zend_string *str, zend_string **dest, zip_error_t *err); diff --git a/ext/zip/tests/gh23276.phpt b/ext/zip/tests/gh23276.phpt new file mode 100644 index 000000000000..ac005cd86354 --- /dev/null +++ b/ext/zip/tests/gh23276.phpt @@ -0,0 +1,95 @@ +--TEST-- +GH-23276 (ZipArchive subclass storing its own stream is collectable) +--CREDITS-- +Eyüp Can Akman +--EXTENSIONS-- +zip +--FILE-- +getStream('entry.txt'); + if (!is_resource($stream)) { + throw new Exception('Failed to open entry stream'); + } + return $stream; +} + +$filename = __DIR__ . '/gh23276.zip'; + +$zip = new Holder; +$zip->open($filename, ZipArchive::CREATE | ZipArchive::OVERWRITE); +$zip->addFromString('entry.txt', 'contents'); +$zip->close(); + +// An archive holding its own stream in a property must be destroyed. +$zip->open($filename, ZipArchive::RDONLY); +$zip->stream = getEntryStream($zip); +$weakRef = WeakReference::create($zip); +unset($zip); +var_dump($weakRef->get()); + +// Same through an indirect edge (property -> array -> resource). +$zip = new Holder; +$zip->open($filename, ZipArchive::RDONLY); +$zip->bag[] = getEntryStream($zip); +$weakRef = WeakReference::create($zip); +unset($zip); +var_dump($weakRef->get()); + +// Two archives cross-holding each other's streams. +$a = new Holder; +$b = new Holder; +$a->open($filename, ZipArchive::RDONLY); +$b->open($filename, ZipArchive::RDONLY); +$a->stream = getEntryStream($b); +$b->stream = getEntryStream($a); +$weakRef = WeakReference::create($a); +unset($a, $b); +var_dump($weakRef->get()); + +// A resurrected object must retain a usable stream. +$zip = new ResurrectingHolder; +$zip->open($filename, ZipArchive::RDONLY); +$zip->stream = getEntryStream($zip); +unset($zip); +var_dump($resurrected instanceof ResurrectingHolder); +var_dump(stream_get_contents($resurrected->stream)); +fclose($resurrected->stream); +unset($resurrected); + +// Externally held streams no longer keep the object alive. Closing one stream +// must not close the archive while another stream still uses it. +$zip = new Holder; +$zip->open($filename, ZipArchive::RDONLY); +$stream1 = getEntryStream($zip); +$stream2 = getEntryStream($zip); +$weakRef = WeakReference::create($zip); +unset($zip); +var_dump($weakRef->get()); +fclose($stream1); +var_dump(stream_get_contents($stream2)); +fclose($stream2); +?> +--CLEAN-- + +--EXPECT-- +NULL +NULL +NULL +bool(true) +string(8) "contents" +NULL +string(8) "contents" diff --git a/ext/zip/tests/gh23276_cancel_callback.phpt b/ext/zip/tests/gh23276_cancel_callback.phpt new file mode 100644 index 000000000000..5aa9057e6f39 --- /dev/null +++ b/ext/zip/tests/gh23276_cancel_callback.phpt @@ -0,0 +1,49 @@ +--TEST-- +GH-23276 (ZipArchive cancel callback outlives the object while a stream holds the archive) +--EXTENSIONS-- +zip +--SKIPIF-- + +--FILE-- +open($filename, ZipArchive::CREATE | ZipArchive::OVERWRITE); +$zip->addFromString('entry.txt', 'contents'); +$zip->close(); + +$zip->open($filename); +$callbackState = new stdClass; +$callbackStateRef = WeakReference::create($callbackState); +var_dump($zip->registerCancelCallback( + static function () use ($callbackState): int { return 0; }, +)); +$zip->addFromString('cancel.txt', 'late'); +$stream = $zip->getStream('entry.txt'); +if (!is_resource($stream)) { + throw new Exception('Failed to open entry stream'); +} +$weakRef = WeakReference::create($zip); +unset($callbackState, $zip); + +var_dump($weakRef->get()); +var_dump($callbackStateRef->get() !== null); +var_dump(stream_get_contents($stream)); +fclose($stream); +var_dump($callbackStateRef->get()); +?> +--CLEAN-- + +--EXPECT-- +bool(true) +NULL +bool(true) +string(8) "contents" +NULL diff --git a/ext/zip/tests/gh23276_close_with_open_stream.phpt b/ext/zip/tests/gh23276_close_with_open_stream.phpt new file mode 100644 index 000000000000..e216c95f4eb9 --- /dev/null +++ b/ext/zip/tests/gh23276_close_with_open_stream.phpt @@ -0,0 +1,47 @@ +--TEST-- +GH-23276 (ZipArchive dropping its archive while a stream is open leaves the object collectable) +--EXTENSIONS-- +zip +--FILE-- +open($filename, ZipArchive::CREATE | ZipArchive::OVERWRITE); +$zip->addFromString('entry.txt', 'contents'); +$zip->close(); + +$zip->open($filename, ZipArchive::RDONLY); +$stream = $zip->getStream('entry.txt'); +var_dump($zip->close()); +$weakRef = WeakReference::create($zip); +unset($zip); +var_dump($weakRef->get()); +var_dump(stream_get_contents($stream)); +fclose($stream); + +$zip = new ZipArchive; +$zip->open($filename, ZipArchive::RDONLY); +$stream = $zip->getStream('entry.txt'); +var_dump($zip->open($filename, ZipArchive::RDONLY)); +$weakRef = WeakReference::create($zip); +unset($zip); +var_dump($weakRef->get()); +var_dump(stream_get_contents($stream)); +fclose($stream); +?> +--CLEAN-- + +--EXPECTF-- +bool(true) +NULL + +Warning: stream_get_contents(): Zip stream error: %s in %s on line %d +string(0) "" +bool(true) +NULL + +Warning: stream_get_contents(): Zip stream error: %s in %s on line %d +string(0) "" diff --git a/ext/zip/tests/gh23276_progress_callback.phpt b/ext/zip/tests/gh23276_progress_callback.phpt new file mode 100644 index 000000000000..ff37b6233e5a --- /dev/null +++ b/ext/zip/tests/gh23276_progress_callback.phpt @@ -0,0 +1,50 @@ +--TEST-- +GH-23276 (ZipArchive progress callback outlives the object while a stream holds the archive) +--EXTENSIONS-- +zip +--SKIPIF-- + +--FILE-- +open($filename, ZipArchive::CREATE | ZipArchive::OVERWRITE); +$zip->addFromString('entry.txt', 'contents'); +$zip->close(); + +$zip->open($filename); +$callbackState = new stdClass; +$callbackStateRef = WeakReference::create($callbackState); +var_dump($zip->registerProgressCallback( + 0.5, + static function (float $rate) use ($callbackState): void {}, +)); +$zip->addFromString('progress.txt', 'late'); +$stream = $zip->getStream('entry.txt'); +if (!is_resource($stream)) { + throw new Exception('Failed to open entry stream'); +} +$weakRef = WeakReference::create($zip); +unset($callbackState, $zip); + +var_dump($weakRef->get()); +var_dump($callbackStateRef->get() !== null); +var_dump(stream_get_contents($stream)); +fclose($stream); +var_dump($callbackStateRef->get()); +?> +--CLEAN-- + +--EXPECT-- +bool(true) +NULL +bool(true) +string(8) "contents" +NULL diff --git a/ext/zip/tests/oo_addfromstring_reopen_memory.phpt b/ext/zip/tests/oo_addfromstring_reopen_memory.phpt new file mode 100644 index 000000000000..fb721c2d6273 --- /dev/null +++ b/ext/zip/tests/oo_addfromstring_reopen_memory.phpt @@ -0,0 +1,26 @@ +--TEST-- +ZipArchive::addFromString() buffers are released when the archive is closed +--EXTENSIONS-- +zip +--FILE-- +open($filename, ZipArchive::CREATE | ZipArchive::OVERWRITE); + $zip->addFromString('entry.txt', $blob); + $zip->close(); +} + +var_dump(memory_get_usage() - $start < 1000000); +?> +--CLEAN-- + +--EXPECT-- +bool(true) diff --git a/ext/zip/zip_stream.c b/ext/zip/zip_stream.c index 89c6a46e653d..a6665630e350 100644 --- a/ext/zip/zip_stream.c +++ b/ext/zip/zip_stream.c @@ -32,7 +32,7 @@ struct php_zip_stream_data_t { struct zip_file *zf; size_t cursor; php_stream *stream; - ze_zip_object *owner; + php_zip_archive *archive; }; #define STREAM_DATA_FROM_STREAM() \ @@ -82,6 +82,7 @@ static ssize_t php_zip_ops_write(php_stream *stream, const char *buf, size_t cou static int php_zip_ops_close(php_stream *stream, int close_handle) { STREAM_DATA_FROM_STREAM(); + bool bailout = false; if (close_handle) { if (self->zf) { zip_fclose(self->zf); @@ -94,13 +95,16 @@ static int php_zip_ops_close(php_stream *stream, int close_handle) } } - /* the pinned object ref is tied to self, so release it regardless of close_handle */ - if (self->owner) { - OBJ_RELEASE(&self->owner->zo); - self->owner = NULL; + /* the archive ref is tied to self, so release it regardless of close_handle */ + if (self->archive) { + bailout = php_zip_archive_release(self->archive); + self->archive = NULL; } efree(self); stream->abstract = NULL; + if (bailout) { + zend_bailout(); + } return EOF; } /* }}} */ @@ -236,7 +240,7 @@ const php_stream_ops php_stream_zipio_ops = { /* {{{ php_stream_zip_open */ php_stream *php_stream_zip_open(ze_zip_object *obj, struct zip_stat *sb, const char *mode, zip_flags_t flags STREAMS_DC) { - struct zip *arch = obj->za; + struct zip *arch = php_zip_object_za(obj); struct zip_file *zf = NULL; php_stream *stream = NULL; @@ -255,9 +259,9 @@ php_stream *php_stream_zip_open(ze_zip_object *obj, struct zip_stat *sb, const c self->zf = zf; self->stream = NULL; self->cursor = 0; - /* keep the archive object alive while the stream borrows its zip_t */ - self->owner = obj; - GC_ADDREF(&obj->zo); + /* keep the zip_t alive while the stream borrows it */ + self->archive = obj->archive; + php_zip_archive_addref(self->archive); #if LIBZIP_ATLEAST(1,9,1) if (zip_file_is_seekable(zf) > 0) { stream = php_stream_alloc(&php_stream_zipio_seek_ops, self, NULL, mode); @@ -343,7 +347,7 @@ php_stream *php_stream_zip_opener(php_stream_wrapper *wrapper, self->zf = zf; self->stream = NULL; self->cursor = 0; - self->owner = NULL; + self->archive = NULL; #if LIBZIP_ATLEAST(1,9,1) if (zip_file_is_seekable(zf) > 0) { stream = php_stream_alloc(&php_stream_zipio_seek_ops, self, NULL, mode);