From a1bb5fc03af896d4ca170fb0a59ebf205dcba235 Mon Sep 17 00:00:00 2001 From: Weilin Du Date: Thu, 9 Jul 2026 18:15:38 +0800 Subject: [PATCH 1/2] Fix GH-22649: avoid ZipArchive comment reset crash --- NEWS | 3 +++ ext/zip/php_zip.c | 46 ++++++++++++++++++++----------- ext/zip/tests/gh22649.phpt | 55 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 89 insertions(+), 15 deletions(-) create mode 100644 ext/zip/tests/gh22649.phpt diff --git a/NEWS b/NEWS index cdd6a48a4874..574478e6c96c 100644 --- a/NEWS +++ b/NEWS @@ -78,6 +78,9 @@ PHP NEWS when parsing an IFF chunk with a size of INT_MAX. (David Carlier, Weilin Du) - Zip: + . Fixed bug GH-22649 (ZipArchive::setCommentName() and setCommentIndex() + could crash after overwriting an entry and resetting its inherited + unchanged comment). (Weilin Du) . Fixed bug GH-21705 (ZipArchive::getFromIndex() ignores ZipArchive::FL_UNCHANGED for deleted entries). (Weilin Du) diff --git a/ext/zip/php_zip.c b/ext/zip/php_zip.c index 16df403dcda5..520c965087c2 100644 --- a/ext/zip/php_zip.c +++ b/ext/zip/php_zip.c @@ -64,17 +64,26 @@ static int le_zip_entry; } /* }}} */ -/* {{{ PHP_ZIP_SET_FILE_COMMENT(za, index, comment, comment_len) */ -#define PHP_ZIP_SET_FILE_COMMENT(za, index, comment, comment_len) \ - if (comment_len == 0) { \ - /* Passing NULL remove the existing comment */ \ - if (zip_file_set_comment(za, index, NULL, 0, 0) < 0) { \ - RETURN_FALSE; \ - } \ - } else if (zip_file_set_comment(za, index, comment, comment_len, 0) < 0) { \ - RETURN_FALSE; \ - } \ - RETURN_TRUE; +/* {{{ php_zip_set_file_comment */ +static bool php_zip_set_file_comment(struct zip *za, zip_uint64_t index, const char *comment, size_t comment_len) +{ + zip_uint32_t current_comment_len; + const char *current_comment = zip_file_get_comment(za, index, ¤t_comment_len, ZIP_FL_ENC_RAW); + + /* Avoid a libzip use-after-free when resetting an unchanged inherited comment. */ + if (current_comment && current_comment_len == comment_len + && memcmp(current_comment, comment, comment_len) == 0) { + return true; + } + + if (comment_len == 0) { + /* Passing NULL removes the existing comment. */ + return zip_file_set_comment(za, index, NULL, 0, 0) == 0; + } + + ZEND_ASSERT(comment_len <= 0xffff); + return zip_file_set_comment(za, index, comment, (zip_uint16_t) comment_len, 0) == 0; +} /* }}} */ # define add_ascii_assoc_string add_assoc_string @@ -2185,7 +2194,7 @@ PHP_METHOD(ZipArchive, setCommentName) zval *self = ZEND_THIS; size_t comment_len, name_len; char * comment, *name; - int idx; + zip_int64_t idx; if (zend_parse_parameters(ZEND_NUM_ARGS(), "ss", &name, &name_len, &comment, &comment_len) == FAILURE) { @@ -2208,7 +2217,7 @@ PHP_METHOD(ZipArchive, setCommentName) if (idx < 0) { RETURN_FALSE; } - PHP_ZIP_SET_FILE_COMMENT(intern, idx, comment, comment_len); + RETURN_BOOL(php_zip_set_file_comment(intern, (zip_uint64_t) idx, comment, comment_len)); } /* }}} */ @@ -2221,6 +2230,7 @@ PHP_METHOD(ZipArchive, setCommentIndex) size_t comment_len; char * comment; struct zip_stat sb; + zip_uint64_t idx; if (zend_parse_parameters(ZEND_NUM_ARGS(), "ls", &index, &comment, &comment_len) == FAILURE) { @@ -2234,8 +2244,14 @@ PHP_METHOD(ZipArchive, setCommentIndex) RETURN_THROWS(); } - PHP_ZIP_STAT_INDEX(intern, index, 0, sb); - PHP_ZIP_SET_FILE_COMMENT(intern, index, comment, comment_len); + if (index < 0) { + RETURN_FALSE; + } + + idx = (zip_uint64_t) index; + + PHP_ZIP_STAT_INDEX(intern, idx, 0, sb); + RETURN_BOOL(php_zip_set_file_comment(intern, idx, comment, comment_len)); } /* }}} */ diff --git a/ext/zip/tests/gh22649.phpt b/ext/zip/tests/gh22649.phpt new file mode 100644 index 000000000000..d6d0a0928184 --- /dev/null +++ b/ext/zip/tests/gh22649.phpt @@ -0,0 +1,55 @@ +--TEST-- +GH-22649 (setCommentName/Index after addFromString should not segfault) +--EXTENSIONS-- +zip +--FILE-- +open($file, ZipArchive::CREATE)) { + exit('failed'); +} + +$zip->addFromString('dir/entry2d.txt', 'entry #2'); +var_dump($zip->setCommentName('dir/entry2d.txt', 'dir/entry2d.txt')); + +$zip->addFromString('dir/entry3d.txt', 'entry #3'); +var_dump($zip->setCommentIndex($zip->lastId, 'dir/entry3d.txt')); + +$zip->close(); + +if (!$zip->open($file, ZipArchive::CREATE)) { + exit('failed'); +} + +$zip->addFromString('dir/entry2d.txt', 'updated entry #2'); +var_dump($zip->setCommentName('dir/entry2d.txt', 'dir/entry2d.txt')); + +$zip->addFromString('dir/entry3d.txt', 'updated entry #3'); +var_dump($zip->setCommentIndex($zip->lastId, 'dir/entry3d.txt')); + +$zip->close(); + +if (!$zip->open($file)) { + exit('failed'); +} + +var_dump($zip->getCommentName('dir/entry2d.txt')); +var_dump($zip->getCommentName('dir/entry3d.txt')); + +$zip->close(); +?> +--EXPECT-- +bool(true) +bool(true) +bool(true) +bool(true) +string(15) "dir/entry2d.txt" +string(15) "dir/entry3d.txt" +--CLEAN-- + From 8aed4406e87ff1ab72af30dceb2b649b5922548c Mon Sep 17 00:00:00 2001 From: Weilin Du Date: Thu, 9 Jul 2026 18:23:18 +0800 Subject: [PATCH 2/2] remove unrelated logic --- ext/zip/php_zip.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/ext/zip/php_zip.c b/ext/zip/php_zip.c index 520c965087c2..ccc474bc715c 100644 --- a/ext/zip/php_zip.c +++ b/ext/zip/php_zip.c @@ -2244,13 +2244,9 @@ PHP_METHOD(ZipArchive, setCommentIndex) RETURN_THROWS(); } - if (index < 0) { - RETURN_FALSE; - } - + PHP_ZIP_STAT_INDEX(intern, index, 0, sb); idx = (zip_uint64_t) index; - PHP_ZIP_STAT_INDEX(intern, idx, 0, sb); RETURN_BOOL(php_zip_set_file_comment(intern, idx, comment, comment_len)); } /* }}} */