Skip to content

Fix GH-22885: SEGV when setting the comment of a modified zip entry#22892

Open
khaledalam wants to merge 1 commit into
php:PHP-8.4from
khaledalam:fix-gh22885-zip-comment-segv
Open

Fix GH-22885: SEGV when setting the comment of a modified zip entry#22892
khaledalam wants to merge 1 commit into
php:PHP-8.4from
khaledalam:fix-gh22885-zip-comment-segv

Conversation

@khaledalam

Copy link
Copy Markdown
Contributor

Fixes GH-22885.

@LamentXU123

Copy link
Copy Markdown
Member

Quite a radical fix I think at the first glance. However I do know why your fix is way more bigger than I expected since you've chosen a robuster way I haven't thought of. I will take a look ASAP.

@LamentXU123 LamentXU123 self-assigned this Jul 26, 2026

@LamentXU123 LamentXU123 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should target branch 8.4 as a bug fix.
At least for new codes I think adding these hint macros are good practices :/

Comment thread ext/zip/php_zip.c Outdated
Comment thread ext/zip/php_zip.c Outdated
Comment thread ext/zip/tests/gh22885.phpt Outdated
$zip->close();

if (!$zip->open($file)) {
exit('failed');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
exit('failed');
die('failed');

Comment thread ext/zip/php_zip.c Outdated
sys_err = zip_error_code_system(err);

comment = zip_file_get_comment(za, (zip_uint64_t) index, &comment_len, ZIP_FL_ENC_RAW);
if (comment == NULL || comment_len == 0) {

@LamentXU123 LamentXU123 Jul 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Given this, I also think we can add a test case where we have a blank comment.
I doubt there is a bug. Try this case:

$zip->open($file);
$zip->setMtimeName('x.txt', 1000000);
$zip->setCommentName('x.txt', '');
$zip->close();

Because I don't think your logic here and line 152 is correct.
You see, in line 66 we have

	/* 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;
	}

a later setCommentName($name, '') hits the equality fast path and returns true without actually removing the original comment.

Comment thread ext/zip/tests/gh22885.phpt Outdated
/* Each entry below is modified first, which makes libzip clone its central
* directory record, and gets a new comment afterwards. */
if (!$zip->open($file)) {
exit('failed');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
exit('failed');
die('failed');

@khaledalam
khaledalam force-pushed the fix-gh22885-zip-comment-segv branch from 9899601 to c8b9e32 Compare July 26, 2026 22:37
@khaledalam
khaledalam changed the base branch from master to PHP-8.4 July 26, 2026 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SEGV zim_ZipArchive_setCommentName

3 participants