Skip to content

Commit e352068

Browse files
tstarlingtstarling
committed
Rename and refactor ZipArchive::openBuffer
Fixes to #14078: * Rename ZipArchive::openBuffer() to ::openString(). * For consistency with ::open(), return int|bool, don't throw an exception on error. Provide error information via existing properties and accessors. * Fix memory leak when ::openString() is called but ::close() is not called. Add test. * Fix memory leak when a call to ::open() is followed by a call to ::openString(). Add test. * Let libzip own the source, don't call zip_source_keep(). * Share buffer handling with ZipArchive::addFromString(). Elsewhere: * If there is an error from zip_close() during a call to ZipArchive::open(), emit a warning but proceed to open the archive, don't return early. Add test. * When buffers are saved by ZipArchive::addFromString(), release them in ZipArchive::close() and ::open(), don't accumulate buffers until the free_obj handler is called. * Factor out buffer handling and reuse it in ZipArchive::openString()
1 parent d391dcb commit e352068

File tree

7 files changed

+144
-102
lines changed

7 files changed

+144
-102
lines changed

‎ext/zip/php_zip.c‎

Lines changed: 90 additions & 94 deletions
Original file line numberDiff line numberDiff line change
@@ -575,6 +575,76 @@ static char * php_zipobj_get_zip_comment(ze_zip_object *obj, int *len) /* {{{ */
575575
}
576576
/* }}} */
577577

578+
/* Add a string to the list of buffers to be released when the object is destroyed.*/
579+
static void php_zipobj_add_buffer(ze_zip_object *obj, zend_string *str) /* {{{ */
580+
{
581+
int pos = 0;
582+
if (obj->buffers_cnt) {
583+
obj->buffers = safe_erealloc(obj->buffers, sizeof(*obj->buffers), (obj->buffers_cnt + 1), 0);
584+
pos = obj->buffers_cnt++;
585+
} else {
586+
obj->buffers = emalloc(sizeof(*obj->buffers));
587+
obj->buffers_cnt++;
588+
pos = 0;
589+
}
590+
obj->buffers[pos] = zend_string_copy(str);
591+
}
592+
/* }}} */
593+
594+
static void php_zipobj_release_buffers(ze_zip_object *obj) /* {{{ */
595+
{
596+
if (obj->buffers_cnt > 0) {
597+
for (int i = 0; i < obj->buffers_cnt; i++) {
598+
zend_string_release(obj->buffers[i]);
599+
}
600+
efree(obj->buffers);
601+
}
602+
obj->buffers_cnt = 0;
603+
}
604+
/* }}} */
605+
606+
/* Close and free the zip_t */
607+
static bool php_zipobj_close(ze_zip_object *obj) /* {{{ */
608+
{
609+
struct zip *intern = obj->za;
610+
bool success = false;
611+
612+
if (intern) {
613+
int err = zip_close(intern);
614+
if (err) {
615+
php_error_docref(NULL, E_WARNING, "%s", zip_strerror(intern));
616+
/* Save error for property reader */
617+
zip_error_t *ziperr;
618+
619+
ziperr = zip_get_error(intern);
620+
obj->err_zip = zip_error_code_zip(ziperr);
621+
obj->err_sys = zip_error_code_system(ziperr);
622+
zip_error_fini(ziperr);
623+
zip_discard(intern);
624+
} else {
625+
obj->err_zip = 0;
626+
obj->err_sys = 0;
627+
}
628+
success = !err;
629+
}
630+
631+
// if we have a filename, we need to free it
632+
if (obj->filename) {
633+
/* clear cache as empty zip are not created but deleted */
634+
php_clear_stat_cache(1, obj->filename, obj->filename_len);
635+
636+
efree(obj->filename);
637+
obj->filename = NULL;
638+
obj->filename_len = 0;
639+
}
640+
641+
php_zipobj_release_buffers(obj);
642+
643+
obj->za = NULL;
644+
return success;
645+
}
646+
/* }}} */
647+
578648
int php_zip_glob(zend_string *spattern, zend_long flags, zval *return_value) /* {{{ */
579649
{
580650
int cwd_skip = 0;
@@ -1021,21 +1091,8 @@ static void php_zip_object_dtor(zend_object *object)
10211091
static void php_zip_object_free_storage(zend_object *object) /* {{{ */
10221092
{
10231093
ze_zip_object * intern = php_zip_fetch_object(object);
1024-
int i;
10251094

1026-
if (intern->za) {
1027-
if (zip_close(intern->za) != 0) {
1028-
php_error_docref(NULL, E_WARNING, "Cannot destroy the zip context: %s", zip_strerror(intern->za));
1029-
zip_discard(intern->za);
1030-
}
1031-
}
1032-
1033-
if (intern->buffers_cnt>0) {
1034-
for (i=0; i<intern->buffers_cnt; i++) {
1035-
zend_string_release(intern->buffers[i]);
1036-
}
1037-
efree(intern->buffers);
1038-
}
1095+
php_zipobj_close(intern);
10391096

10401097
#ifdef HAVE_PROGRESS_CALLBACK
10411098
/* if not properly called by libzip */
@@ -1047,12 +1104,7 @@ static void php_zip_object_free_storage(zend_object *object) /* {{{ */
10471104
php_zip_cancel_callback_free(intern);
10481105
#endif
10491106

1050-
intern->za = NULL;
10511107
zend_object_std_dtor(&intern->zo);
1052-
1053-
if (intern->filename) {
1054-
efree(intern->filename);
1055-
}
10561108
}
10571109
/* }}} */
10581110

@@ -1448,19 +1500,8 @@ PHP_METHOD(ZipArchive, open)
14481500
RETURN_FALSE;
14491501
}
14501502

1451-
if (ze_obj->za) {
1452-
/* we already have an opened zip, free it */
1453-
if (zip_close(ze_obj->za) != 0) {
1454-
php_error_docref(NULL, E_WARNING, "Empty string as source");
1455-
efree(resolved_path);
1456-
RETURN_FALSE;
1457-
}
1458-
ze_obj->za = NULL;
1459-
}
1460-
if (ze_obj->filename) {
1461-
efree(ze_obj->filename);
1462-
ze_obj->filename = NULL;
1463-
}
1503+
// If we already have an opened zip, free it
1504+
php_zipobj_close(ze_obj);
14641505

14651506
/* open for write without option to empty the archive */
14661507
if ((flags & (ZIP_TRUNCATE | ZIP_RDONLY)) == 0) {
@@ -1488,44 +1529,46 @@ PHP_METHOD(ZipArchive, open)
14881529
}
14891530
/* }}} */
14901531

1491-
/* {{{ Create new read-only zip using given buffer */
1492-
PHP_METHOD(ZipArchive, openBuffer)
1532+
/* {{{ Create new read-only zip using given string */
1533+
PHP_METHOD(ZipArchive, openString)
14931534
{
14941535
struct zip *intern;
14951536
zend_string *buffer;
14961537
zval *self = ZEND_THIS;
1497-
ze_zip_object *ze_obj;
14981538

14991539
if (zend_parse_parameters(ZEND_NUM_ARGS(), "S", &buffer) == FAILURE) {
15001540
RETURN_THROWS();
15011541
}
15021542

1543+
ze_zip_object *ze_obj = Z_ZIP_P(self);
1544+
15031545
zip_error_t err;
15041546
zip_error_init(&err);
15051547

15061548
zip_source_t * zip_source = zip_source_buffer_create(ZSTR_VAL(buffer), ZSTR_LEN(buffer), 0, &err);
15071549

15081550
if (!zip_source) {
1509-
zend_value_error("Cannot open zip: %s", zip_error_strerror(&err));
1551+
ze_obj->err_zip = zip_error_code_zip(&err);
1552+
ze_obj->err_sys = zip_error_code_system(&err);
15101553
zip_error_fini(&err);
1511-
RETURN_THROWS();
1554+
RETURN_LONG(ze_obj->err_zip);
15121555
}
15131556

1514-
ze_obj = Z_ZIP_P(self);
1557+
php_zipobj_close(ze_obj);
15151558

15161559
intern = zip_open_from_source(zip_source, ZIP_RDONLY, &err);
15171560
if (!intern) {
1518-
zip_source_free(zip_source);
1519-
zend_value_error("Cannot open zip: %s", zip_error_strerror(&err));
1561+
ze_obj->err_zip = zip_error_code_zip(&err);
1562+
ze_obj->err_sys = zip_error_code_system(&err);
15201563
zip_error_fini(&err);
1521-
RETURN_THROWS();
1564+
zip_source_free(zip_source);
1565+
RETURN_LONG(ze_obj->err_zip);
15221566
}
15231567

1524-
zip_source_keep(zip_source);
1525-
zip_error_fini(&err);
1526-
1568+
php_zipobj_add_buffer(ze_obj, buffer);
15271569
ze_obj->za = intern;
1528-
ze_obj->source = zip_source;
1570+
zip_error_fini(&err);
1571+
RETURN_TRUE;
15291572
}
15301573
/* }}} */
15311574

@@ -1556,52 +1599,14 @@ PHP_METHOD(ZipArchive, close)
15561599
{
15571600
struct zip *intern;
15581601
zval *self = ZEND_THIS;
1559-
ze_zip_object *ze_obj;
1560-
int err;
15611602

15621603
if (zend_parse_parameters_none() == FAILURE) {
15631604
RETURN_THROWS();
15641605
}
15651606

15661607
ZIP_FROM_OBJECT(intern, self);
15671608

1568-
ze_obj = Z_ZIP_P(self);
1569-
1570-
err = zip_close(intern);
1571-
if (err) {
1572-
php_error_docref(NULL, E_WARNING, "%s", zip_strerror(intern));
1573-
/* Save error for property reader */
1574-
zip_error_t *ziperr;
1575-
1576-
ziperr = zip_get_error(intern);
1577-
ze_obj->err_zip = zip_error_code_zip(ziperr);
1578-
ze_obj->err_sys = zip_error_code_system(ziperr);
1579-
zip_error_fini(ziperr);
1580-
zip_discard(intern);
1581-
} else {
1582-
ze_obj->err_zip = 0;
1583-
ze_obj->err_sys = 0;
1584-
}
1585-
1586-
// if we have a filename, we need to free it
1587-
if (ze_obj->filename) {
1588-
/* clear cache as empty zip are not created but deleted */
1589-
php_clear_stat_cache(1, ze_obj->filename, ze_obj->filename_len);
1590-
1591-
efree(ze_obj->filename);
1592-
ze_obj->filename = NULL;
1593-
ze_obj->filename_len = 0;
1594-
}
1595-
1596-
if (ze_obj->source) {
1597-
zip_source_close(ze_obj->source);
1598-
zip_source_free(ze_obj->source);
1599-
ze_obj->source = NULL;
1600-
}
1601-
1602-
ze_obj->za = NULL;
1603-
1604-
RETURN_BOOL(!err);
1609+
RETURN_BOOL(php_zipobj_close(Z_ZIP_P(self)));
16051610
}
16061611
/* }}} */
16071612

@@ -1915,7 +1920,6 @@ PHP_METHOD(ZipArchive, addFromString)
19151920
size_t name_len;
19161921
ze_zip_object *ze_obj;
19171922
struct zip_source *zs;
1918-
int pos = 0;
19191923
zend_long flags = ZIP_FL_OVERWRITE;
19201924

19211925
if (zend_parse_parameters(ZEND_NUM_ARGS(), "sS|l",
@@ -1926,15 +1930,7 @@ PHP_METHOD(ZipArchive, addFromString)
19261930
ZIP_FROM_OBJECT(intern, self);
19271931

19281932
ze_obj = Z_ZIP_P(self);
1929-
if (ze_obj->buffers_cnt) {
1930-
ze_obj->buffers = safe_erealloc(ze_obj->buffers, sizeof(*ze_obj->buffers), (ze_obj->buffers_cnt + 1), 0);
1931-
pos = ze_obj->buffers_cnt++;
1932-
} else {
1933-
ze_obj->buffers = emalloc(sizeof(*ze_obj->buffers));
1934-
ze_obj->buffers_cnt++;
1935-
pos = 0;
1936-
}
1937-
ze_obj->buffers[pos] = zend_string_copy(buffer);
1933+
php_zipobj_add_buffer(ze_obj, buffer);
19381934

19391935
zs = zip_source_buffer(intern, ZSTR_VAL(buffer), ZSTR_LEN(buffer), 0);
19401936

‎ext/zip/php_zip.h‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,6 @@ typedef struct _ze_zip_read_rsrc {
6868
/* Extends zend object */
6969
typedef struct _ze_zip_object {
7070
struct zip *za;
71-
zip_source_t *source;
7271
zend_string **buffers;
7372
HashTable *prop_handler;
7473
char *filename;

‎ext/zip/php_zip.stub.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -646,7 +646,7 @@ class ZipArchive implements Countable
646646
/** @tentative-return-type */
647647
public function open(string $filename, int $flags = 0): bool|int {}
648648

649-
public function openBuffer(string $data): void {}
649+
public function openString(string $data): bool|int {}
650650

651651
/**
652652
* @tentative-return-type

‎ext/zip/php_zip_arginfo.h‎

Lines changed: 4 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

ext/zip/tests/ziparchive_openbuffer.phpt renamed to ext/zip/tests/ZipArchive_openString.phpt

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
11
--TEST--
2-
ZipArchive::openBuffer() method
2+
ZipArchive::openString() method
33
--EXTENSIONS--
44
zip
55
--FILE--
66
<?php
77
$zip = new ZipArchive();
8-
$zip->openBuffer(file_get_contents(__DIR__."/test_procedural.zip"));
8+
$zip->openString(file_get_contents(__DIR__."/test_procedural.zip"));
99

1010
for ($i = 0; $i < $zip->numFiles; $i++) {
1111
$stat = $zip->statIndex($i);

‎ext/zip/tests/ZipArchive_openString_leak.phpt‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
--TEST--
2+
ZipArchive::openString leak tests
3+
--EXTENSIONS--
4+
zip
5+
--FILE--
6+
<?php
7+
$zip = new ZipArchive;
8+
$zip->openString(file_get_contents(__DIR__ . '/test.zip'));
9+
$zip = null;
10+
11+
$zip = new ZipArchive;
12+
$zip->openString(file_get_contents(__DIR__ . '/test.zip'));
13+
$zip->openString(file_get_contents(__DIR__ . '/test.zip'));
14+
$zip = null;
15+
16+
$zip = new ZipArchive;
17+
$zip->openString(file_get_contents(__DIR__ . '/test.zip'));
18+
$zip->open(__DIR__ . '/test.zip');
19+
$zip = null;
20+
21+
$zip = new ZipArchive;
22+
$zip->open(__DIR__ . '/test.zip');
23+
$zip->openString(file_get_contents(__DIR__ . '/test.zip'));
24+
$zip = null;
25+
--EXPECT--

‎ext/zip/tests/ZipArchive_open_with_close_fail.phpt‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
--TEST--
2+
zip_close() failure from ZipArchive::open()
3+
--EXTENSIONS--
4+
zip
5+
--FILE--
6+
<?php
7+
// Try to create an archive
8+
$zip = new ZipArchive();
9+
var_dump($zip->open(__DIR__ . '/close-fail.zip', ZipArchive::CREATE));
10+
file_put_contents(__DIR__.'/close-fail-file', '');
11+
$zip->addFile(__DIR__.'/close-fail-file');
12+
// Delete the file to trigger an error on close
13+
@unlink(__DIR__.'/close-fail-file');
14+
var_dump($zip->open(__DIR__ . '/test.zip', ZipArchive::RDONLY));
15+
var_dump($zip->getFromName('bar'));
16+
--EXPECTF--
17+
bool(true)
18+
19+
Warning: ZipArchive::open(): Can't open file: No such file or directory in %s on line %d
20+
bool(true)
21+
string(4) "bar
22+
"

0 commit comments

Comments
 (0)