php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login

go to bug id or search bugs for

Bug #66786 ZipArchive fails when an added file is removed before close()
Submitted: 2014-02-26 23:16 UTC Modified: 2014-03-02 13:43 UTC
From: lists dot ban at herbesfolles dot org Assigned:
Status: Not a bug Package: Zip Related
PHP Version: master-Git-2014-02-26 (Git) OS: Linux (Debian Sid), probably any
Private report: No CVE-ID: None
View Add Comment Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
You can add a comment by following this link or if you reported this bug, you can edit this bug over here.
(description)
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: lists dot ban at herbesfolles dot org
New email:
PHP Version: OS:

 

 [2014-02-26 23:16 UTC] lists dot ban at herbesfolles dot org 
Description:
------------
When adding a file to a Zip archive, the user expects to be able to unlink the original file without issues.  The documentation on ZipArchive::addFile() even has a (admittedly strangely worded) note suggesting it would work:

    "When a file is set to be added to the archive, PHP will attempt to lock the file and it is only released once the ZIP operation is done. In short, it means you can first delete an added file after the archive is closed."

However, in practice it doesn't work, and depending on the PHP version (or build?) it could even silently fail.  PHP 5.5.9 from Debian Sid fails on ZipArchive::close() with StatusString as "No error";  PHP 5.7.0-dev from today Git don't even report any problem.  In any case, the result zip is not created.

This is both with system (0.11.2) and bundled libzip.

Test script:
---------------
$zip = new ZipArchive();
$zip->open('output.zip', ZipArchive::CREATE | ZipArchive::OVERWRITE);
file_put_contents('dummy.txt', 'hello world');
if (! $zip->addFile('dummy.txt'))
	echo "addFile() failed\n";
unlink('dummy.txt');
if (! $zip->close())
	echo "close() failed\n";
echo "output.zip size: ", filesize('output.zip'), "\n";
@unlink('output.zip');

Expected result:
----------------
output.zip size: 129

Actual result:
--------------
output.zip size: 
Warning: filesize(): stat failed for output.zip in zipbug.php on line 9

Patches

0001-Zip-fix-creating-archive-if-an-added-file-is-removed.patch (last revision 2014-02-26 23:18 UTC by lists dot ban at herbesfolles dot org)

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2014-02-28 10:09 UTC] ab@php.net
-Status: Open +Status: Feedback
 [2014-02-28 10:09 UTC] ab@php.net 
Maybe I misread it, but according to the quote you gave it shouldn't work:

"you can first delete an added file after the archive is closed"

In 5.5 there's libzip-0.10.1 and in 5.6+ is libzip 0.11.2 . Looking at those code, the diff between the current situation and your patch were only that you pass an open descriptor. When the file is deleted, the descriptor will be still open, true. That however will not work on Windows, so different handling. Were it not safer just using addFromString?
 [2014-03-01 00:28 UTC] lists dot ban at herbesfolles dot org
-Status: Feedback +Status: Open
 [2014-03-01 00:28 UTC] lists dot ban at herbesfolles dot org 
> Maybe I misread it, but according to the quote you gave it shouldn't
> work:
> 
> "you can first delete an added file after the archive is closed"

After your comment I did some research and asked a few English speakers, and even though no dictionary helped me understand this sentence (particularly the use of "first" is never suggest to also mean something like "only"), the asked native speakers understood it like you seem to do -- while admitting the sentence was confusing.

However, the first sentence taken alone, "When a file is set to be added to the archive, *PHP will attempt to lock the file* and it is only released once the ZIP operation is done" is consistently understood as that it should either just work or `unlink()` should fail.

So, I suggest the note be rewritten to avoid any confusion -- and if actually PHP don't attempt any locking (or if it doesn't work in practice) maybe the note should not suggest it does.

> Looking at those code, the diff between the current situation and
> your patch were only that you pass an open descriptor. When the
> file is deleted, the descriptor will be still open, true. That
> however will not work on Windows, so different handling.

Right, Windows will not accept unlinking an open file.  Well, alternatively this could work if PHP delayed the unlinking because the file was open by the script, but I guess this is way out of scope.

> Were it not safer just using addFromString?

I was worried that adding several files with addFromString() would consume unreasonable amount of memory -- but I must admit I didn't perform actual tests to compare.

---

So, I guess this isn't really a bug in the Zip extension, and only a possibly confusing note in its documentation.

Though, there still seem to be a problem in the Git version, since a test case reported absolutely no error through the API -- see the test in the patch, it will of course fail, but neither addFile() nor close() will report any problem.
 [2014-03-01 07:18 UTC] pajoye@php.net
-Status: Open +Status: Not a bug
 [2014-03-01 07:18 UTC] pajoye@php.net 
Files handles are kept until the archive is closed. That's why they cannot be deleted or removed.

addFromString uses memory buffers, so you do not have this issue.

It is not really a bug but how libzip works. I did not hear about a change in this area but when this change happens, the doc will be updated accordingly.

move to "not a bug".
 [2014-03-02 13:43 UTC] lists dot ban at herbesfolles dot org 
> It is not really a bug but how libzip works.

OK, fair enough.

> I did not hear about a change in this area but when this change happens, the doc will be updated accordingly.

As I said earlier, I believe the documentation should be updated to be more clear in any case.  Currently, even native English speakers agree that the second sentence ("In short, it means you can first delete an added file after the archive is closed") is at best confusing, and I for myself (non-native speaker) could simply not understand anything in this sentence.

I propose to e.g. simply replace "first" with "only":

  "In short, it means you can only delete an added file after the archive is closed."

This way, I would have understood, and AFAICT it's only clearer for everyone.

Though, I read the previous sentence ("When a file is set to be added to the archive, PHP will attempt to lock the file and it is only released once the ZIP operation is done.") and its "PHP will attempt to lock the file" as that at worse unlink() would fail if I call it before the zip operation is done.

So maybe the part about "PHP will attempt to lock the file" should be removed since in practice it's not the case (or it doesn't work under Linux).

---

Also, again, the test case in the patch (even without the patch) show a regression in Git where if the file was unlinked before save() and after addFile() *no error* would be reported by the API.  This is a bug in any case, and 5.5.9 used to report a problem on close().

Should I create new reports for each of these issues?  One against the documentation and the other again against the Zip extension for that regression?
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved. 		Last updated: Sat Apr 20 09:01:28 2024 UTC