Fix GH-22649: avoid ZipArchive comment reset crash

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)
 

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, &current_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) {
@@ -2235,7 +2245,9 @@ PHP_METHOD(ZipArchive, setCommentIndex)
 	}
 
 	PHP_ZIP_STAT_INDEX(intern, index, 0, sb);
-	PHP_ZIP_SET_FILE_COMMENT(intern, index, comment, comment_len);
+	idx = (zip_uint64_t) index;
+
+	RETURN_BOOL(php_zip_set_file_comment(intern, idx, comment, comment_len));
 }
 /* }}} */
 

ext/zip/tests/gh22649.phpt

@@ -0,0 +1,55 @@
+--TEST--
+GH-22649 (setCommentName/Index after addFromString should not segfault)
+--EXTENSIONS--
+zip
+--FILE--
+<?php
+$file = __DIR__ . '/gh22649.zip';
+
+@unlink($file);
+
+$zip = new ZipArchive;
+if (!$zip->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--
+<?php
+@unlink(__DIR__ . '/gh22649.zip');
+?>