Fix bug #76524 - ZipArchive memory leak #3330

timurib · 2018-06-25T11:17:29Z

(This PR duplicates the previous one, but targets to the 7.1 branch instead of 5.6)

The memory leak occurs when

a ZipArchive is opened with ZipArchive::CREATE|ZipArchive::OVERWRITE flags,
it's empty,
it's destroyed without explicit close() method call.

The leak does not occur if the zip file already exist.

https://bugs.php.net/bug.php?id=76524

cmb69 · 2018-06-25T11:24:47Z

cc @remicollet

timurib · 2018-06-25T13:16:18Z

Hmm, if I correctly understand the AppVeyor build log, ZipArchive doesn't print the warning message on Windows:

========DIFF========
002- Warning: Unknown: Cannot destroy the zip context: Can't remove file: No such file or directory in Unknown on line 0

It's not directly related with patch, looks like zip_close() just doesn't fail in this case, but better to check it. I don't have Windows right now, so it will take some time…

weltling · 2018-06-27T20:37:11Z

ext/zip/tests/bug76524.phpt

+$filename = __DIR__ . '/nonexistent.zip';
+
+// The error is not reproduced when file already exist:
+if (!file_exists($filename)) {


Why not just ensure the filename doesn't exist?

Btw., how would it behave if something was added to that non existent archive? Perhaps this case deserves a separate test.

Thanks.

Why not just ensure the filename doesn't exist?

Of course, it's not necessary. I thought this is clearer shows the conditions which cause the problem. If it's not obvious, then I'll rather remove this.

Btw., how would it behave if something was added to that non existent archive? Perhaps this case deserves a separate test.

In this case all right: it just doesn't create an empty archives.
If an archive is not exist and we add some files then new zip file will be created.
If an archive already exists and we don't add any files, but use OVERWRITE then it remove the file without warning. I found this is weird, but it works so.
But you right, I'll check the test suite of the extension for coverage this cases.

weltling · 2018-06-27T20:40:01Z

ext/zip/php_zip.c

 	if (intern->za) {
 		if (zip_close(intern->za) != 0) {
 			php_error_docref(NULL, E_WARNING, "Cannot destroy the zip context: %s", zip_strerror(intern->za));
-			return;


intern->za was not NULL, so it's probably not destroyed at this point. In further, there are also two superfluous NULL assignments.

Thanks.

Ok, I'll check it. Thanks!

…ndows

timurib · 2018-06-30T21:32:30Z

So, now the zip resources freeing is well as in close() method implementation. @weltling was right, it need to explicit zip_discard call after failed zip_close.

Also, tests are depends on OS now, because on Windows it doesn't show the warning message. I tried to debug this, but I'm not very well on Windows development tools. The difference in this case is occurred because this remove() call in Linux returns error code, but the corresponding function in Windows does not. And I don't even know which of them is correct. Finally, it's irrelevant to the memory leak, so I just split the tests.

weltling · 2018-07-05T16:54:24Z

ext/zip/php_zip.c

 	if (intern->za) {
 		if (zip_close(intern->za) != 0) {
+#if LIBZIP_VERSION_MAJOR == 1 && LIBZIP_VERSION_MINOR == 3 && LIBZIP_VERSION_MICRO == 1
 			php_error_docref(NULL, E_WARNING, "Cannot destroy the zip context: %s", zip_strerror(intern->za));


Not sure, why this condition is necessary, is there something special with the exact version? Also, the call to php_error_docref is same in both branches.

Thanks.

Not sure, why this condition is necessary, is there something special with the exact version?

Yes, in the 1.3.1 a call zip_discard() after faulty zip_close() may cause segfault (because of use after free). For example, on the build with libzip 1.3.1 and without this condition, next code reproduces the failure:

<?php $zip = new ZipArchive(); $zip->open(__DIR__.'/filename.zip', ZipArchive::CREATE); // regardless OVERWRITE flag $zip->addFile(__DIR__); // just an error action

In the gdb:

Warning: Unknown: Cannot destroy the zip context: Read error: Is a directory in Unknown on line 0 Program received signal SIGSEGV, Segmentation fault. zip_source_free (src=0x20000000b) at zip_source_free.c:46 46 if (src->refcount > 0) { (gdb) backtrace #0 zip_source_free (src=0x20000000b) at zip_source_free.c:46 #1 0x00007ffff7bcbf7d in zip_source_free (src=0xc83960) at zip_source_free.c:68 #2 0x00007ffff7bcbf7d in zip_source_free (src=0xc7f3f0) at zip_source_free.c:68 #3 0x00007ffff7bc4867 in zip_discard (za=0xc7f680) at zip_discard.c:54 #4 0x00000000005d3d93 in php_zip_object_free_storage (object=0x7ffff6c681e8) at /home/timur/dev/c/php-src/ext/zip/php_zip.c:1008\ ...

As @remicollet noticed in the comments for bug 75540, only this libzip version is affected (just in case, I checked the 1.3.0 and 1.3.2, they are ok).

Also, the call to php_error_docref is same in both branches.

Yes, they should be different: the first branch shouldn't call zip_strerror (702ef2736). I'm going to fix it in next commit.

weltling · 2018-07-05T16:57:44Z

The behavior difference in throwing the warning seems to regard to libzip and platform. I guess, it should be ok to workaround the test by using the %A placeholder, to avoid the test duplication.

Thanks.

timurib · 2018-07-07T13:59:42Z

I guess, it should be ok to workaround the test by using the %A placeholder, to avoid the test duplication.

Unfortunatetly, the %A also "hides" Zend MM messages and the test will passed even if leak is actually present.

What about regular expression in the EXPECTF? Something like that:

--EXPECTF--
ok%r((?!memory leaks detected).)*%r

It works, but I am not sure that is looks fine.

cmb69 · 2018-07-07T14:09:53Z

You can use EXPECTREGEX instead of EXPECTF.

weltling · 2018-07-10T09:48:10Z

Merged as 08f0885. Thanks!

Fix bug #76524 - ZipArchive memory leak

b7d9497

timurib mentioned this pull request Jun 25, 2018

Fix bug #76524 - ZipArchive memory leak #3325

Closed

weltling reviewed Jun 27, 2018

View reviewed changes

Bugfix #76524: Free up zip internal state and adjust the tests for Wi…

02d2192

…ndows

krakjoe added the Bug label Jul 2, 2018

weltling reviewed Jul 5, 2018

View reviewed changes

Bugfix #76524: Fix possible use after free for libzip 1.3.1

8cafbd6

Bugfix #76524: Make the test independent of platform

3c5086a

weltling closed this Jul 10, 2018

Fix bug #76524 - ZipArchive memory leak #3330

Fix bug #76524 - ZipArchive memory leak #3330

Uh oh!

Conversation

timurib commented Jun 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmb69 commented Jun 25, 2018

Uh oh!

timurib commented Jun 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weltling Jun 27, 2018

Choose a reason for hiding this comment

Uh oh!

weltling Jun 27, 2018

Choose a reason for hiding this comment

Uh oh!

timurib Jun 28, 2018

Choose a reason for hiding this comment

Uh oh!

weltling Jun 27, 2018

Choose a reason for hiding this comment

Uh oh!

timurib Jun 28, 2018

Choose a reason for hiding this comment

Uh oh!

timurib commented Jun 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weltling Jul 5, 2018

Choose a reason for hiding this comment

Uh oh!

timurib Jul 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weltling commented Jul 5, 2018

Uh oh!

timurib commented Jul 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmb69 commented Jul 7, 2018

Uh oh!

weltling commented Jul 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

timurib commented Jun 25, 2018 •

edited

Loading

timurib commented Jun 25, 2018 •

edited

Loading

timurib commented Jun 30, 2018 •

edited

Loading

timurib Jul 7, 2018 •

edited

Loading

timurib commented Jul 7, 2018 •

edited

Loading