Skip to content

Commit ac7fddf

Browse files
devnexendevnexen
authored
ext/zip: further micro optimisations. (#20362)
- earlier return on invalid inputs from some ZipArchive methods. - apply comp_flags type check for setCompressionName()/setCompressionIndex(). and throw exception for these - add comp_flags check when passed as userland array option. - adding a wrapper for zip_file_set_encryption workaround.
1 parent 9c33091 commit ac7fddf

File tree

4 files changed

+130
-51
lines changed

4 files changed

+130
-51
lines changed

‎ext/zip/php_zip.c‎

Lines changed: 74 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,17 @@ static int le_zip_entry;
7474
# define add_ascii_assoc_string add_assoc_string
7575
# define add_ascii_assoc_long add_assoc_long
7676

77+
static bool php_zip_file_set_encryption(struct zip *intern, zend_long index, zend_long method, char *password) {
78+
// FIXME: is a workaround to reset/free the password in case of consecutive calls.
79+
// when libzip 1.11.5 is available, we can save this call in this case.
80+
if (UNEXPECTED(zip_file_set_encryption(intern, (zip_uint64_t)index, ZIP_EM_NONE, NULL) < 0)) {
81+
php_error_docref(NULL, E_WARNING, "password reset failed");
82+
return false;
83+
}
84+
85+
return (zip_file_set_encryption(intern, (zip_uint64_t)index, (zip_uint16_t)method, password) == 0);
86+
}
87+
7788
/* Flatten a path by making a relative path (to .)*/
7889
static char * php_zip_make_relative_path(char *path, size_t path_len) /* {{{ */
7990
{
@@ -367,14 +378,22 @@ static zend_result php_zip_parse_options(HashTable *options, zip_options *opts)
367378
php_error_docref(NULL, E_WARNING, "Option \"comp_method\" must be of type int, %s given",
368379
zend_zval_value_name(option));
369380
}
370-
opts->comp_method = zval_get_long(option);
381+
zend_long comp_method = zval_get_long(option);
382+
if (comp_method < 0 || comp_method > INT_MAX) {
383+
php_error_docref(NULL, E_WARNING, "Option \"comp_method\" must be between 0 and %d", INT_MAX);
384+
}
385+
opts->comp_method = (zip_int32_t)comp_method;
371386

372387
if ((option = zend_hash_str_find(options, "comp_flags", sizeof("comp_flags") - 1)) != NULL) {
373388
if (Z_TYPE_P(option) != IS_LONG) {
374389
php_error_docref(NULL, E_WARNING, "Option \"comp_flags\" must be of type int, %s given",
375390
zend_zval_value_name(option));
376391
}
377-
opts->comp_flags = zval_get_long(option);
392+
zend_long comp_flags = zval_get_long(option);
393+
if (comp_flags < 0 || comp_flags > USHRT_MAX) {
394+
php_error_docref(NULL, E_WARNING, "Option \"comp_flags\" must be between 0 and %u", USHRT_MAX);
395+
}
396+
opts->comp_flags = (zip_uint32_t)comp_flags;
378397
}
379398
}
380399

@@ -1752,12 +1771,7 @@ static void php_zip_add_from_pattern(INTERNAL_FUNCTION_PARAMETERS, int type) /*
17521771
}
17531772
#ifdef HAVE_ENCRYPTION
17541773
if (opts.enc_method >= 0) {
1755-
if (UNEXPECTED(zip_file_set_encryption(ze_obj->za, ze_obj->last_id, ZIP_EM_NONE, NULL) < 0)) {
1756-
zend_array_destroy(Z_ARR_P(return_value));
1757-
php_error_docref(NULL, E_WARNING, "password reset failed");
1758-
RETURN_FALSE;
1759-
}
1760-
if (zip_file_set_encryption(ze_obj->za, ze_obj->last_id, opts.enc_method, opts.enc_password)) {
1774+
if (!php_zip_file_set_encryption(ze_obj->za, ze_obj->last_id, opts.enc_method, opts.enc_password)) {
17611775
zend_array_destroy(Z_ARR_P(return_value));
17621776
RETURN_FALSE;
17631777
}
@@ -2279,15 +2293,7 @@ PHP_METHOD(ZipArchive, setEncryptionName)
22792293
RETURN_FALSE;
22802294
}
22812295

2282-
if (UNEXPECTED(zip_file_set_encryption(intern, idx, ZIP_EM_NONE, NULL) < 0)) {
2283-
php_error_docref(NULL, E_WARNING, "password reset failed");
2284-
RETURN_FALSE;
2285-
}
2286-
2287-
if (zip_file_set_encryption(intern, idx, (zip_uint16_t)method, password)) {
2288-
RETURN_FALSE;
2289-
}
2290-
RETURN_TRUE;
2296+
RETURN_BOOL(php_zip_file_set_encryption(intern, idx, method, password));
22912297
}
22922298
/* }}} */
22932299

@@ -2307,12 +2313,7 @@ PHP_METHOD(ZipArchive, setEncryptionIndex)
23072313

23082314
ZIP_FROM_OBJECT(intern, self);
23092315

2310-
if (UNEXPECTED(zip_file_set_encryption(intern, index, ZIP_EM_NONE, NULL) < 0)) {
2311-
php_error_docref(NULL, E_WARNING, "password reset failed");
2312-
RETURN_FALSE;
2313-
}
2314-
2315-
RETURN_BOOL(zip_file_set_encryption(intern, index, (zip_uint16_t)method, password) == 0);
2316+
RETURN_BOOL(php_zip_file_set_encryption(intern, index, method, password));
23162317
}
23172318
/* }}} */
23182319
#endif
@@ -2390,13 +2391,23 @@ PHP_METHOD(ZipArchive, setCompressionName)
23902391
RETURN_THROWS();
23912392
}
23922393

2393-
ZIP_FROM_OBJECT(intern, this);
2394-
23952394
if (name_len == 0) {
23962395
zend_argument_must_not_be_empty_error(1);
23972396
RETURN_THROWS();
23982397
}
23992398

2399+
if (comp_method < -1 || comp_method > INT_MAX) {
2400+
zend_argument_value_error(2, "must be between -1 and %d", INT_MAX);
2401+
RETURN_THROWS();
2402+
}
2403+
2404+
if (comp_flags < 0 || comp_flags > USHRT_MAX) {
2405+
// comp_flags is cast down accordingly in libzip, zip_entry_t compression_level is of zip_uint16_t
2406+
zend_argument_value_error(3, "must be between 0 and %u", USHRT_MAX);
2407+
RETURN_THROWS();
2408+
}
2409+
2410+
ZIP_FROM_OBJECT(intern, this);
24002411
idx = zip_name_locate(intern, name, 0);
24012412

24022413
if (idx < 0) {
@@ -2421,6 +2432,21 @@ PHP_METHOD(ZipArchive, setCompressionIndex)
24212432
RETURN_THROWS();
24222433
}
24232434

2435+
if (index < 0) {
2436+
RETURN_FALSE;
2437+
}
2438+
2439+
if (comp_method < -1 || comp_method > INT_MAX) {
2440+
zend_argument_value_error(2, "must be between -1 and %d", INT_MAX);
2441+
RETURN_THROWS();
2442+
}
2443+
2444+
if (comp_flags < 0 || comp_flags > USHRT_MAX) {
2445+
// comp_flags is cast down accordingly in libzip, zip_entry_t compression_level is of zip_uint16_t
2446+
zend_argument_value_error(3, "must be between 0 and %u", USHRT_MAX);
2447+
RETURN_THROWS();
2448+
}
2449+
24242450
ZIP_FROM_OBJECT(intern, this);
24252451

24262452
RETURN_BOOL(zip_set_file_compression(intern, (zip_uint64_t)index,
@@ -2443,13 +2469,13 @@ PHP_METHOD(ZipArchive, setMtimeName)
24432469
RETURN_THROWS();
24442470
}
24452471

2446-
ZIP_FROM_OBJECT(intern, this);
2447-
24482472
if (name_len == 0) {
24492473
zend_argument_must_not_be_empty_error(1);
24502474
RETURN_THROWS();
24512475
}
24522476

2477+
ZIP_FROM_OBJECT(intern, this);
2478+
24532479
idx = zip_name_locate(intern, name, 0);
24542480

24552481
if (idx < 0) {
@@ -2515,17 +2541,15 @@ PHP_METHOD(ZipArchive, deleteName)
25152541
RETURN_THROWS();
25162542
}
25172543

2518-
ZIP_FROM_OBJECT(intern, self);
2519-
25202544
if (name_len < 1) {
25212545
RETURN_FALSE;
25222546
}
25232547

2548+
ZIP_FROM_OBJECT(intern, self);
2549+
25242550
PHP_ZIP_STAT_PATH(intern, name, name_len, 0, sb);
2525-
if (zip_delete(intern, sb.index)) {
2526-
RETURN_FALSE;
2527-
}
2528-
RETURN_TRUE;
2551+
2552+
RETURN_BOOL(zip_delete(intern, sb.index) == 0);
25292553
}
25302554
/* }}} */
25312555

@@ -2546,13 +2570,13 @@ PHP_METHOD(ZipArchive, renameIndex)
25462570
RETURN_FALSE;
25472571
}
25482572

2549-
ZIP_FROM_OBJECT(intern, self);
2550-
25512573
if (new_name_len == 0) {
25522574
zend_argument_must_not_be_empty_error(2);
25532575
RETURN_THROWS();
25542576
}
25552577

2578+
ZIP_FROM_OBJECT(intern, self);
2579+
25562580
RETURN_BOOL(zip_file_rename(intern, index, (const char *)new_name, 0) == 0);
25572581
}
25582582
/* }}} */
@@ -2594,12 +2618,12 @@ PHP_METHOD(ZipArchive, unchangeIndex)
25942618
RETURN_THROWS();
25952619
}
25962620

2597-
ZIP_FROM_OBJECT(intern, self);
2598-
25992621
if (index < 0) {
26002622
RETURN_FALSE;
26012623
}
26022624

2625+
ZIP_FROM_OBJECT(intern, self);
2626+
26032627
RETURN_BOOL(zip_unchange(intern, index) == 0);
26042628
}
26052629
/* }}} */
@@ -2617,12 +2641,12 @@ PHP_METHOD(ZipArchive, unchangeName)
26172641
RETURN_THROWS();
26182642
}
26192643

2620-
ZIP_FROM_OBJECT(intern, self);
2621-
26222644
if (name_len < 1) {
26232645
RETURN_FALSE;
26242646
}
26252647

2648+
ZIP_FROM_OBJECT(intern, self);
2649+
26262650
PHP_ZIP_STAT_PATH(intern, name, name_len, 0, sb);
26272651

26282652
RETURN_BOOL(zip_unchange(intern, sb.index) == 0);
@@ -2686,8 +2710,6 @@ PHP_METHOD(ZipArchive, extractTo)
26862710
Z_PARAM_ARRAY_HT_OR_STR_OR_NULL(files_ht, files_str)
26872711
ZEND_PARSE_PARAMETERS_END();
26882712

2689-
ZIP_FROM_OBJECT(intern, self);
2690-
26912713
if (pathto_len < 1) {
26922714
RETURN_FALSE;
26932715
}
@@ -2700,6 +2722,7 @@ PHP_METHOD(ZipArchive, extractTo)
27002722
}
27012723

27022724
uint32_t nelems, i;
2725+
ZIP_FROM_OBJECT(intern, self);
27032726

27042727
if (files_str) {
27052728
if (!php_zip_extract_file(intern, pathto, ZSTR_VAL(files_str), ZSTR_LEN(files_str), -1)) {
@@ -2796,7 +2819,7 @@ static void php_zip_get_from(INTERNAL_FUNCTION_PARAMETERS, int type) /* {{{ */
27962819
RETURN_FALSE;
27972820
}
27982821

2799-
buffer = zend_string_safe_alloc(1, len, 0, 0);
2822+
buffer = zend_string_safe_alloc(1, len, 0, false);
28002823
n = zip_fread(zf, ZSTR_VAL(buffer), ZSTR_LEN(buffer));
28012824
if (n < 1) {
28022825
zend_string_efree(buffer);
@@ -3005,6 +3028,10 @@ PHP_METHOD(ZipArchive, isCompressionMethodSupported)
30053028
if (zend_parse_parameters(ZEND_NUM_ARGS(), "l|b", &method, &enc) == FAILURE) {
30063029
return;
30073030
}
3031+
if (method < -1 || method > INT_MAX) {
3032+
zend_argument_value_error(1, "must be between -1 and %d", INT_MAX);
3033+
RETURN_THROWS();
3034+
}
30083035
RETVAL_BOOL(zip_compression_method_supported((zip_int32_t)method, enc));
30093036
}
30103037
/* }}} */
@@ -3018,7 +3045,11 @@ PHP_METHOD(ZipArchive, isEncryptionMethodSupported)
30183045
if (zend_parse_parameters(ZEND_NUM_ARGS(), "l|b", &method, &enc) == FAILURE) {
30193046
return;
30203047
}
3021-
RETVAL_BOOL(zip_encryption_method_supported((zip_uint16_t)method, enc));
3048+
if (method < 0 || method > USHRT_MAX) {
3049+
zend_argument_value_error(1, "must be between 0 and %u", USHRT_MAX);
3050+
RETURN_THROWS();
3051+
}
3052+
RETURN_BOOL(zip_encryption_method_supported((zip_uint16_t)method, enc));
30223053
}
30233054
/* }}} */
30243055
#endif

‎ext/zip/tests/oo_addglob2.phpt‎

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,10 +33,26 @@ if (!$zip->addGlob($dirname . 'foo.*', GLOB_BRACE, $options)) {
3333
$options = [
3434
'remove_all_path' => true,
3535
'comp_method' => ZipArchive::CM_STORE,
36-
'comp_flags' => 5,
36+
'comp_flags' => PHP_INT_MIN,
3737
'enc_method' => ZipArchive::EM_AES_256,
3838
'enc_password' => 'secret',
3939
];
40+
41+
try {
42+
$zip->addGlob($dirname. 'bar.*', GLOB_BRACE, $options);
43+
} catch (\ValueError $e) {
44+
echo $e->getMessage(), PHP_EOL;
45+
}
46+
47+
$options['comp_flags'] = 65536;
48+
49+
try {
50+
$zip->addGlob($dirname. 'bar.*', GLOB_BRACE, $options);
51+
} catch (\ValueError $e) {
52+
echo $e->getMessage(), PHP_EOL;
53+
}
54+
55+
$options['comp_flags'] = 5;
4056
if (!$zip->addGlob($dirname . 'bar.*', GLOB_BRACE, $options)) {
4157
echo "failed 2\n";
4258
}
@@ -61,6 +77,10 @@ $dirname = __DIR__ . '/';
6177
include $dirname . 'utils.inc';
6278
rmdir_rf(__DIR__ . '/__tmp_oo_addglob2/');
6379
?>
64-
--EXPECT--
80+
--EXPECTF--
81+
82+
Warning: ZipArchive::addGlob(): Option "comp_flags" must be between 0 and 65535 in %s on line %d
83+
84+
Warning: ZipArchive::addGlob(): Option "comp_flags" must be between 0 and 65535 in %s on line %d
6585
0: foo.txt, comp=8, enc=0
6686
1: bar.txt, comp=0, enc=259

‎ext/zip/tests/oo_setcompression.phpt‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,30 @@ var_dump($zip->setCompressionName('entry2.txt', ZipArchive::CM_DEFAULT));
2828
var_dump($zip->setCompressionName('dir/entry3.txt', ZipArchive::CM_STORE));
2929
var_dump($zip->setCompressionName('entry4.txt', ZipArchive::CM_DEFLATE));
3030

31+
try {
32+
$zip->setCompressionName('entry5.txt', PHP_INT_MIN);
33+
} catch (\ValueError $e) {
34+
echo $e->getMessage(), PHP_EOL;
35+
}
36+
37+
try {
38+
$zip->setCompressionName('entry5.txt', PHP_INT_MAX);
39+
} catch (\ValueError $e) {
40+
echo $e->getMessage(), PHP_EOL;
41+
}
42+
43+
try {
44+
$zip->setCompressionIndex(4, PHP_INT_MIN);
45+
} catch (\ValueError $e) {
46+
echo $e->getMessage(), PHP_EOL;
47+
}
48+
49+
try {
50+
$zip->setCompressionIndex(4, PHP_INT_MAX);
51+
} catch (\ValueError $e) {
52+
echo $e->getMessage(), PHP_EOL;
53+
}
54+
3155
var_dump($zip->setCompressionIndex(4, ZipArchive::CM_STORE));
3256
var_dump($zip->setCompressionIndex(5, ZipArchive::CM_DEFLATE));
3357
var_dump($zip->setCompressionIndex(6, ZipArchive::CM_DEFAULT));
@@ -57,6 +81,10 @@ unlink($tmpfile);
5781
bool(true)
5882
bool(true)
5983
bool(true)
84+
ZipArchive::setCompressionName(): Argument #2 ($method) must be between -1 and %d
85+
ZipArchive::setCompressionName(): Argument #2 ($method) must be between -1 and %d
86+
ZipArchive::setCompressionIndex(): Argument #2 ($method) must be between -1 and %d
87+
ZipArchive::setCompressionIndex(): Argument #2 ($method) must be between -1 and %d
6088
bool(true)
6189
bool(true)
6290
bool(true)

0 commit comments

Comments
 (0)