Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
189 changes: 118 additions & 71 deletions ext/zip/php_zip.c
Original file line number Diff line number Diff line change
Expand Up @@ -575,6 +575,76 @@ static char * php_zipobj_get_zip_comment(ze_zip_object *obj, int *len) /* {{{ */
}
/* }}} */

/* Add a string to the list of buffers to be released when the object is destroyed.*/
static void php_zipobj_add_buffer(ze_zip_object *obj, zend_string *str) /* {{{ */
{
int pos = 0;
if (obj->buffers_cnt) {
obj->buffers = safe_erealloc(obj->buffers, sizeof(*obj->buffers), (obj->buffers_cnt + 1), 0);
pos = obj->buffers_cnt++;
} else {
obj->buffers = emalloc(sizeof(*obj->buffers));
obj->buffers_cnt++;
pos = 0;
}
obj->buffers[pos] = zend_string_copy(str);
}
/* }}} */

static void php_zipobj_release_buffers(ze_zip_object *obj) /* {{{ */
{
if (obj->buffers_cnt > 0) {
for (int i = 0; i < obj->buffers_cnt; i++) {
zend_string_release(obj->buffers[i]);
}
efree(obj->buffers);
}
obj->buffers_cnt = 0;
}
/* }}} */

/* Close and free the zip_t */
static bool php_zipobj_close(ze_zip_object *obj) /* {{{ */
{
struct zip *intern = obj->za;
bool success = false;

if (intern) {
int err = zip_close(intern);
if (err) {
php_error_docref(NULL, E_WARNING, "%s", zip_strerror(intern));
/* Save error for property reader */
zip_error_t *ziperr;

ziperr = zip_get_error(intern);
obj->err_zip = zip_error_code_zip(ziperr);
obj->err_sys = zip_error_code_system(ziperr);
zip_error_fini(ziperr);
zip_discard(intern);
} else {
obj->err_zip = 0;
obj->err_sys = 0;
}
success = !err;
}

// if we have a filename, we need to free it
if (obj->filename) {
/* clear cache as empty zip are not created but deleted */
php_clear_stat_cache(1, obj->filename, obj->filename_len);

efree(obj->filename);
obj->filename = NULL;
obj->filename_len = 0;
}

php_zipobj_release_buffers(obj);

obj->za = NULL;
return success;
}
/* }}} */

int php_zip_glob(zend_string *spattern, zend_long flags, zval *return_value) /* {{{ */
{
int cwd_skip = 0;
Expand Down Expand Up @@ -1021,21 +1091,8 @@ static void php_zip_object_dtor(zend_object *object)
static void php_zip_object_free_storage(zend_object *object) /* {{{ */
{
ze_zip_object * intern = php_zip_fetch_object(object);
int i;

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));
zip_discard(intern->za);
}
}

if (intern->buffers_cnt>0) {
for (i=0; i<intern->buffers_cnt; i++) {
zend_string_release(intern->buffers[i]);
}
efree(intern->buffers);
}
php_zipobj_close(intern);

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

intern->za = NULL;
zend_object_std_dtor(&intern->zo);

if (intern->filename) {
efree(intern->filename);
}
}
/* }}} */

Expand Down Expand Up @@ -1448,19 +1500,8 @@ PHP_METHOD(ZipArchive, open)
RETURN_FALSE;
}

if (ze_obj->za) {
/* we already have an opened zip, free it */
if (zip_close(ze_obj->za) != 0) {
php_error_docref(NULL, E_WARNING, "Empty string as source");
efree(resolved_path);
RETURN_FALSE;
}
ze_obj->za = NULL;
}
if (ze_obj->filename) {
efree(ze_obj->filename);
ze_obj->filename = NULL;
}
// If we already have an opened zip, free it
php_zipobj_close(ze_obj);

/* open for write without option to empty the archive */
if ((flags & (ZIP_TRUNCATE | ZIP_RDONLY)) == 0) {
Expand Down Expand Up @@ -1488,6 +1529,49 @@ PHP_METHOD(ZipArchive, open)
}
/* }}} */

/* {{{ Create new read-only zip using given string */
PHP_METHOD(ZipArchive, openString)
{
struct zip *intern;
zend_string *buffer;
zval *self = ZEND_THIS;

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

ze_zip_object *ze_obj = Z_ZIP_P(self);

zip_error_t err;
zip_error_init(&err);

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

if (!zip_source) {
ze_obj->err_zip = zip_error_code_zip(&err);
ze_obj->err_sys = zip_error_code_system(&err);
zip_error_fini(&err);
RETURN_LONG(ze_obj->err_zip);
}

php_zipobj_close(ze_obj);

intern = zip_open_from_source(zip_source, ZIP_RDONLY, &err);
if (!intern) {
ze_obj->err_zip = zip_error_code_zip(&err);
ze_obj->err_sys = zip_error_code_system(&err);
zip_error_fini(&err);
zip_source_free(zip_source);
RETURN_LONG(ze_obj->err_zip);
}

php_zipobj_add_buffer(ze_obj, buffer);
ze_obj->za = intern;
zip_error_fini(&err);
RETURN_TRUE;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just return 0? Having a return type of int|true is incredibly weird.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency with ZipArchive::open. #14078 proposed to throw exceptions instead, but @remicollet complained that it was inconsistent with ZipArchive::open, so here I am proposing to make it consistent. ZipArchive::open has a couple of error cases which can return false, but they don't apply here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're prepared to take responsibility for this PR and hit the merge button when it meets your requirements, I'll make it return whatever you want.

}
/* }}} */

/* {{{ Set the password for the active archive */
PHP_METHOD(ZipArchive, setPassword)
{
Expand Down Expand Up @@ -1515,42 +1599,14 @@ PHP_METHOD(ZipArchive, close)
{
struct zip *intern;
zval *self = ZEND_THIS;
ze_zip_object *ze_obj;
int err;

if (zend_parse_parameters_none() == FAILURE) {
RETURN_THROWS();
}

ZIP_FROM_OBJECT(intern, self);

ze_obj = Z_ZIP_P(self);

err = zip_close(intern);
if (err) {
php_error_docref(NULL, E_WARNING, "%s", zip_strerror(intern));
/* Save error for property reader */
zip_error_t *ziperr;

ziperr = zip_get_error(intern);
ze_obj->err_zip = zip_error_code_zip(ziperr);
ze_obj->err_sys = zip_error_code_system(ziperr);
zip_error_fini(ziperr);
zip_discard(intern);
} else {
ze_obj->err_zip = 0;
ze_obj->err_sys = 0;
}

/* clear cache as empty zip are not created but deleted */
php_clear_stat_cache(1, ze_obj->filename, ze_obj->filename_len);

efree(ze_obj->filename);
ze_obj->filename = NULL;
ze_obj->filename_len = 0;
ze_obj->za = NULL;

RETURN_BOOL(!err);
RETURN_BOOL(php_zipobj_close(Z_ZIP_P(self)));
}
/* }}} */

Expand Down Expand Up @@ -1864,7 +1920,6 @@ PHP_METHOD(ZipArchive, addFromString)
size_t name_len;
ze_zip_object *ze_obj;
struct zip_source *zs;
int pos = 0;
zend_long flags = ZIP_FL_OVERWRITE;

if (zend_parse_parameters(ZEND_NUM_ARGS(), "sS|l",
Expand All @@ -1875,15 +1930,7 @@ PHP_METHOD(ZipArchive, addFromString)
ZIP_FROM_OBJECT(intern, self);

ze_obj = Z_ZIP_P(self);
if (ze_obj->buffers_cnt) {
ze_obj->buffers = safe_erealloc(ze_obj->buffers, sizeof(*ze_obj->buffers), (ze_obj->buffers_cnt + 1), 0);
pos = ze_obj->buffers_cnt++;
} else {
ze_obj->buffers = emalloc(sizeof(*ze_obj->buffers));
ze_obj->buffers_cnt++;
pos = 0;
}
ze_obj->buffers[pos] = zend_string_copy(buffer);
php_zipobj_add_buffer(ze_obj, buffer);

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

Expand Down
2 changes: 2 additions & 0 deletions ext/zip/php_zip.stub.php
Original file line number Diff line number Diff line change
Expand Up @@ -646,6 +646,8 @@ class ZipArchive implements Countable
/** @tentative-return-type */
public function open(string $filename, int $flags = 0): bool|int {}

public function openString(string $data): true|int {}

/**
* @tentative-return-type
*/
Expand Down
8 changes: 7 additions & 1 deletion ext/zip/php_zip_arginfo.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

28 changes: 28 additions & 0 deletions ext/zip/tests/ZipArchive_openString.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
--TEST--
ZipArchive::openString() method
--EXTENSIONS--
zip
--FILE--
<?php
$zip = new ZipArchive();
$zip->openString(file_get_contents(__DIR__."/test_procedural.zip"));

for ($i = 0; $i < $zip->numFiles; $i++) {
$stat = $zip->statIndex($i);
echo $stat['name'] . "\n";
}

// Zip is read-only, not allowed
var_dump($zip->addFromString("foobar/baz", "baz"));
var_dump($zip->addEmptyDir("blub"));

var_dump($zip->close());
?>
--EXPECTF--
foo
bar
foobar/
foobar/baz
bool(false)
bool(false)
bool(true)
25 changes: 25 additions & 0 deletions ext/zip/tests/ZipArchive_openString_leak.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
--TEST--
ZipArchive::openString leak tests
--EXTENSIONS--
zip
--FILE--
<?php
$zip = new ZipArchive;
$zip->openString(file_get_contents(__DIR__ . '/test.zip'));
$zip = null;

$zip = new ZipArchive;
$zip->openString(file_get_contents(__DIR__ . '/test.zip'));
$zip->openString(file_get_contents(__DIR__ . '/test.zip'));
$zip = null;

$zip = new ZipArchive;
$zip->openString(file_get_contents(__DIR__ . '/test.zip'));
$zip->open(__DIR__ . '/test.zip');
$zip = null;

$zip = new ZipArchive;
$zip->open(__DIR__ . '/test.zip');
$zip->openString(file_get_contents(__DIR__ . '/test.zip'));
$zip = null;
--EXPECT--
22 changes: 22 additions & 0 deletions ext/zip/tests/ZipArchive_open_with_close_fail.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
--TEST--
zip_close() failure from ZipArchive::open()
--EXTENSIONS--
zip
--FILE--
<?php
// Try to create an archive
$zip = new ZipArchive();
var_dump($zip->open(__DIR__ . '/close-fail.zip', ZipArchive::CREATE));
file_put_contents(__DIR__.'/close-fail-file', '');
$zip->addFile(__DIR__.'/close-fail-file');
// Delete the file to trigger an error on close
@unlink(__DIR__.'/close-fail-file');
var_dump($zip->open(__DIR__ . '/test.zip', ZipArchive::RDONLY));
var_dump($zip->getFromName('bar'));
--EXPECTF--
bool(true)

Warning: ZipArchive::open(): Can't open file: No such file or directory in %s on line %d
bool(true)
string(4) "bar
"
Loading