Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ZipArchive deletes zip file with no contents and doesn't update stat cache #8781

Closed
timint opened this issue Jun 14, 2022 · 10 comments
Closed

Comments

@timint
Copy link

timint commented Jun 14, 2022

Description

There is a problem with ZipArchive deleting empty archives without notice upon successful close. And statcache not updated.

<?php
    $tmp_file = tempnam(sys_get_temp_dir(), '');
    var_dump(is_file($tmp_file)); // returns true

    $zip = new ZipArchive;
    $zip->open($tmp_file, ZipArchive::OVERWRITE);
    $zip->close(); // returns true

    var_dump(is_file($tmp_file)); // returns true
    var_dump(file_get_contents($tmp_file)); // returns error file does not exist

I expected the defined file to remain. If I can't count on that I expected the statcache to be updated.

PHP Version

PHP 8.1.6

Operating System

Windows 11 x64

@remicollet
Copy link
Member

Confirmed

I expected the defined file to remain.

This the default behavior of the libzip library, (perhaps since empty file are not valid zip archive), so nothing we can change in php

If I can't count on that I expected the statcache to be updated.

PR pending

@remicollet
Copy link
Member

remicollet commented Jun 15, 2022

Fixed in standalone pecl extension
pierrejoye/php_zip@fac6809

remicollet added a commit to remicollet/php-src that referenced this issue Jun 15, 2022
remicollet added a commit to remicollet/php-src that referenced this issue Jun 15, 2022
@remicollet
Copy link
Member

See PR #8783

@cmb69
Copy link
Member

cmb69 commented Jun 15, 2022

perhaps since empty file are not valid zip archive

I don't think this is true. On Windows, I can create an empty.zip via the explorer, and the specification says:

4.1.2 ZIP files SHOULD contain at least one file and MAY contain multiple files.

"SHOULD contain" is not "MUST contain".

We may consider to still create such an empty archive, even if libzip would delete the file. Or maybe upstream would reconsider the behavior for empty archives.

@remicollet
Copy link
Member

remicollet commented Jun 15, 2022

perhaps since empty file are not valid zip archive

I don't think this is true

I mean "empty file" (size=0) are not valid, not about "empty archive" ;)

@cmb69
Copy link
Member

cmb69 commented Jun 15, 2022

Yeah, but this ticket is about empty archives:

    $zip = new ZipArchive;
    $zip->open($tmp_file, ZipArchive::OVERWRITE);
    $zip->close(); // returns true

@remicollet
Copy link
Member

remicollet commented Jun 15, 2022

Yes, but the example use a "empty file" (tempnam) which is not a valid archive.

And indeed, the library doesn't allow to create an empty archive, but I don't think we can fix this in the extension.

P.S. opening an empty archive works (and it is not deleted during the close)

For memory, upstream code: https://github.com/nih-at/libzip/blob/v1.9.0/lib/zip_close.c#L65

@cmb69
Copy link
Member

cmb69 commented Jun 15, 2022

Yes, but the example use a "empty file" (tempnam) which is not a valid archive.

Oh, right!

And indeed, the library doesn't allow to create an empty archive, but I don't think we can fix this in the extension.

Thinking about it, that should really be fixed upstream. For instance, given that I have an archive, and delete some entries, after ::close() the archive is gone, if there are no more entries. At least that happens with libzip 1.7.3 (I'm feeling bad, because winlibs are so far behind).

@remicollet
Copy link
Member

libzip discussion nih-at/libzip#299

remicollet added a commit that referenced this issue Jun 15, 2022
* PHP-8.0:
  NEWS
  Fix GH-8781 ZipArchive::close deletes zip file without updating stat cache
remicollet added a commit that referenced this issue Jun 15, 2022
* PHP-8.1:
  NEWS
  NEWS
  Fix GH-8781 ZipArchive::close deletes zip file without updating stat cache
@remicollet
Copy link
Member

Followup: add documentation of library expected /documented behavior in our documentation
php/doc-en#1628

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants
@adoy @remicollet @timint @cmb69 and others