ext/zip: Minor refactorings to callbacks

ext/zip/php_zip.c

@@ -1019,25 +1019,23 @@ static HashTable *php_zip_get_properties(zend_object *object)/* {{{ */
 /* }}} */
 
 #ifdef HAVE_PROGRESS_CALLBACK
-static void _php_zip_progress_callback_free(void *ptr)
+static void php_zip_progress_callback_free(void *ptr)
 {
 	ze_zip_object *obj = ptr;
 
-	if (!Z_ISUNDEF(obj->progress_callback)) {
-		zval_ptr_dtor(&obj->progress_callback);
-		ZVAL_UNDEF(&obj->progress_callback);
+	if (ZEND_FCC_INITIALIZED(obj->progress_callback)) {
+		zend_fcc_dtor(&obj->progress_callback);
 	}
 }
 #endif
 
 #ifdef HAVE_CANCEL_CALLBACK
-static void _php_zip_cancel_callback_free(void *ptr)
+static void php_zip_cancel_callback_free(void *ptr)
 {
 	ze_zip_object *obj = ptr;
 
-	if (!Z_ISUNDEF(obj->cancel_callback)) {
-		zval_ptr_dtor(&obj->cancel_callback);
-		ZVAL_UNDEF(&obj->cancel_callback);
+	if (ZEND_FCC_INITIALIZED(obj->cancel_callback)) {
+		zend_fcc_dtor(&obj->cancel_callback);
 	}
 }
 #endif
@@ -1066,12 +1064,12 @@ static void php_zip_object_free_storage(zend_object *object) /* {{{ */
 
 #ifdef HAVE_PROGRESS_CALLBACK
 	/* if not properly called by libzip */
-	_php_zip_progress_callback_free(intern);
+	php_zip_progress_callback_free(intern);
 #endif
 
 #ifdef HAVE_CANCEL_CALLBACK
 	/* if not properly called by libzip */
-	_php_zip_cancel_callback_free(intern);
+	php_zip_cancel_callback_free(intern);
 #endif
 
 	intern->za = NULL;
@@ -3018,42 +3016,43 @@ PHP_METHOD(ZipArchive, getStream)
 }
 
 #ifdef HAVE_PROGRESS_CALLBACK
-static void _php_zip_progress_callback(zip_t *arch, double state, void *ptr)
+static void php_zip_progress_callback(zip_t *arch, double state, void *ptr)
 {
 	zval cb_args[1];
-	zval cb_retval;
 	ze_zip_object *obj = ptr;
 
 	ZVAL_DOUBLE(&cb_args[0], state);
-	if (call_user_function(EG(function_table), NULL, &obj->progress_callback, &cb_retval, 1, cb_args) == SUCCESS && !Z_ISUNDEF(cb_retval)) {
-		zval_ptr_dtor(&cb_retval);
-	}
+	zend_call_known_fcc(&obj->progress_callback, NULL, 1, cb_args, NULL);
 }
 
 /* {{{ register a progression callback: void callback(double state); */
 PHP_METHOD(ZipArchive, registerProgressCallback)
 {
 	struct zip *intern;
-	zval *self = ZEND_THIS;
 	double rate;
-	zend_fcall_info fci;
+	zend_fcall_info dummy_fci;
 	zend_fcall_info_cache fcc;
 	ze_zip_object *obj;
 
-	if (zend_parse_parameters(ZEND_NUM_ARGS(), "df", &rate, &fci, &fcc) == FAILURE) {
+	if (zend_parse_parameters(ZEND_NUM_ARGS(), "dF", &rate, &dummy_fci, &fcc) == FAILURE) {
 		RETURN_THROWS();
 	}
 
-	ZIP_FROM_OBJECT(intern, self);
-
-	obj = Z_ZIP_P(self);
+	/* Inline ZIP_FROM_OBJECT(intern, self); */
+	obj = Z_ZIP_P(ZEND_THIS);
+	intern = obj->za;
+	if (!intern) { \
+		zend_value_error("Invalid or uninitialized Zip object");
+		zend_release_fcall_info_cache(&fcc);
+		RETURN_THROWS();
+	}
 
 	/* free if called twice */
-	_php_zip_progress_callback_free(obj);
+	php_zip_progress_callback_free(obj);
 
 	/* register */
-	ZVAL_COPY(&obj->progress_callback, &fci.function_name);
-	if (zip_register_progress_callback_with_state(intern, rate, _php_zip_progress_callback, _php_zip_progress_callback_free, obj)) {
+	zend_fcc_dup(&obj->progress_callback, &fcc);
+	if (zip_register_progress_callback_with_state(intern, rate, php_zip_progress_callback, php_zip_progress_callback_free, obj)) {
 		RETURN_FALSE;
 	}
 
@@ -3063,42 +3062,55 @@ PHP_METHOD(ZipArchive, registerProgressCallback)
 #endif
 
 #ifdef HAVE_CANCEL_CALLBACK
-static int _php_zip_cancel_callback(zip_t *arch, void *ptr)
+static int php_zip_cancel_callback(zip_t *arch, void *ptr)
 {
 	zval cb_retval;
-	int retval = 0;
 	ze_zip_object *obj = ptr;
 
-	if (call_user_function(EG(function_table), NULL, &obj->cancel_callback, &cb_retval, 0, NULL) == SUCCESS && !Z_ISUNDEF(cb_retval)) {
-		retval = zval_get_long(&cb_retval);
+	zend_call_known_fcc(&obj->cancel_callback, &cb_retval, 0, NULL, NULL);
+	if (Z_ISUNDEF(cb_retval)) {
+		/* Cancel if an exception has been thrown */
+		return -1;
+	}
+	bool failed = false;
+	zend_long retval = zval_try_get_long(&cb_retval, &failed);
+	if (failed) {
+		zend_type_error("Return value of callback provided to ZipArchive::registerCancelCallback()"
+			" must be of type int, %s returned", zend_zval_value_name(&cb_retval));
 		zval_ptr_dtor(&cb_retval);
+		return -1;
 	}
+	zval_ptr_dtor(&cb_retval);
 
-	return retval;
+	return (int) retval;
 }
 
 /* {{{ register a progression callback: int callback(double state); */
 PHP_METHOD(ZipArchive, registerCancelCallback)
 {
 	struct zip *intern;
-	zval *self = ZEND_THIS;
-	zend_fcall_info fci;
+	zend_fcall_info dummy_fci;
 	zend_fcall_info_cache fcc;
 	ze_zip_object *obj;
-	if (zend_parse_parameters(ZEND_NUM_ARGS(), "f", &fci, &fcc) == FAILURE) {
+	if (zend_parse_parameters(ZEND_NUM_ARGS(), "F", &dummy_fci, &fcc) == FAILURE) {
 		RETURN_THROWS();
 	}
 
-	ZIP_FROM_OBJECT(intern, self);
-
-	obj = Z_ZIP_P(self);
+	/* Inline ZIP_FROM_OBJECT(intern, self); */
+	obj = Z_ZIP_P(ZEND_THIS);
+	intern = obj->za;
+	if (!intern) { \
+		zend_value_error("Invalid or uninitialized Zip object");
+		zend_release_fcall_info_cache(&fcc);
+		RETURN_THROWS();
+	}
 
 	/* free if called twice */
-	_php_zip_cancel_callback_free(obj);
+	php_zip_cancel_callback_free(obj);
 
 	/* register */
-	ZVAL_COPY(&obj->cancel_callback, &fci.function_name);
-	if (zip_register_cancel_callback_with_state(intern, _php_zip_cancel_callback, _php_zip_cancel_callback_free, obj)) {
+	zend_fcc_dup(&obj->cancel_callback, &fcc);
+	if (zip_register_cancel_callback_with_state(intern, php_zip_cancel_callback, php_zip_cancel_callback_free, obj)) {
 		RETURN_FALSE;
 	}
 

ext/zip/php_zip.h

@@ -74,10 +74,10 @@ typedef struct _ze_zip_object {
 	int err_zip;
 	int err_sys;
 #ifdef HAVE_PROGRESS_CALLBACK
-	zval progress_callback;
+	zend_fcall_info_cache progress_callback;
 #endif
 #ifdef HAVE_CANCEL_CALLBACK
-	zval cancel_callback;
+	zend_fcall_info_cache cancel_callback;
 #endif
 	zend_object zo;
 } ze_zip_object;

ext/zip/tests/oo_cancel.phpt

@@ -1,5 +1,5 @@
 --TEST--
-registerCancelCallback
+ZipArchive::registerCancelCallback() with a normal callback
 --EXTENSIONS--
 zip
 --SKIPIF--
@@ -14,8 +14,6 @@ date.timezone=UTC
 $dirname = dirname(__FILE__) . '/';
 $file = $dirname . '__tmp_oo_cancel.zip';
 
-@unlink($file);
-
 $zip = new ZipArchive;
 if (!$zip->open($file, ZIPARCHIVE::CREATE)) {
     exit('failed');
@@ -33,6 +31,13 @@ var_dump($zip->getStatusString());
 @unlink($file);
 ?>
 Done
+--CLEAN--
+<?php
+$dirname = dirname(__FILE__) . '/';
+$file = $dirname . '__tmp_oo_cancel.zip';
+
+@unlink($file);
+?>
 --EXPECTF--
 bool(true)
 bool(true)

ext/zip/tests/oo_cancel_non_int_return.phpt

@@ -0,0 +1,52 @@
+--TEST--
+ZipArchive::registerCancelCallback() with a callback returning an incorrect type
+--EXTENSIONS--
+zip
+--SKIPIF--
+<?php
+/* $Id$ */
+if (!method_exists('ZipArchive', 'registerCancelCallback')) die('skip libzip too old');
+?>
+--INI--
+date.timezone=UTC
+--FILE--
+<?php
+$dirname = dirname(__FILE__) . '/';
+$file = $dirname . '__tmp_oo_cancel_incorrect_return.zip';
+
+$zip = new ZipArchive;
+if (!$zip->open($file, ZIPARCHIVE::CREATE)) {
+    exit('failed');
+}
+
+/* Register a bogus callback */
+var_dump($zip->registerCancelCallback(function () {
+    return [];
+}));
+var_dump($zip->addFromString(PHP_BINARY, 'entry #1'));
+try {
+    var_dump($zip->close());
+} catch (Throwable $e) {
+    echo $e::class, ': ', $e->getMessage(), PHP_EOL;
+}
+var_dump($zip->status == ZipArchive::ER_CANCELLED);
+var_dump($zip->getStatusString());
+@unlink($file);
+?>
+Done
+--CLEAN--
+<?php
+$dirname = dirname(__FILE__) . '/';
+$file = $dirname . '__tmp_oo_cancel_incorrect_return.zip';
+
+@unlink($file);
+?>
+--EXPECTF--
+bool(true)
+bool(true)
+
+Warning: ZipArchive::close(): Operation cancelled in %s on line %d
+TypeError: Return value of callback provided to ZipArchive::registerCancelCallback() must be of type int, array returned
+bool(true)
+string(19) "Operation cancelled"
+Done

ext/zip/tests/oo_cancel_trampoline.phpt

@@ -0,0 +1,99 @@
+--TEST--
+ZipArchive::registerCancelCallback() with a trampoline
+--EXTENSIONS--
+zip
+--SKIPIF--
+<?php
+/* $Id$ */
+if (!method_exists('ZipArchive', 'registerCancelCallback')) die('skip libzip too old');
+?>
+--INI--
+date.timezone=UTC
+--FILE--
+<?php
+$dirname = dirname(__FILE__) . '/';
+$file = $dirname . '__tmp_oo_cancel_trampoline.zip';
+
+class TrampolineTest {
+    public function __call(string $name, array $arguments): int {
+        echo 'Trampoline for ', $name, PHP_EOL;
+        if ($name === 'trampolineThrow') {
+            throw new Exception('boo');
+        }
+        var_dump($arguments);
+        return -1;
+    }
+}
+
+$o = new TrampolineTest();
+$callback = [$o, 'trampoline'];
+$callbackThrow = [$o, 'trampolineThrow'];
+
+$zip = new ZipArchive;
+if (!$zip->open($file, ZIPARCHIVE::CREATE)) {
+    exit('failed');
+}
+
+var_dump($zip->registerCancelCallback($callback));
+var_dump($zip->addFromString(PHP_BINARY, 'entry #1'));
+var_dump($zip->close());
+var_dump($zip->status == ZipArchive::ER_CANCELLED);
+var_dump($zip->getStatusString());
+
+echo "Set trampoline after closed Archive:\n";
+try {
+    var_dump($zip->registerCancelCallback($callback));
+} catch (Throwable $e) {
+    echo $e::class, ': ', $e->getMessage(), PHP_EOL;
+}
+
+@unlink($file);
+unset($zip);
+
+$zip = new ZipArchive;
+if (!$zip->open($file, ZIPARCHIVE::CREATE)) {
+    exit('failed');
+}
+
+var_dump($zip->registerCancelCallback($callbackThrow));
+var_dump($zip->addFromString(PHP_BINARY, 'entry #1'));
+try {
+    var_dump($zip->close());
+} catch (Throwable $e) {
+    echo $e::class, ': ', $e->getMessage(), PHP_EOL;
+}
+var_dump($zip->status == ZipArchive::ER_CANCELLED);
+var_dump($zip->getStatusString());
+@unlink($file);
+
+?>
+Done
+--CLEAN--
+<?php
+$dirname = dirname(__FILE__) . '/';
+$file = $dirname . '__tmp_oo_cancel_trampoline.zip';
+
+@unlink($file);
+?>
+--EXPECTF--
+bool(true)
+bool(true)
+Trampoline for trampoline
+array(0) {
+}
+
+Warning: ZipArchive::close(): Operation cancelled in %s on line %d
+bool(false)
+bool(true)
+string(19) "Operation cancelled"
+Set trampoline after closed Archive:
+ValueError: Invalid or uninitialized Zip object
+bool(true)
+bool(true)
+Trampoline for trampolineThrow
+
+Warning: ZipArchive::close(): Operation cancelled in %s on line %d
+Exception: boo
+bool(true)
+string(19) "Operation cancelled"
+Done