From 18fa1e4a829fe4375126eaa167672713b6ef2276 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Wed, 8 Jul 2020 15:51:28 +0200 Subject: [PATCH 1/8] ZPP Callable --- ext/zip/php_zip.c | 35 ++++++++++------------------------- 1 file changed, 10 insertions(+), 25 deletions(-) diff --git a/ext/zip/php_zip.c b/ext/zip/php_zip.c index de3a695d96401..ed10ac4d9549d 100644 --- a/ext/zip/php_zip.c +++ b/ext/zip/php_zip.c @@ -2886,19 +2886,12 @@ PHP_METHOD(ZipArchive, registerProgressCallback) struct zip *intern; zval *self = ZEND_THIS; double rate; - zval *callback; + zend_fcall_info fci; + zend_fcall_info_cache fcc; ze_zip_object *obj; - if (zend_parse_parameters(ZEND_NUM_ARGS(), "dz", &rate, &callback) == FAILURE) { - return; - } - - /* callable? */ - if (!zend_is_callable(callback, 0, NULL)) { - zend_string *callback_name = zend_get_callable_name(callback); - php_error_docref(NULL, E_WARNING, "Invalid callback '%s'", ZSTR_VAL(callback_name)); - zend_string_release_ex(callback_name, 0); - RETURN_FALSE; + if (zend_parse_parameters(ZEND_NUM_ARGS(), "df", &rate, &fci, &fcc) == FAILURE) { + RETURN_THROWS(); } ZIP_FROM_OBJECT(intern, self); @@ -2909,7 +2902,7 @@ PHP_METHOD(ZipArchive, registerProgressCallback) _php_zip_progress_callback_free(obj); /* register */ - ZVAL_COPY(&obj->progress_callback, callback); + 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)) { RETURN_FALSE; } @@ -2939,30 +2932,22 @@ PHP_METHOD(ZipArchive, registerCancelCallback) { struct zip *intern; zval *self = ZEND_THIS; - zval *callback; + zend_fcall_info fci; + zend_fcall_info_cache fcc; ze_zip_object *obj; - - if (zend_parse_parameters(ZEND_NUM_ARGS(), "z", &callback) == FAILURE) { - return; + if (zend_parse_parameters(ZEND_NUM_ARGS(), "f", &fci, &fcc) == FAILURE) { + RETURN_THROWS(); } ZIP_FROM_OBJECT(intern, self); - /* callable? */ - if (!zend_is_callable(callback, 0, NULL)) { - zend_string *callback_name = zend_get_callable_name(callback); - php_error_docref(NULL, E_WARNING, "Invalid callback '%s'", ZSTR_VAL(callback_name)); - zend_string_release_ex(callback_name, 0); - RETURN_FALSE; - } - obj = Z_ZIP_P(self); /* free if called twice */ _php_zip_cancel_callback_free(obj); /* register */ - ZVAL_COPY(&obj->cancel_callback, callback); + 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)) { RETURN_FALSE; } From d81ae8697e3d0ced2f6d11bf980723e052b3dac5 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Wed, 8 Jul 2020 15:02:06 +0200 Subject: [PATCH 2/8] Notice promotion to ValueError --- ext/zip/php_zip.c | 112 ++++++++++++++----------- ext/zip/tests/oo_getcomment.phpt | 18 ++-- ext/zip/tests/oo_open.phpt | 11 ++- ext/zip/tests/oo_setcomment_error.phpt | 31 ++++--- ext/zip/tests/zip_open_error.phpt | 9 +- 5 files changed, 108 insertions(+), 73 deletions(-) diff --git a/ext/zip/php_zip.c b/ext/zip/php_zip.c index ed10ac4d9549d..47fcac8d388f3 100644 --- a/ext/zip/php_zip.c +++ b/ext/zip/php_zip.c @@ -52,11 +52,12 @@ static int le_zip_entry; } /* }}} */ -/* {{{ PHP_ZIP_STAT_PATH(za, path, path_len, flags, sb) */ +/* {{{ PHP_ZIP_STAT_PATH(za, path, path_len, flags, sb) + This is always used for the first argument*/ #define PHP_ZIP_STAT_PATH(za, path, path_len, flags, sb) \ - if (path_len < 1) { \ - php_error_docref(NULL, E_NOTICE, "Empty string as entry name"); \ - RETURN_FALSE; \ + if (path_len == 0) { \ + zend_argument_value_error(1, "cannot be empty"); \ + RETURN_THROWS(); \ } \ if (zip_stat(za, path, flags, &sb) != 0) { \ RETURN_FALSE; \ @@ -599,6 +600,7 @@ int php_zip_glob(char *pattern, int pattern_len, zend_long flags, zval *return_v } if ((GLOB_AVAILABLE_FLAGS & flags) != flags) { + php_error_docref(NULL, E_WARNING, "At least one of the passed flags is invalid or not supported on this platform"); return -1; } @@ -1134,8 +1136,8 @@ PHP_FUNCTION(zip_open) } if (ZSTR_LEN(filename) == 0) { - php_error_docref(NULL, E_WARNING, "Empty string as source"); - RETURN_FALSE; + zend_argument_value_error(1, "cannot be empty"); + RETURN_THROWS(); } if (ZIP_OPENBASEDIR_CHECKPATH(ZSTR_VAL(filename))) { @@ -1414,8 +1416,8 @@ PHP_METHOD(ZipArchive, open) ze_obj = Z_ZIP_P(self); if (ZSTR_LEN(filename) == 0) { - php_error_docref(NULL, E_WARNING, "Empty string as source"); - RETURN_FALSE; + zend_argument_value_error(1, "cannot be empty"); + RETURN_THROWS(); } if (ZIP_OPENBASEDIR_CHECKPATH(ZSTR_VAL(filename))) { @@ -1429,6 +1431,7 @@ PHP_METHOD(ZipArchive, open) if (ze_obj->za) { /* we already have an opened zip, free it */ if (zip_close(ze_obj->za) != 0) { + // TODO Check this can be reached? php_error_docref(NULL, E_WARNING, "Empty string as source"); efree(resolved_path); RETURN_FALSE; @@ -1677,8 +1680,8 @@ static void php_zip_add_from_pattern(INTERNAL_FUNCTION_PARAMETERS, int type) /* } if (ZSTR_LEN(pattern) == 0) { - php_error_docref(NULL, E_NOTICE, "Empty string as pattern"); - RETURN_FALSE; + zend_argument_value_error(1, "cannot be empty"); + RETURN_THROWS(); } if (options && zend_hash_num_elements(options) > 0 && (php_zip_parse_options(options, &opts) < 0)) { RETURN_FALSE; @@ -1795,8 +1798,8 @@ PHP_METHOD(ZipArchive, addFile) } if (ZSTR_LEN(filename) == 0) { - php_error_docref(NULL, E_NOTICE, "Empty string as filename"); - RETURN_FALSE; + zend_argument_value_error(1, "cannot be empty"); + RETURN_THROWS(); } if (entry_name_len == 0) { @@ -1828,13 +1831,13 @@ PHP_METHOD(ZipArchive, replaceFile) } if (ZSTR_LEN(filename) == 0) { - php_error_docref(NULL, E_NOTICE, "Empty string as filename"); - RETURN_FALSE; + zend_argument_value_error(1, "cannot be empty"); + RETURN_THROWS(); } if (index < 0) { - php_error_docref(NULL, E_NOTICE, "Invalid negative index"); - RETURN_FALSE; + zend_argument_value_error(2, "must be greater than or equal to 0"); + RETURN_THROWS(); } if (php_zip_add_file(Z_ZIP_P(self), ZSTR_VAL(filename), ZSTR_LEN(filename), @@ -2008,8 +2011,8 @@ PHP_METHOD(ZipArchive, setArchiveComment) ZIP_FROM_OBJECT(intern, self); if (comment_len > 0xffff) { - php_error_docref(NULL, E_WARNING, "Comment must not exceed 65535 bytes"); - RETURN_FALSE; + zend_argument_value_error(1, "must be less than 65535 bytes"); + RETURN_THROWS(); } if (zip_set_archive_comment(intern, (const char *)comment, comment_len)) { @@ -2057,19 +2060,21 @@ PHP_METHOD(ZipArchive, setCommentName) RETURN_THROWS(); } - if (name_len < 1) { - php_error_docref(NULL, E_NOTICE, "Empty string as entry name"); + if (name_len == 0) { + zend_argument_value_error(1, "cannot be empty"); + RETURN_THROWS(); } ZIP_FROM_OBJECT(intern, self); if (comment_len > 0xffff) { - php_error_docref(NULL, E_WARNING, "Comment must not exceed 65535 bytes"); - RETURN_FALSE; + zend_argument_value_error(2, "must be less than 65535 bytes"); + RETURN_THROWS(); } idx = zip_name_locate(intern, name, 0); if (idx < 0) { + // TODO Warning? RETURN_FALSE; } PHP_ZIP_SET_FILE_COMMENT(intern, idx, comment, comment_len); @@ -2093,9 +2098,10 @@ PHP_METHOD(ZipArchive, setCommentIndex) ZIP_FROM_OBJECT(intern, self); + // TODO Check for Index value? if (comment_len > 0xffff) { - php_error_docref(NULL, E_WARNING, "Comment must not exceed 65535 bytes"); - RETURN_FALSE; + zend_argument_value_error(2, "must be less than 65535 bytes"); + RETURN_THROWS(); } PHP_ZIP_STAT_INDEX(intern, index, 0, sb); @@ -2123,11 +2129,13 @@ PHP_METHOD(ZipArchive, setExternalAttributesName) ZIP_FROM_OBJECT(intern, self); - if (name_len < 1) { - php_error_docref(NULL, E_NOTICE, "Empty string as entry name"); + if (name_len == 0) { + zend_argument_value_error(1, "cannot be empty"); + RETURN_THROWS(); } idx = zip_name_locate(intern, name, 0); + // TODO Warning? if (idx < 0) { RETURN_FALSE; } @@ -2182,11 +2190,13 @@ PHP_METHOD(ZipArchive, getExternalAttributesName) ZIP_FROM_OBJECT(intern, self); - if (name_len < 1) { - php_error_docref(NULL, E_NOTICE, "Empty string as entry name"); + if (name_len == 0) { + zend_argument_value_error(1, "cannot be empty"); + RETURN_THROWS(); } idx = zip_name_locate(intern, name, 0); + // TODO Warning? if (idx < 0) { RETURN_FALSE; } @@ -2247,11 +2257,13 @@ PHP_METHOD(ZipArchive, setEncryptionName) ZIP_FROM_OBJECT(intern, self); - if (name_len < 1) { - php_error_docref(NULL, E_NOTICE, "Empty string as entry name"); + if (name_len == 0) { + zend_argument_value_error(1, "cannot be empty"); + RETURN_THROWS(); } idx = zip_name_locate(intern, name, 0); + // TODO Warning? if (idx < 0) { RETURN_FALSE; } @@ -2306,12 +2318,13 @@ PHP_METHOD(ZipArchive, getCommentName) ZIP_FROM_OBJECT(intern, self); - if (name_len < 1) { - php_error_docref(NULL, E_NOTICE, "Empty string as entry name"); - RETURN_FALSE; + if (name_len == 0) { + zend_argument_value_error(1, "cannot be empty"); + RETURN_THROWS(); } idx = zip_name_locate(intern, name, 0); + // TODO Warning? if (idx < 0) { RETURN_FALSE; } @@ -2346,7 +2359,7 @@ PHP_METHOD(ZipArchive, getCommentIndex) /* {{{ Set the compression of a file in zip, using its name */ PHP_METHOD(ZipArchive, setCompressionName) - { +{ struct zip *intern; zval *this = ZEND_THIS; size_t name_len; @@ -2361,11 +2374,13 @@ PHP_METHOD(ZipArchive, setCompressionName) ZIP_FROM_OBJECT(intern, this); - if (name_len < 1) { - php_error_docref(NULL, E_NOTICE, "Empty string as entry name"); + if (name_len == 0) { + zend_argument_value_error(1, "cannot be empty"); + RETURN_THROWS(); } idx = zip_name_locate(intern, name, 0); + // TODO Warning? if (idx < 0) { RETURN_FALSE; } @@ -2404,7 +2419,7 @@ PHP_METHOD(ZipArchive, setCompressionIndex) #ifdef HAVE_SET_MTIME /* {{{ Set the modification time of a file in zip, using its name */ PHP_METHOD(ZipArchive, setMtimeName) - { +{ struct zip *intern; zval *this = ZEND_THIS; size_t name_len; @@ -2419,11 +2434,13 @@ PHP_METHOD(ZipArchive, setMtimeName) ZIP_FROM_OBJECT(intern, this); - if (name_len < 1) { - php_error_docref(NULL, E_NOTICE, "Empty string as entry name"); + if (name_len == 0) { + zend_argument_value_error(1, "cannot be empty"); + RETURN_THROWS(); } idx = zip_name_locate(intern, name, 0); + // TODO Warning? if (idx < 0) { RETURN_FALSE; } @@ -2525,15 +2542,16 @@ PHP_METHOD(ZipArchive, renameIndex) RETURN_THROWS(); } + // TODO Warning? if (index < 0) { RETURN_FALSE; } ZIP_FROM_OBJECT(intern, self); - if (new_name_len < 1) { - php_error_docref(NULL, E_NOTICE, "Empty string as new entry name"); - RETURN_FALSE; + if (new_name_len == 0) { + zend_argument_value_error(2, "cannot be empty"); + RETURN_THROWS(); } if (zip_file_rename(intern, index, (const char *)new_name, 0) != 0) { @@ -2559,9 +2577,9 @@ PHP_METHOD(ZipArchive, renameName) ZIP_FROM_OBJECT(intern, self); - if (new_name_len < 1) { - php_error_docref(NULL, E_NOTICE, "Empty string as new entry name"); - RETURN_FALSE; + if (new_name_len == 0) { + zend_argument_value_error(2, "cannot be empty"); + RETURN_THROWS(); } PHP_ZIP_STAT_PATH(intern, name, name_len, 0, sb); @@ -2902,7 +2920,7 @@ PHP_METHOD(ZipArchive, registerProgressCallback) _php_zip_progress_callback_free(obj); /* register */ - ZVAL_COPY(&obj->progress_callback, fci->function_name); + 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)) { RETURN_FALSE; } @@ -2947,7 +2965,7 @@ PHP_METHOD(ZipArchive, registerCancelCallback) _php_zip_cancel_callback_free(obj); /* register */ - ZVAL_COPY(&obj->cancel_callback, fci->function_name); + 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)) { RETURN_FALSE; } diff --git a/ext/zip/tests/oo_getcomment.phpt b/ext/zip/tests/oo_getcomment.phpt index 6f54105cc88d2..8687167a40e93 100644 --- a/ext/zip/tests/oo_getcomment.phpt +++ b/ext/zip/tests/oo_getcomment.phpt @@ -16,16 +16,20 @@ if (!$zip->open($file)) { echo $zip->getArchiveComment() . "\n"; $idx = $zip->locateName('foo'); -echo $zip->getCommentName('foo') . "\n"; -echo $zip->getCommentIndex($idx); +var_dump($zip->getCommentName('foo')); +var_dump($zip->getCommentIndex($idx)); -echo $zip->getCommentName('') . "\n"; +try { + echo $zip->getCommentName('') . "\n"; +} catch (\ValueError $e) { + echo $e->getMessage() . \PHP_EOL; +} $zip->close(); ?> ---EXPECTF-- +--EXPECT-- Zip archive comment -foo comment -foo comment -Notice: ZipArchive::getCommentName(): Empty string as entry name in %s on line %d +string(11) "foo comment" +string(11) "foo comment" +ZipArchive::getCommentName(): Argument #1 ($name) cannot be empty diff --git a/ext/zip/tests/oo_open.phpt b/ext/zip/tests/oo_open.phpt index 2dffdc0d889f7..a7e8d308186df 100644 --- a/ext/zip/tests/oo_open.phpt +++ b/ext/zip/tests/oo_open.phpt @@ -25,7 +25,11 @@ if (!$r) { @unlink($dirname . 'nofile'); $zip = new ZipArchive; -$zip->open(''); +try { + $zip->open(''); +} catch (\ValueError $e) { + echo $e->getMessage() . \PHP_EOL; +} if (!$zip->open($dirname . 'test.zip')) { exit("failed 1\n"); @@ -37,9 +41,8 @@ if ($zip->status == ZIPARCHIVE::ER_OK) { echo "failed\n"; } ?> ---EXPECTF-- +--EXPECT-- ER_OPEN: ok create: ok - -Warning: ZipArchive::open(): Empty string as source in %s on line %d +ZipArchive::open(): Argument #1 ($filename) cannot be empty OK diff --git a/ext/zip/tests/oo_setcomment_error.phpt b/ext/zip/tests/oo_setcomment_error.phpt index 78d8d3dc35602..74a582a6f8efe 100644 --- a/ext/zip/tests/oo_setcomment_error.phpt +++ b/ext/zip/tests/oo_setcomment_error.phpt @@ -20,21 +20,28 @@ $zip->addFromString('entry2.txt', 'entry #2'); $longComment = str_repeat('a', 0x10000); -var_dump($zip->setArchiveComment($longComment)); -var_dump($zip->setCommentName('entry1.txt', $longComment)); -var_dump($zip->setCommentIndex(1, $longComment)); +try { + var_dump($zip->setArchiveComment($longComment)); +} catch (\ValueError $e) { + echo $e->getMessage() . \PHP_EOL; +} +try { + var_dump($zip->setCommentName('entry1.txt', $longComment)); +} catch (\ValueError $e) { + echo $e->getMessage() . \PHP_EOL; +} +try { + var_dump($zip->setCommentIndex(1, $longComment)); +} catch (\ValueError $e) { + echo $e->getMessage() . \PHP_EOL; +} $zip->close(); ?> ---EXPECTF-- -Warning: ZipArchive::setArchiveComment(): Comment must not exceed 65535 bytes in %s on line %d -bool(false) - -Warning: ZipArchive::setCommentName(): Comment must not exceed 65535 bytes in %s on line %d -bool(false) - -Warning: ZipArchive::setCommentIndex(): Comment must not exceed 65535 bytes in %s on line %d -bool(false) +--EXPECT-- +ZipArchive::setArchiveComment(): Argument #1 ($comment) must be less than 65535 bytes +ZipArchive::setCommentName(): Argument #2 ($comment) must be less than 65535 bytes +ZipArchive::setCommentIndex(): Argument #2 ($comment) must be less than 65535 bytes --CLEAN-- getMessage() . \PHP_EOL; +} echo "Test case 2:\n"; $zip = zip_open("/non_exisitng_directory/test_procedural.zip"); @@ -19,8 +23,7 @@ echo is_resource($zip) ? "OK" : "Failure"; --EXPECTF-- Test case 1: Deprecated: Function zip_open() is deprecated in %s on line %d - -Warning: zip_open(): Empty string as source in %s on line %d +zip_open(): Argument #1 ($filename) cannot be empty Test case 2: Deprecated: Function zip_open() is deprecated in %s on line %d From 1a80aa139153ebc36acd23e7872510d1d5689018 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Wed, 8 Jul 2020 17:08:34 +0200 Subject: [PATCH 3/8] php_zip_parse_options() Errors --- ext/zip/php_zip.c | 78 ++++++++++++++++++++++++++++++----------------- 1 file changed, 50 insertions(+), 28 deletions(-) diff --git a/ext/zip/php_zip.c b/ext/zip/php_zip.c index 47fcac8d388f3..395968a4e4502 100644 --- a/ext/zip/php_zip.c +++ b/ext/zip/php_zip.c @@ -335,7 +335,7 @@ typedef struct { #endif } zip_options; -static int php_zip_parse_options(HashTable *options, zip_options *opts) +static bool php_zip_parse_options(HashTable *options, zip_options *opts) /* {{{ */ { zval *option; @@ -349,25 +349,46 @@ static int php_zip_parse_options(HashTable *options, zip_options *opts) #endif if ((option = zend_hash_str_find(options, "remove_all_path", sizeof("remove_all_path") - 1)) != NULL) { - opts->remove_all_path = zval_get_long(option); + if (Z_TYPE_P(option) != IS_FALSE && Z_TYPE_P(option) != IS_TRUE) { + zend_type_error("Option \"remove_all_path\" must be of type bool, %s given", + zend_zval_type_name(option)); + return false; + } + opts->remove_all_path = Z_LVAL_P(option); } if ((option = zend_hash_str_find(options, "comp_method", sizeof("comp_method") - 1)) != NULL) { - opts->comp_method = zval_get_long(option); + if (Z_TYPE_P(option) != IS_LONG) { + zend_type_error("Option \"comp_method\" must be of type int, %s given", + zend_zval_type_name(option)); + return false; + } + opts->comp_method = Z_LVAL_P(option); if ((option = zend_hash_str_find(options, "comp_flags", sizeof("comp_flags") - 1)) != NULL) { - opts->comp_flags = zval_get_long(option); + if (Z_TYPE_P(option) != IS_LONG) { + zend_type_error("Option \"comp_flags\" must be of type int, %s given", + zend_zval_type_name(option)); + return false; + } + opts->comp_flags = Z_LVAL_P(option); } } #ifdef HAVE_ENCRYPTION if ((option = zend_hash_str_find(options, "enc_method", sizeof("enc_method") - 1)) != NULL) { - opts->enc_method = zval_get_long(option); + if (Z_TYPE_P(option) != IS_LONG) { + zend_type_error("Option \"enc_method\" must be of type int, %s given", + zend_zval_type_name(option)); + return false; + } + opts->enc_method = Z_LVAL_P(option); if ((option = zend_hash_str_find(options, "enc_password", sizeof("enc_password") - 1)) != NULL) { if (Z_TYPE_P(option) != IS_STRING) { - php_error_docref(NULL, E_WARNING, "enc_password option expected to be a string"); - return -1; + zend_type_error("Option \"enc_password\" must be of type string, %s given", + zend_zval_type_name(option)); + return false; } opts->enc_password = Z_STRVAL_P(option); } @@ -376,19 +397,19 @@ static int php_zip_parse_options(HashTable *options, zip_options *opts) if ((option = zend_hash_str_find(options, "remove_path", sizeof("remove_path") - 1)) != NULL) { if (Z_TYPE_P(option) != IS_STRING) { - php_error_docref(NULL, E_WARNING, "remove_path option expected to be a string"); - return -1; + zend_type_error("Option \"remove_path\" must be of type string, %s given", + zend_zval_type_name(option)); + return false; } - if (Z_STRLEN_P(option) < 1) { - php_error_docref(NULL, E_NOTICE, "Empty string given as remove_path option"); - return -1; + if (Z_STRLEN_P(option) == 0) { + zend_value_error("Option \"remove_path\" cannot be empty"); + return false; } if (Z_STRLEN_P(option) >= MAXPATHLEN) { - php_error_docref(NULL, E_WARNING, "remove_path string is too long (max: %d, %zd given)", - MAXPATHLEN - 1, Z_STRLEN_P(option)); - return -1; + zend_value_error("Option \"remove_path\" must be less than %d bytes", MAXPATHLEN - 1); + return false; } opts->remove_path_len = Z_STRLEN_P(option); opts->remove_path = Z_STRVAL_P(option); @@ -396,19 +417,19 @@ static int php_zip_parse_options(HashTable *options, zip_options *opts) if ((option = zend_hash_str_find(options, "add_path", sizeof("add_path") - 1)) != NULL) { if (Z_TYPE_P(option) != IS_STRING) { - php_error_docref(NULL, E_WARNING, "add_path option expected to be a string"); - return -1; + zend_type_error("Option \"add_path\" must be of type string, %s given", + zend_zval_type_name(option)); + return false; } - if (Z_STRLEN_P(option) < 1) { - php_error_docref(NULL, E_NOTICE, "Empty string given as the add_path option"); - return -1; + if (Z_STRLEN_P(option) == 0) { + zend_value_error("Option \"add_path\" cannot be empty"); + return false; } if (Z_STRLEN_P(option) >= MAXPATHLEN) { - php_error_docref(NULL, E_WARNING, "add_path string too long (max: %d, %zd given)", - MAXPATHLEN - 1, Z_STRLEN_P(option)); - return -1; + zend_value_error("Option \"add_path\" must be less than %d bytes", MAXPATHLEN - 1); + return false; } opts->add_path_len = Z_STRLEN_P(option); opts->add_path = Z_STRVAL_P(option); @@ -416,13 +437,14 @@ static int php_zip_parse_options(HashTable *options, zip_options *opts) if ((option = zend_hash_str_find(options, "flags", sizeof("flags") - 1)) != NULL) { if (Z_TYPE_P(option) != IS_LONG) { - php_error_docref(NULL, E_WARNING, "flags option expected to be a integer"); - return -1; + zend_type_error("Option \"flags\" must be of type int, %s given", + zend_zval_type_name(option)); + return false; } opts->flags = Z_LVAL_P(option); } - return 1; + return true; } /* }}} */ @@ -1683,8 +1705,8 @@ static void php_zip_add_from_pattern(INTERNAL_FUNCTION_PARAMETERS, int type) /* zend_argument_value_error(1, "cannot be empty"); RETURN_THROWS(); } - if (options && zend_hash_num_elements(options) > 0 && (php_zip_parse_options(options, &opts) < 0)) { - RETURN_FALSE; + if (options && zend_hash_num_elements(options) > 0 && (php_zip_parse_options(options, &opts) == false)) { + RETURN_THROWS(); } if (type == 1) { From a0e6ffc7d1202105380bd3696edcf0aacfefaa77 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Mon, 20 Jul 2020 11:27:55 +0100 Subject: [PATCH 4/8] Warning instead of TypeError for current silent issues --- ext/zip/php_zip.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/ext/zip/php_zip.c b/ext/zip/php_zip.c index 395968a4e4502..291b86b21a4eb 100644 --- a/ext/zip/php_zip.c +++ b/ext/zip/php_zip.c @@ -350,39 +350,35 @@ static bool php_zip_parse_options(HashTable *options, zip_options *opts) if ((option = zend_hash_str_find(options, "remove_all_path", sizeof("remove_all_path") - 1)) != NULL) { if (Z_TYPE_P(option) != IS_FALSE && Z_TYPE_P(option) != IS_TRUE) { - zend_type_error("Option \"remove_all_path\" must be of type bool, %s given", + php_error_docref(NULL, E_WARNING, "Option \"remove_all_path\" must be of type bool, %s given", zend_zval_type_name(option)); - return false; } - opts->remove_all_path = Z_LVAL_P(option); + opts->remove_all_path = zval_get_long(option); } if ((option = zend_hash_str_find(options, "comp_method", sizeof("comp_method") - 1)) != NULL) { if (Z_TYPE_P(option) != IS_LONG) { - zend_type_error("Option \"comp_method\" must be of type int, %s given", + php_error_docref(NULL, E_WARNING, "Option \"comp_method\" must be of type int, %s given", zend_zval_type_name(option)); - return false; } - opts->comp_method = Z_LVAL_P(option); + opts->comp_method = zval_get_long(option); if ((option = zend_hash_str_find(options, "comp_flags", sizeof("comp_flags") - 1)) != NULL) { if (Z_TYPE_P(option) != IS_LONG) { - zend_type_error("Option \"comp_flags\" must be of type int, %s given", + php_error_docref(NULL, E_WARNING, "Option \"comp_flags\" must be of type int, %s given", zend_zval_type_name(option)); - return false; } - opts->comp_flags = Z_LVAL_P(option); + opts->comp_flags = zval_get_long(option); } } #ifdef HAVE_ENCRYPTION if ((option = zend_hash_str_find(options, "enc_method", sizeof("enc_method") - 1)) != NULL) { if (Z_TYPE_P(option) != IS_LONG) { - zend_type_error("Option \"enc_method\" must be of type int, %s given", + php_error_docref(NULL, E_WARNING, "Option \"enc_method\" must be of type int, %s given", zend_zval_type_name(option)); - return false; } - opts->enc_method = Z_LVAL_P(option); + opts->enc_method = zval_get_long(option); if ((option = zend_hash_str_find(options, "enc_password", sizeof("enc_password") - 1)) != NULL) { if (Z_TYPE_P(option) != IS_STRING) { From 7ae81b03aa7db19539cc474e06fb4f3bee342aa4 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Mon, 20 Jul 2020 11:35:28 +0100 Subject: [PATCH 5/8] Promote newly introduced warnings to TypeError in case of strict_types --- ext/zip/php_zip.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/ext/zip/php_zip.c b/ext/zip/php_zip.c index 291b86b21a4eb..1cd310659c83c 100644 --- a/ext/zip/php_zip.c +++ b/ext/zip/php_zip.c @@ -350,6 +350,11 @@ static bool php_zip_parse_options(HashTable *options, zip_options *opts) if ((option = zend_hash_str_find(options, "remove_all_path", sizeof("remove_all_path") - 1)) != NULL) { if (Z_TYPE_P(option) != IS_FALSE && Z_TYPE_P(option) != IS_TRUE) { + if (ZEND_CALL_USES_STRICT_TYPES(EG(current_execute_data))) { + zend_type_error("Option \"comp_method\" must be of type bool, %s given", + zend_zval_type_name(option)); + return false; + } php_error_docref(NULL, E_WARNING, "Option \"remove_all_path\" must be of type bool, %s given", zend_zval_type_name(option)); } @@ -358,6 +363,11 @@ static bool php_zip_parse_options(HashTable *options, zip_options *opts) if ((option = zend_hash_str_find(options, "comp_method", sizeof("comp_method") - 1)) != NULL) { if (Z_TYPE_P(option) != IS_LONG) { + if (ZEND_CALL_USES_STRICT_TYPES(EG(current_execute_data))) { + zend_type_error("Option \"comp_method\" must be of type int, %s given", + zend_zval_type_name(option)); + return false; + } php_error_docref(NULL, E_WARNING, "Option \"comp_method\" must be of type int, %s given", zend_zval_type_name(option)); } @@ -365,6 +375,11 @@ static bool php_zip_parse_options(HashTable *options, zip_options *opts) if ((option = zend_hash_str_find(options, "comp_flags", sizeof("comp_flags") - 1)) != NULL) { if (Z_TYPE_P(option) != IS_LONG) { + if (ZEND_CALL_USES_STRICT_TYPES(EG(current_execute_data))) { + zend_type_error("Option \"comp_flags\" must be of type int, %s given", + zend_zval_type_name(option)); + return false; + } php_error_docref(NULL, E_WARNING, "Option \"comp_flags\" must be of type int, %s given", zend_zval_type_name(option)); } @@ -375,6 +390,11 @@ static bool php_zip_parse_options(HashTable *options, zip_options *opts) #ifdef HAVE_ENCRYPTION if ((option = zend_hash_str_find(options, "enc_method", sizeof("enc_method") - 1)) != NULL) { if (Z_TYPE_P(option) != IS_LONG) { + if (ZEND_CALL_USES_STRICT_TYPES(EG(current_execute_data))) { + zend_type_error("Option \"enc_method\" must be of type int, %s given", + zend_zval_type_name(option)); + return false; + } php_error_docref(NULL, E_WARNING, "Option \"enc_method\" must be of type int, %s given", zend_zval_type_name(option)); } From 20523f8cb298ac5a7e6d8aa5d95f2cfb5a255004 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Tue, 11 Aug 2020 18:45:32 +0200 Subject: [PATCH 6/8] Revert "Promote newly introduced warnings to TypeError in case of strict_types" This reverts commit 9cefc060cff505adefc9f9a19311e614e83b567e. --- ext/zip/php_zip.c | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/ext/zip/php_zip.c b/ext/zip/php_zip.c index 1cd310659c83c..291b86b21a4eb 100644 --- a/ext/zip/php_zip.c +++ b/ext/zip/php_zip.c @@ -350,11 +350,6 @@ static bool php_zip_parse_options(HashTable *options, zip_options *opts) if ((option = zend_hash_str_find(options, "remove_all_path", sizeof("remove_all_path") - 1)) != NULL) { if (Z_TYPE_P(option) != IS_FALSE && Z_TYPE_P(option) != IS_TRUE) { - if (ZEND_CALL_USES_STRICT_TYPES(EG(current_execute_data))) { - zend_type_error("Option \"comp_method\" must be of type bool, %s given", - zend_zval_type_name(option)); - return false; - } php_error_docref(NULL, E_WARNING, "Option \"remove_all_path\" must be of type bool, %s given", zend_zval_type_name(option)); } @@ -363,11 +358,6 @@ static bool php_zip_parse_options(HashTable *options, zip_options *opts) if ((option = zend_hash_str_find(options, "comp_method", sizeof("comp_method") - 1)) != NULL) { if (Z_TYPE_P(option) != IS_LONG) { - if (ZEND_CALL_USES_STRICT_TYPES(EG(current_execute_data))) { - zend_type_error("Option \"comp_method\" must be of type int, %s given", - zend_zval_type_name(option)); - return false; - } php_error_docref(NULL, E_WARNING, "Option \"comp_method\" must be of type int, %s given", zend_zval_type_name(option)); } @@ -375,11 +365,6 @@ static bool php_zip_parse_options(HashTable *options, zip_options *opts) if ((option = zend_hash_str_find(options, "comp_flags", sizeof("comp_flags") - 1)) != NULL) { if (Z_TYPE_P(option) != IS_LONG) { - if (ZEND_CALL_USES_STRICT_TYPES(EG(current_execute_data))) { - zend_type_error("Option \"comp_flags\" must be of type int, %s given", - zend_zval_type_name(option)); - return false; - } php_error_docref(NULL, E_WARNING, "Option \"comp_flags\" must be of type int, %s given", zend_zval_type_name(option)); } @@ -390,11 +375,6 @@ static bool php_zip_parse_options(HashTable *options, zip_options *opts) #ifdef HAVE_ENCRYPTION if ((option = zend_hash_str_find(options, "enc_method", sizeof("enc_method") - 1)) != NULL) { if (Z_TYPE_P(option) != IS_LONG) { - if (ZEND_CALL_USES_STRICT_TYPES(EG(current_execute_data))) { - zend_type_error("Option \"enc_method\" must be of type int, %s given", - zend_zval_type_name(option)); - return false; - } php_error_docref(NULL, E_WARNING, "Option \"enc_method\" must be of type int, %s given", zend_zval_type_name(option)); } From f7e01fac84f816e02ec9d10402343d1f164185f5 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Mon, 17 Aug 2020 14:30:13 +0200 Subject: [PATCH 7/8] Drop TODO comments --- ext/zip/php_zip.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/ext/zip/php_zip.c b/ext/zip/php_zip.c index 291b86b21a4eb..f36a7f6c6b141 100644 --- a/ext/zip/php_zip.c +++ b/ext/zip/php_zip.c @@ -1449,7 +1449,6 @@ PHP_METHOD(ZipArchive, open) if (ze_obj->za) { /* we already have an opened zip, free it */ if (zip_close(ze_obj->za) != 0) { - // TODO Check this can be reached? php_error_docref(NULL, E_WARNING, "Empty string as source"); efree(resolved_path); RETURN_FALSE; @@ -2092,7 +2091,6 @@ PHP_METHOD(ZipArchive, setCommentName) idx = zip_name_locate(intern, name, 0); if (idx < 0) { - // TODO Warning? RETURN_FALSE; } PHP_ZIP_SET_FILE_COMMENT(intern, idx, comment, comment_len); @@ -2116,7 +2114,6 @@ PHP_METHOD(ZipArchive, setCommentIndex) ZIP_FROM_OBJECT(intern, self); - // TODO Check for Index value? if (comment_len > 0xffff) { zend_argument_value_error(2, "must be less than 65535 bytes"); RETURN_THROWS(); @@ -2153,7 +2150,7 @@ PHP_METHOD(ZipArchive, setExternalAttributesName) } idx = zip_name_locate(intern, name, 0); - // TODO Warning? + if (idx < 0) { RETURN_FALSE; } @@ -2214,7 +2211,7 @@ PHP_METHOD(ZipArchive, getExternalAttributesName) } idx = zip_name_locate(intern, name, 0); - // TODO Warning? + if (idx < 0) { RETURN_FALSE; } @@ -2281,7 +2278,7 @@ PHP_METHOD(ZipArchive, setEncryptionName) } idx = zip_name_locate(intern, name, 0); - // TODO Warning? + if (idx < 0) { RETURN_FALSE; } @@ -2342,7 +2339,7 @@ PHP_METHOD(ZipArchive, getCommentName) } idx = zip_name_locate(intern, name, 0); - // TODO Warning? + if (idx < 0) { RETURN_FALSE; } @@ -2398,7 +2395,7 @@ PHP_METHOD(ZipArchive, setCompressionName) } idx = zip_name_locate(intern, name, 0); - // TODO Warning? + if (idx < 0) { RETURN_FALSE; } @@ -2458,7 +2455,7 @@ PHP_METHOD(ZipArchive, setMtimeName) } idx = zip_name_locate(intern, name, 0); - // TODO Warning? + if (idx < 0) { RETURN_FALSE; } @@ -2560,7 +2557,6 @@ PHP_METHOD(ZipArchive, renameIndex) RETURN_THROWS(); } - // TODO Warning? if (index < 0) { RETURN_FALSE; } From 3364f8b7759b42caf713bb051563024e9be68818 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Mon, 17 Aug 2020 14:30:50 +0200 Subject: [PATCH 8/8] Revert int to bool return type change --- ext/zip/php_zip.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/ext/zip/php_zip.c b/ext/zip/php_zip.c index f36a7f6c6b141..43c2da9af6660 100644 --- a/ext/zip/php_zip.c +++ b/ext/zip/php_zip.c @@ -335,7 +335,7 @@ typedef struct { #endif } zip_options; -static bool php_zip_parse_options(HashTable *options, zip_options *opts) +static int php_zip_parse_options(HashTable *options, zip_options *opts) /* {{{ */ { zval *option; @@ -384,7 +384,7 @@ static bool php_zip_parse_options(HashTable *options, zip_options *opts) if (Z_TYPE_P(option) != IS_STRING) { zend_type_error("Option \"enc_password\" must be of type string, %s given", zend_zval_type_name(option)); - return false; + return -1; } opts->enc_password = Z_STRVAL_P(option); } @@ -395,17 +395,17 @@ static bool php_zip_parse_options(HashTable *options, zip_options *opts) if (Z_TYPE_P(option) != IS_STRING) { zend_type_error("Option \"remove_path\" must be of type string, %s given", zend_zval_type_name(option)); - return false; + return -1; } if (Z_STRLEN_P(option) == 0) { zend_value_error("Option \"remove_path\" cannot be empty"); - return false; + return -1; } if (Z_STRLEN_P(option) >= MAXPATHLEN) { zend_value_error("Option \"remove_path\" must be less than %d bytes", MAXPATHLEN - 1); - return false; + return -1; } opts->remove_path_len = Z_STRLEN_P(option); opts->remove_path = Z_STRVAL_P(option); @@ -415,17 +415,17 @@ static bool php_zip_parse_options(HashTable *options, zip_options *opts) if (Z_TYPE_P(option) != IS_STRING) { zend_type_error("Option \"add_path\" must be of type string, %s given", zend_zval_type_name(option)); - return false; + return -1; } if (Z_STRLEN_P(option) == 0) { zend_value_error("Option \"add_path\" cannot be empty"); - return false; + return -1; } if (Z_STRLEN_P(option) >= MAXPATHLEN) { zend_value_error("Option \"add_path\" must be less than %d bytes", MAXPATHLEN - 1); - return false; + return -1; } opts->add_path_len = Z_STRLEN_P(option); opts->add_path = Z_STRVAL_P(option); @@ -435,12 +435,12 @@ static bool php_zip_parse_options(HashTable *options, zip_options *opts) if (Z_TYPE_P(option) != IS_LONG) { zend_type_error("Option \"flags\" must be of type int, %s given", zend_zval_type_name(option)); - return false; + return -1; } opts->flags = Z_LVAL_P(option); } - return true; + return 1; } /* }}} */ @@ -1700,7 +1700,7 @@ static void php_zip_add_from_pattern(INTERNAL_FUNCTION_PARAMETERS, int type) /* zend_argument_value_error(1, "cannot be empty"); RETURN_THROWS(); } - if (options && zend_hash_num_elements(options) > 0 && (php_zip_parse_options(options, &opts) == false)) { + if (options && zend_hash_num_elements(options) > 0 && (php_zip_parse_options(options, &opts) < 0)) { RETURN_THROWS(); }