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
180 changes: 109 additions & 71 deletions ext/zip/php_zip.c
Original file line number Diff line number Diff line change
Expand Up @@ -575,6 +575,68 @@ 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) /* {{{ */
{
size_t pos = obj->buffers_cnt++;
obj->buffers = safe_erealloc(obj->buffers, sizeof(*obj->buffers), obj->buffers_cnt, 0);
obj->buffers[pos] = zend_string_copy(str);
}
/* }}} */

static void php_zipobj_release_buffers(ze_zip_object *obj) /* {{{ */
{
if (obj->buffers_cnt > 0) {
for (size_t i = 0; i < obj->buffers_cnt; i++) {
zend_string_release(obj->buffers[i]);
}
efree(obj->buffers);
obj->buffers = NULL;
}
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 = 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 +1083,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 +1096,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 +1492,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 +1521,48 @@ PHP_METHOD(ZipArchive, open)
}
/* }}} */

/* {{{ Create new read-only zip using given string */
PHP_METHOD(ZipArchive, openString)
{
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);
Copy link
Member

Choose a reason for hiding this comment

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

I'd use Z_ZIP_P(ZEND_THIS) instead of storing ZEND_THIS in a variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Every method in this class uses this pattern. I can clean it up in a separate PR if you like. Maybe it makes sense for performance if ZEND_THIS is accessed more than once, but the original author decided to be consistent about it regardless of how many times it's accessed.


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);

struct zip *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.

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 forwards compatibility and consistency with ZipArchive::open, it would be better if the method was declared as returning int|bool, not int|true. Callers should check for a false return in case some future version returns false. But I don't care enough to delay the merge. Daniel asked for int|true, so it returns int|true.

Copy link
Member

Choose a reason for hiding this comment

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

If there is a reason to use int|bool, i.e. because you want to be able to return false in some future version, I have no objections - I just wanted to make sure that if there wasn't a reason to have bool in the type, that it would be narrowed, so that people wouldn't check for false values unnecessarily - future compatibility is a valid reason

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I changed it to bool|int.

}
/* }}} */

/* {{{ Set the password for the active archive */
PHP_METHOD(ZipArchive, setPassword)
{
Expand Down Expand Up @@ -1515,42 +1590,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 +1911,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 +1921,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
4 changes: 2 additions & 2 deletions ext/zip/php_zip.h
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,8 @@ typedef struct _ze_zip_object {
zend_string **buffers;
HashTable *prop_handler;
char *filename;
int filename_len;
int buffers_cnt;
size_t filename_len;
size_t buffers_cnt;
zip_int64_t last_id;
int err_zip;
int err_sys;
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): bool|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)
26 changes: 26 additions & 0 deletions ext/zip/tests/ZipArchive_openString_leak.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
--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--
23 changes: 23 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,23 @@
--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