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

[khaledalam]

Fixes GH-22885.

[LamentXU123]

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]

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

[devnexen]

Can the following test be added ?

--TEST--
GH-22885 (unchangeArchive() must not drop the modified entry bookkeeping)
--EXTENSIONS--
zip
--FILE--
<?php
$file = __DIR__ . '/gh22885_unchange.zip';

@unlink($file);

$zip = new ZipArchive;
if (!$zip->open($file, ZipArchive::CREATE)) {
    exit('failed');
}
$zip->addFromString('a.txt', 'data of a');
$zip->setCommentName('a.txt', 'comment of a');
$zip->addFromString('b.txt', 'data of b');
$zip->setCommentName('b.txt', 'comment of b');
$zip->setArchiveComment('archive comment');
$zip->close();

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

/* unchangeArchive() only reverts the archive level changes, the entries stay
 * modified, so the comment of a modified entry must still be refused. */
var_dump($zip->setMtimeName('a.txt', 1000000));
var_dump($zip->setArchiveComment('new archive comment'));
var_dump($zip->unchangeArchive());
var_dump($zip->getArchiveComment());
var_dump($zip->setCommentName('a.txt', 'new comment of a'));

/* unchangeName() does revert the entry, so its comment can be set again. */
var_dump($zip->setMtimeName('b.txt', 1000000));
var_dump($zip->unchangeName('b.txt'));
var_dump($zip->setCommentName('b.txt', 'new comment of b'));

$zip->close();

if (!$zip->open($file)) {
    exit('failed');
}
var_dump($zip->getArchiveComment());
var_dump($zip->getCommentName('a.txt'));
var_dump($zip->getCommentName('b.txt'));
$zip->close();
?>
--EXPECTF--
bool(true)
bool(true)
bool(true)
string(15) "archive comment"

Warning: ZipArchive::setCommentName(): Cannot set the comment of an entry that has been modified, set the comment before modifying the entry in %s on line %d
bool(false)
bool(true)
bool(true)
bool(true)
string(15) "archive comment"
string(12) "comment of a"
string(16) "new comment of b"
--CLEAN--
<?php
@unlink(__DIR__ . '/gh22885_unchange.zip');
?>

[LamentXU123]

Could you please add these cases in tests? I doubt these are bugs:

  <?php
  $file = __DIR__ . "/poc_encryption_failed.zip";
  @unlink($file);

  $zip = new ZipArchive();
  $zip->open($file, ZipArchive::CREATE);
  $zip->addFromString("a.txt", "data");
  $zip->setCommentName("a.txt", "old comment");
  $zip->close();

  $zip->open($file);

  var_dump($zip->setEncryptionName("a.txt", 9999, "pw"));
  var_dump($zip->setCommentName("a.txt", "new comment"));
  $zip->close();

<?php
  $file = __DIR__ . "/poc_false_positive.zip";
  @unlink($file);

  $zip = new ZipArchive();
  $zip->open($file, ZipArchive::CREATE);
  $zip->addFromString("a.txt", "data");
  $zip->setCommentName("a.txt", "old comment");
  $zip->close();

  $zip->open($file);
  var_dump($zip->setCommentName("a.txt", "first new comment"));
  var_dump($zip->setMtimeName("a.txt", 1000000));
  var_dump($zip->setCommentName("a.txt", "second new comment"));

  $zip->close();

[pierrejoye]

Did we not fix this already in a previous PR? that one #22652

I cannot reproduce using master. Not sure if that merge was back-ported to previous stable branches.

[LamentXU123]

#22652 fixes the identical-comment reset case. this PR handles the remaining modified-entry or different-comment cases. So they are different bugs

[LamentXU123]

And the fix only affect low libzip versions as a workaround. So most people with a not ancient libzip isn't affected.