Skip to content

ext/zip: further micro optimisations. #20362

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Nov 16, 2025
Merged

ext/zip: further micro optimisations. #20362

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
37575e9
ext/zip: further micro optimisations.
devnexen Nov 2, 2025
39ac723
apply comp_flags type check for setCompressionName()/setCompressionIn…
devnexen Nov 2, 2025
480a1d8
add comp_flags check when passed as userland array option.
devnexen Nov 2, 2025
f45b893
updating test cases.
devnexen Nov 2, 2025
3e05fa4
adding a wrapper for zip_file_set_encryption workaround.
devnexen Nov 2, 2025
2cd1368
fix last change
devnexen Nov 2, 2025
c7d0dae
feedback
devnexen Nov 3, 2025
a6925a2
review
devnexen Nov 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
117 changes: 74 additions & 43 deletions ext/zip/php_zip.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,17 @@ static int le_zip_entry;
# define add_ascii_assoc_string add_assoc_string
# define add_ascii_assoc_long add_assoc_long

static bool php_zip_file_set_encryption(struct zip *intern, zend_long index, zend_long method, char *password) {
// FIXME: is a workaround to reset/free the password in case of consecutive calls.
// when libzip 1.11.5 is available, we can save this call in this case.
if (UNEXPECTED(zip_file_set_encryption(intern, (zip_uint64_t)index, ZIP_EM_NONE, NULL) < 0)) {
php_error_docref(NULL, E_WARNING, "password reset failed");
return false;
}

return (zip_file_set_encryption(intern, (zip_uint64_t)index, (zip_uint16_t)method, password) == 0);
}

/* Flatten a path by making a relative path (to .)*/
static char * php_zip_make_relative_path(char *path, size_t path_len) /* {{{ */
{
Expand Down Expand Up @@ -367,14 +378,22 @@ static zend_result php_zip_parse_options(HashTable *options, zip_options *opts)
php_error_docref(NULL, E_WARNING, "Option \"comp_method\" must be of type int, %s given",
zend_zval_value_name(option));
}
opts->comp_method = zval_get_long(option);
zend_long comp_method = zval_get_long(option);
if (comp_method < 0 || comp_method > INT_MAX) {
php_error_docref(NULL, E_WARNING, "Option \"comp_method\" must be between 0 and %d", INT_MAX);
}
opts->comp_method = (zip_int32_t)comp_method;

if ((option = zend_hash_str_find(options, "comp_flags", sizeof("comp_flags") - 1)) != NULL) {
if (Z_TYPE_P(option) != IS_LONG) {
php_error_docref(NULL, E_WARNING, "Option \"comp_flags\" must be of type int, %s given",
zend_zval_value_name(option));
}
opts->comp_flags = zval_get_long(option);
zend_long comp_flags = zval_get_long(option);
Copy link
Member

@Girgias Girgias Nov 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another follow-up I'd like to see is to use zval_try_get_long() :)

if (comp_flags < 0 || comp_flags > USHRT_MAX) {
php_error_docref(NULL, E_WARNING, "Option \"comp_flags\" must be between 0 and %u", USHRT_MAX);
}
opts->comp_flags = (zip_uint32_t)comp_flags;
}
}

Expand Down Expand Up @@ -1752,12 +1771,7 @@ static void php_zip_add_from_pattern(INTERNAL_FUNCTION_PARAMETERS, int type) /*
}
#ifdef HAVE_ENCRYPTION
if (opts.enc_method >= 0) {
if (UNEXPECTED(zip_file_set_encryption(ze_obj->za, ze_obj->last_id, ZIP_EM_NONE, NULL) < 0)) {
zend_array_destroy(Z_ARR_P(return_value));
php_error_docref(NULL, E_WARNING, "password reset failed");
RETURN_FALSE;
}
if (zip_file_set_encryption(ze_obj->za, ze_obj->last_id, opts.enc_method, opts.enc_password)) {
if (!php_zip_file_set_encryption(ze_obj->za, ze_obj->last_id, opts.enc_method, opts.enc_password)) {
zend_array_destroy(Z_ARR_P(return_value));
RETURN_FALSE;
}
Expand Down Expand Up @@ -2280,15 +2294,7 @@ PHP_METHOD(ZipArchive, setEncryptionName)
RETURN_FALSE;
}

if (UNEXPECTED(zip_file_set_encryption(intern, idx, ZIP_EM_NONE, NULL) < 0)) {
php_error_docref(NULL, E_WARNING, "password reset failed");
RETURN_FALSE;
}

if (zip_file_set_encryption(intern, idx, (zip_uint16_t)method, password)) {
RETURN_FALSE;
}
RETURN_TRUE;
RETURN_BOOL(php_zip_file_set_encryption(intern, idx, method, password));
}
/* }}} */

Expand All @@ -2308,12 +2314,7 @@ PHP_METHOD(ZipArchive, setEncryptionIndex)

ZIP_FROM_OBJECT(intern, self);

if (UNEXPECTED(zip_file_set_encryption(intern, index, ZIP_EM_NONE, NULL) < 0)) {
php_error_docref(NULL, E_WARNING, "password reset failed");
RETURN_FALSE;
}

RETURN_BOOL(zip_file_set_encryption(intern, index, (zip_uint16_t)method, password) == 0);
RETURN_BOOL(php_zip_file_set_encryption(intern, index, method, password));
}
/* }}} */
#endif
Expand Down Expand Up @@ -2391,13 +2392,23 @@ PHP_METHOD(ZipArchive, setCompressionName)
RETURN_THROWS();
}

ZIP_FROM_OBJECT(intern, this);

if (name_len == 0) {
zend_argument_must_not_be_empty_error(1);
RETURN_THROWS();
}

if (comp_method < -1 || comp_method > INT_MAX) {
zend_argument_value_error(2, "must be between -1 and %d", INT_MAX);
RETURN_THROWS();
}

if (comp_flags < 0 || comp_flags > USHRT_MAX) {
// comp_flags is cast down accordingly in libzip, zip_entry_t compression_level is of zip_uint16_t
zend_argument_value_error(3, "must be between 0 and %u", USHRT_MAX);
RETURN_THROWS();
}

ZIP_FROM_OBJECT(intern, this);
idx = zip_name_locate(intern, name, 0);

if (idx < 0) {
Expand All @@ -2422,6 +2433,21 @@ PHP_METHOD(ZipArchive, setCompressionIndex)
RETURN_THROWS();
}

if (index < 0) {
RETURN_FALSE;
}

if (comp_method < -1 || comp_method > INT_MAX) {
zend_argument_value_error(2, "must be between -1 and %d", INT_MAX);
RETURN_THROWS();
}

if (comp_flags < 0 || comp_flags > USHRT_MAX) {
// comp_flags is cast down accordingly in libzip, zip_entry_t compression_level is of zip_uint16_t
zend_argument_value_error(3, "must be between 0 and %u", USHRT_MAX);
RETURN_THROWS();
}

ZIP_FROM_OBJECT(intern, this);

RETURN_BOOL(zip_set_file_compression(intern, (zip_uint64_t)index,
Expand All @@ -2444,13 +2470,13 @@ PHP_METHOD(ZipArchive, setMtimeName)
RETURN_THROWS();
}

ZIP_FROM_OBJECT(intern, this);

if (name_len == 0) {
zend_argument_must_not_be_empty_error(1);
RETURN_THROWS();
}

ZIP_FROM_OBJECT(intern, this);

idx = zip_name_locate(intern, name, 0);

if (idx < 0) {
Expand Down Expand Up @@ -2516,17 +2542,15 @@ PHP_METHOD(ZipArchive, deleteName)
RETURN_THROWS();
}

ZIP_FROM_OBJECT(intern, self);

if (name_len < 1) {
RETURN_FALSE;
}

ZIP_FROM_OBJECT(intern, self);

PHP_ZIP_STAT_PATH(intern, name, name_len, 0, sb);
if (zip_delete(intern, sb.index)) {
RETURN_FALSE;
}
RETURN_TRUE;

RETURN_BOOL(zip_delete(intern, sb.index) == 0);
}
/* }}} */

Expand All @@ -2547,13 +2571,13 @@ PHP_METHOD(ZipArchive, renameIndex)
RETURN_FALSE;
}

ZIP_FROM_OBJECT(intern, self);

if (new_name_len == 0) {
zend_argument_must_not_be_empty_error(2);
RETURN_THROWS();
}

ZIP_FROM_OBJECT(intern, self);

RETURN_BOOL(zip_file_rename(intern, index, (const char *)new_name, 0) == 0);
}
/* }}} */
Expand Down Expand Up @@ -2595,12 +2619,12 @@ PHP_METHOD(ZipArchive, unchangeIndex)
RETURN_THROWS();
}

ZIP_FROM_OBJECT(intern, self);

if (index < 0) {
RETURN_FALSE;
}
Comment on lines 2622 to 2624
Copy link
Member

@Girgias Girgias Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it might make sense to make those sorts of conditions a warning/value error in the future?

Copy link
Member Author

@devnexen devnexen Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I kept "api compatible" for now on purpose.


ZIP_FROM_OBJECT(intern, self);

RETURN_BOOL(zip_unchange(intern, index) == 0);
}
/* }}} */
Expand All @@ -2618,12 +2642,12 @@ PHP_METHOD(ZipArchive, unchangeName)
RETURN_THROWS();
}

ZIP_FROM_OBJECT(intern, self);

if (name_len < 1) {
RETURN_FALSE;
}

ZIP_FROM_OBJECT(intern, self);

PHP_ZIP_STAT_PATH(intern, name, name_len, 0, sb);

RETURN_BOOL(zip_unchange(intern, sb.index) == 0);
Expand Down Expand Up @@ -2687,8 +2711,6 @@ PHP_METHOD(ZipArchive, extractTo)
Z_PARAM_ARRAY_HT_OR_STR_OR_NULL(files_ht, files_str)
ZEND_PARSE_PARAMETERS_END();

ZIP_FROM_OBJECT(intern, self);

if (pathto_len < 1) {
RETURN_FALSE;
}
Expand All @@ -2701,6 +2723,7 @@ PHP_METHOD(ZipArchive, extractTo)
}

uint32_t nelems, i;
ZIP_FROM_OBJECT(intern, self);

if (files_str) {
if (!php_zip_extract_file(intern, pathto, ZSTR_VAL(files_str), ZSTR_LEN(files_str), -1)) {
Expand Down Expand Up @@ -2797,7 +2820,7 @@ static void php_zip_get_from(INTERNAL_FUNCTION_PARAMETERS, int type) /* {{{ */
RETURN_FALSE;
}

buffer = zend_string_safe_alloc(1, len, 0, 0);
buffer = zend_string_safe_alloc(1, len, 0, false);
n = zip_fread(zf, ZSTR_VAL(buffer), ZSTR_LEN(buffer));
if (n < 1) {
zend_string_efree(buffer);
Expand Down Expand Up @@ -3006,6 +3029,10 @@ PHP_METHOD(ZipArchive, isCompressionMethodSupported)
if (zend_parse_parameters(ZEND_NUM_ARGS(), "l|b", &method, &enc) == FAILURE) {
return;
}
if (method < -1 || method > INT_MAX) {
zend_argument_value_error(1, "must be between -1 and %d", INT_MAX);
RETURN_THROWS();
}
RETVAL_BOOL(zip_compression_method_supported((zip_int32_t)method, enc));
}
/* }}} */
Expand All @@ -3019,7 +3046,11 @@ PHP_METHOD(ZipArchive, isEncryptionMethodSupported)
if (zend_parse_parameters(ZEND_NUM_ARGS(), "l|b", &method, &enc) == FAILURE) {
return;
}
RETVAL_BOOL(zip_encryption_method_supported((zip_uint16_t)method, enc));
if (method < 0 || method > USHRT_MAX) {
zend_argument_value_error(1, "must be between 0 and %u", USHRT_MAX);
RETURN_THROWS();
}
RETURN_BOOL(zip_encryption_method_supported((zip_uint16_t)method, enc));
}
/* }}} */
#endif
Expand Down
24 changes: 22 additions & 2 deletions ext/zip/tests/oo_addglob2.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,26 @@ if (!$zip->addGlob($dirname . 'foo.*', GLOB_BRACE, $options)) {
$options = [
'remove_all_path' => true,
'comp_method' => ZipArchive::CM_STORE,
'comp_flags' => 5,
'comp_flags' => PHP_INT_MIN,
'enc_method' => ZipArchive::EM_AES_256,
'enc_password' => 'secret',
];

try {
$zip->addGlob($dirname. 'bar.*', GLOB_BRACE, $options);
} catch (\ValueError $e) {
echo $e->getMessage(), PHP_EOL;
}

$options['comp_flags'] = 65536;

try {
$zip->addGlob($dirname. 'bar.*', GLOB_BRACE, $options);
} catch (\ValueError $e) {
echo $e->getMessage(), PHP_EOL;
}

$options['comp_flags'] = 5;
if (!$zip->addGlob($dirname . 'bar.*', GLOB_BRACE, $options)) {
echo "failed 2\n";
}
Expand All @@ -61,6 +77,10 @@ $dirname = __DIR__ . '/';
include $dirname . 'utils.inc';
rmdir_rf(__DIR__ . '/__tmp_oo_addglob2/');
?>
--EXPECT--
--EXPECTF--

Warning: ZipArchive::addGlob(): Option "comp_flags" must be between 0 and 65535 in %s on line %d

Warning: ZipArchive::addGlob(): Option "comp_flags" must be between 0 and 65535 in %s on line %d
0: foo.txt, comp=8, enc=0
1: bar.txt, comp=0, enc=259
28 changes: 28 additions & 0 deletions ext/zip/tests/oo_setcompression.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,30 @@ var_dump($zip->setCompressionName('entry2.txt', ZipArchive::CM_DEFAULT));
var_dump($zip->setCompressionName('dir/entry3.txt', ZipArchive::CM_STORE));
var_dump($zip->setCompressionName('entry4.txt', ZipArchive::CM_DEFLATE));

try {
$zip->setCompressionName('entry5.txt', PHP_INT_MIN);
} catch (\ValueError $e) {
echo $e->getMessage(), PHP_EOL;
}

try {
$zip->setCompressionName('entry5.txt', PHP_INT_MAX);
} catch (\ValueError $e) {
echo $e->getMessage(), PHP_EOL;
}

try {
$zip->setCompressionIndex(4, PHP_INT_MIN);
} catch (\ValueError $e) {
echo $e->getMessage(), PHP_EOL;
}

try {
$zip->setCompressionIndex(4, PHP_INT_MAX);
} catch (\ValueError $e) {
echo $e->getMessage(), PHP_EOL;
}

var_dump($zip->setCompressionIndex(4, ZipArchive::CM_STORE));
var_dump($zip->setCompressionIndex(5, ZipArchive::CM_DEFLATE));
var_dump($zip->setCompressionIndex(6, ZipArchive::CM_DEFAULT));
Expand Down Expand Up @@ -57,6 +81,10 @@ unlink($tmpfile);
bool(true)
bool(true)
bool(true)
ZipArchive::setCompressionName(): Argument #2 ($method) must be between -1 and %d
ZipArchive::setCompressionName(): Argument #2 ($method) must be between -1 and %d
ZipArchive::setCompressionIndex(): Argument #2 ($method) must be between -1 and %d
ZipArchive::setCompressionIndex(): Argument #2 ($method) must be between -1 and %d
bool(true)
bool(true)
bool(true)
Expand Down
Loading