Fix GH-22649: avoid ZipArchive comment reset crash

[LamentXU123]

Rewrite the PHP_ZIP_SET_FILE_COMMENT macro to solve this uaf that occurs after overwriting an existing entry and setting its inherited unchanged comment again. My solution is to simply add a if branch.

I also add some defensive checks in the function by the way (ZEND_ASSERT(comment_len <= 0xffff);) just in case someone will re-use this helper in the future.

Fixes #22649

[devnexen]

Sometimes an issue not (or not entirely) in php side but in the dependency part. It needs to be reported upstream if that is the case, ideally with a C reproducer (unless it had been fixed recently already). I ll let you investigate it .. I think however the necessary workaround looks ok.

[pierrejoye]

I am fine with this PR. We bundle libzip and some distros build php with external libzip. This PR handles both cases. The macro should have been a function to begin with (I think I did it ;-) as it is older than the svn sync (oh my).

[pierrejoye]

I am fine with this PR. We bundle libzip and some distros build php with external libzip. This PR handles both cases. The macro should have been a function to begin with (I think I did it ;-).

and I confirm the crash. I can't approve tho'.... while being the original author of this extension :)

[LamentXU123]

@devnexen Can you please take a look code-wise? I am the only codeowner of this extension now so I couldn't approve myself :)

[LamentXU123]

This is an upstream bug, php itself calls the API very fine. However, as @pierrejoye just said, a workaround in the php side might be more robust than upstream fixes. wdyt?

[devnexen]

as I said the workaround looks correct to me but I also suspect a bug in libzip at least in earlier releases.

[LamentXU123]

Ha. The bug has already been reported in nih-at/libzip#514 and fixed in libzip 1.11.5.
Good. So we only need to merge this workaround to support for low libzip versions.