php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #62106 zip can (and will, in real-life cases) leak huge tempfiles
Submitted: 2012-05-22 07:47 UTC Modified: 2016-09-05 15:39 UTC
Votes:7
Avg. Score:3.1 ± 0.3
Reproduced:2 of 2 (100.0%)
Same Version:2 (100.0%)
Same OS:2 (100.0%)
From: t dot glaser at tarent dot de Assigned: ab (profile)
Status: Assigned Package: Zip Related
PHP Version: master-Git-2012-05-22 (Git) OS: Unix/POSIX, maybe Windows
Private report: No CVE-ID: None
View Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
If you reported this bug, you can edit this bug over here.
(description)
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: t dot glaser at tarent dot de
New email:
PHP Version: OS:

 

 [2012-05-22 07:47 UTC] t dot glaser at tarent dot de
Description:
------------
I’ve been investigating a disc full case on some of our servers, which was 
fixed by removing eight Gibibytes (!) of *.zip.?????? files (about a dozen 
files) that looked like mkstemp(3)-generated, and investigated this problem.

To cut a long story short: an instance of the ZipArchive class does 
its “action” when the close() method is called. (There should also be an 
abort() method which frees all ressources and unlocks all files, for if an 
error is detected in between open/addfile and further addfile calls or the 
close call, but that’s not the problem at hand, it just irritated me.) This 
ends up being [php-src.git] / ext / zip / lib / zip_close.c (I’ve just looked 
at gitweb master to reconfirm this), which has the following problem:

zip_close() forcibly allocates a temporary file, operates on that and then 
atomically renames the temporary file to the desired output filename. This may 
be a good thing in a regular Unix environment (and looking at the authors, I 
know at least one of them from NetBSD®, so the code probably does come from a 
Unix environment). HOWEVER, this is a PHP environment, not a regular Unix 
environment, even if it may run on Unix, and PHP, when running in a webserver, 
but also as CLI, asserts certain resource limits – execution time (not in 
CLI), memory size, etc.

When the script is terminated due to such a limit (in our case, probably the 
execution time limit in apache2 mod_php), the tempfile is not cleaned up. 
Period. And when you were zipping up 400 MiB, and the termination happens when 
300 MiB of that are already written to disc… well, not nice.

The cleanest way would probably be for the PHP core to offer an extension (C, 
but PHP would also make sense) to register cleanup hooks that run when script 
execution is terminated. This would be more generically usable than just a 
method with which an extension could register tempfiles which were then 
removed, as cleanup may involve other tasks. (As a general suggestion, cleanup 
execution time could be limited to the greater of 5 seconds and a tenth of the 
script execution time limit, and memory size should be similarly extended by a 
small amount so it won’t terminate abnormally during the cleanup phase.)

Test script:
---------------
createNewestReleaseFilesAsZip method (at the end of the file) in:

https://evolvis.org/scm/viewvc.php/trunk/gforge_base/evolvisforge-5.1/src/common/frs/FRSPackage.class.php?revision=18116&root=evolvis&view=markup

Note how a later revision of the same file already contains some attempts at error handling, and how it the lack of an abort method makes that awkward.

Expected result:
----------------
No tempfile leak. If no resource limit is hit, the .zip file is created; if 
one is hit, the .zip file is either untouched or removed (unlinked).

Actual result:
--------------
If a resource limit is hit, a tempfile of arbitrary size (at least several 
hundred Mebibytes confirmed) is leaked.

Patches

62106.patch (last revision 2020-03-19 17:43 UTC by tatzaad at gmail dot com)

Pull Requests

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2012-07-10 14:14 UTC] ab@php.net
The following patch has been added/updated:

Patch Name: 62106.patch
Revision:   1341929640
URL:        https://bugs.php.net/patch-display.php?bug=62106&patch=62106.patch&revision=1341929640
 [2012-07-10 14:20 UTC] ab@php.net
Please test the supplied patch. Ideally you could do this with your productive 
dev environment. Because cleanup happens on request shutdown, looks like it's 
nearly impossible to test this with phpt. Consider the following piece of code:

<?php

set_time_limit(0);
ini_set('memory_limit', '768M');

$zip_fname = dirname(__FILE__) . DIRECTORY_SEPARATOR . 'bug62106.zip';

$s = 'a';
foreach (array(1024, 1024, 200) as $b) {
        $s = str_repeat($s, $b);
}

$zip = new ZipArchive();
$r = $zip->open($zip_fname, ZipArchive::CREATE);

if ($r) {
        $zip->addFromString('huhu.txt', $s);

        set_time_limit(3);
        $zip->close();
}

register_shutdown_function will not work here because it's a part of request. 
But it's clearly to see - without patch there are temp files there, and with the 
patch the cleanup works. But the best were of course if you test it directly 
with your app. If you have an idea how to test this with phpt, it would be also 
great :)
 [2012-07-10 14:20 UTC] ab@php.net
-Status: Open +Status: Feedback -Assigned To: +Assigned To: ab
 [2012-07-12 14:19 UTC] t dot glaser at tarent dot de
Hum. Unfortunately, I cannot bring a self-compiled PHP into the production 
system, especially as it runs Debian lenny still. But within some time, I 
should be able to test the patch on Debian wheezy, on a test instance, by 
using a self-compiled locally patched source package, as they’re on 5.4 
already.

Your testcase looks sound to exhibit this behaviour on all but the fastest 
machines.

I have no idea what phpt is, sorry.
 [2012-07-12 14:19 UTC] t dot glaser at tarent dot de
-Status: Feedback +Status: Assigned
 [2012-07-12 15:27 UTC] ab@php.net
Yep, I'd better not start do get all the 5.4 deps on lenny :)

The test code should work also on faster mashines. If not - decreasing the last 
set_time_limit(3) or increasing the data amount should work. On 
current PHP versions a temp file should be laying around anyway after the script 
has 
exited with the time limit warning. I mean even the fastest drive cant get 200M 
wrote on it in 3 sec ) . But way much better is of course to test 
with your app on a test instance.

The patch should apply to 5.3, 5.4 and master as well. 

PHPT are the test case files from the PHP test framework. Such files are usually 
to find in "tests" subfolders inside the PHP source tree. You can read more 
under http://qa.php.net .
 [2012-07-12 17:53 UTC] pajoye@php.net
hi!

Thanks for the bug report and patch!

I do not like the idea of having php specific function called or used like it is 
in this patch.

Temporary files are created while working with a given archive. These files 
should be destroyed when the archive is not used anymore, on resource or object 
destroy.

Maybe a function in the zip library could be added for this cleanup.
 [2012-07-12 18:58 UTC] ab@php.net
I'll be working on improving libzip then. But I think, testing the current patch 
would make sense anyway. At least it could prove the code path, despite we could 
pack the improved libzip stuff into a __destruct() or alike.
 [2016-09-05 15:39 UTC] cmb@php.net
> Maybe a function in the zip library could be added for this
> cleanup.

Wouldn't calling zip_discard() be sufficient if zip_close() fails?
Cf. <https://github.com/php/php-src/blob/PHP-7.0.10/ext/zip/php_zip.c#L1515-L1518>.
 [2020-03-19 17:43 UTC] tatzaad at gmail dot com
The following patch has been added/updated:

Patch Name: 62106.patch
Revision:   1584639809
URL:        https://bugs.php.net/patch-display.php?bug=62106&patch=62106.patch&revision=1584639809
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Thu Nov 21 12:01:29 2024 UTC