Skip to content

Add ZipArchive::closeString() #21497

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tstarling wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add ZipArchive::closeString() #21497

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
72 changes: 61 additions & 11 deletions ext/zip/php_zip.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -573,8 +573,12 @@ static char * php_zipobj_get_zip_comment(ze_zip_object *obj, int *len) /* {{{ */
}
/* }}} */

/* Close and free the zip_t */
static bool php_zipobj_close(ze_zip_object *obj) /* {{{ */
/* Close and free the zip_t. If the archive was opened as a string, the
* final contents of the archive will be assigned to *out_str and that
* string will afterwards be owned by the caller.
*
* If out_str is NULL, the final string contents, if any, will be discarded. */
static bool php_zipobj_close(ze_zip_object *obj, zend_string **out_str) /* {{{ */
{
struct zip *intern = obj->za;
bool success = false;
Expand Down Expand Up @@ -606,7 +610,19 @@ static bool php_zipobj_close(ze_zip_object *obj) /* {{{ */
obj->filename_len = 0;
}

if (obj->out_str) {
if (out_str) {
*out_str = obj->out_str;
} else {
zend_string_release(obj->out_str);
}
obj->out_str = NULL;
} else {
ZEND_ASSERT(!out_str);
}
Copy link
Copy Markdown
Member

@DanielEScherzer DanielEScherzer Apr 19, 2026

Choose a reason for hiding this comment

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

if obj->out_str is not set, we should ensure that callers do not try to use the out_str, suggest adding

Suggested change
}
} else {
ZEND_ASSERT(!out_str);
}

Copy link
Copy Markdown
Contributor Author

@tstarling tstarling May 21, 2026

Choose a reason for hiding this comment

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

Done.

Copy link
Copy Markdown
Contributor Author

@tstarling tstarling May 21, 2026

Choose a reason for hiding this comment

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

I had to revert this, since the assertion fails when closeString() is called but something goes wrong with saving the archive. The caller is already guarding for that, and false is returned to userspace. There was already a test, which failed due to the assertion.

Copy link
Copy Markdown
Member

@DanielEScherzer DanielEScherzer May 24, 2026

Choose a reason for hiding this comment

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

can you add a comment explaining that here? Maybe something like

out_str can be passed even when obj->out_str is not set, in which case out_str will not be changed

Copy link
Copy Markdown
Contributor Author

@tstarling tstarling May 28, 2026

Choose a reason for hiding this comment

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

In reviewing zip_close() to write a response to this, I found a surprising special case. If you open an archive and close it without changing it, obj->out_str was NULL and ZipArchive::closeString() was returning an empty string. I fixed it by copying the input string to obj->out_str when the archive is opened. So obj->out_str is always set now, as long as the archive was opened with ZipArchive::openString().

With this change, I was able to re-add your suggested assertion.

I added a test to ensure that ZipArchive::closeString() will return the unchanged input string when no changes are made.


obj->za = NULL;
obj->from_string = false;
return success;
}
/* }}} */
Expand Down Expand Up @@ -1060,7 +1076,7 @@ static void php_zip_object_free_storage(zend_object *object) /* {{{ */
{
ze_zip_object * intern = php_zip_fetch_object(object);

php_zipobj_close(intern);
php_zipobj_close(intern, NULL);

#ifdef HAVE_PROGRESS_CALLBACK
/* if not properly called by libzip */
Expand Down Expand Up @@ -1467,7 +1483,7 @@ PHP_METHOD(ZipArchive, open)
}

/* If we already have an opened zip, free it */
php_zipobj_close(ze_obj);
php_zipobj_close(ze_obj, NULL);

/* open for write without option to empty the archive */
if ((flags & (ZIP_TRUNCATE | ZIP_RDONLY)) == 0) {
Expand All @@ -1491,28 +1507,34 @@ PHP_METHOD(ZipArchive, open)
ze_obj->filename = resolved_path;
ze_obj->filename_len = strlen(resolved_path);
ze_obj->za = intern;
ze_obj->from_string = false;
RETURN_TRUE;
}
/* }}} */

/* {{{ Create new read-only zip using given string */
/* {{{ Create new zip from a string, or a create an empty zip to be saved to a string */
PHP_METHOD(ZipArchive, openString)
{
zend_string *buffer;
zend_string *buffer = NULL;
zend_long flags = 0;
zval *self = ZEND_THIS;

if (zend_parse_parameters(ZEND_NUM_ARGS(), "S", &buffer) == FAILURE) {
if (zend_parse_parameters(ZEND_NUM_ARGS(), "|Sl", &buffer, &flags) == FAILURE) {
Comment thread
DanielEScherzer marked this conversation as resolved.
RETURN_THROWS();
}

if (!buffer) {
buffer = ZSTR_EMPTY_ALLOC();
}

ze_zip_object *ze_obj = Z_ZIP_P(self);

php_zipobj_close(ze_obj);
php_zipobj_close(ze_obj, NULL);

zip_error_t err;
zip_error_init(&err);

zip_source_t * zip_source = php_zip_create_string_source(buffer, NULL, &err);
zip_source_t * zip_source = php_zip_create_string_source(buffer, &ze_obj->out_str, &err);

if (!zip_source) {
ze_obj->err_zip = zip_error_code_zip(&err);
Expand All @@ -1521,7 +1543,7 @@ PHP_METHOD(ZipArchive, openString)
RETURN_LONG(ze_obj->err_zip);
}

struct zip *intern = zip_open_from_source(zip_source, ZIP_RDONLY, &err);
struct zip *intern = zip_open_from_source(zip_source, flags, &err);
Comment thread
tstarling marked this conversation as resolved.
if (!intern) {
ze_obj->err_zip = zip_error_code_zip(&err);
ze_obj->err_sys = zip_error_code_system(&err);
Expand All @@ -1530,6 +1552,7 @@ PHP_METHOD(ZipArchive, openString)
RETURN_LONG(ze_obj->err_zip);
}

ze_obj->from_string = true;
ze_obj->za = intern;
zip_error_fini(&err);
RETURN_TRUE;
Expand Down Expand Up @@ -1568,7 +1591,34 @@ PHP_METHOD(ZipArchive, close)

ZIP_FROM_OBJECT(intern, self);

RETURN_BOOL(php_zipobj_close(Z_ZIP_P(self)));
RETURN_BOOL(php_zipobj_close(Z_ZIP_P(self), NULL));
}
/* }}} */

/* {{{ close the zip archive and get the result as a string */
PHP_METHOD(ZipArchive, closeString)
{
struct zip *intern;
zval *self = ZEND_THIS;

ZEND_PARSE_PARAMETERS_NONE();

ZIP_FROM_OBJECT(intern, self);

Comment thread
DanielEScherzer marked this conversation as resolved.
if (!Z_ZIP_P(self)->from_string) {
zend_throw_error(NULL, "ZipArchive::closeString can only be called on "
"an archive opened with ZipArchive::openString");
RETURN_THROWS();
}

zend_string *ret = NULL;
bool success = php_zipobj_close(Z_ZIP_P(self), &ret);
ZEND_ASSERT(ret);
if (success) {
RETURN_STR(ret);
}
zend_string_release(ret);
RETURN_FALSE;
}
/* }}} */

Expand Down
2 changes: 2 additions & 0 deletions ext/zip/php_zip.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ typedef struct _ze_zip_object {
HashTable *prop_handler;
char *filename;
size_t filename_len;
zend_string *out_str;
bool from_string;
zip_int64_t last_id;
int err_zip;
int err_sys;
Expand Down
4 changes: 3 additions & 1 deletion ext/zip/php_zip.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -646,7 +646,7 @@ class ZipArchive implements Countable
/** @tentative-return-type */
public function open(string $filename, int $flags = 0): bool|int {}

public function openString(string $data): bool|int {}
public function openString(string $data = '', int $flags = 0): bool|int {}

/**
* @tentative-return-type
Expand All @@ -656,6 +656,8 @@ public function setPassword(#[\SensitiveParameter] string $password): bool {}
/** @tentative-return-type */
public function close(): bool {}

public function closeString(): string|false {}

/** @tentative-return-type */
public function count(): int {}

Expand Down
12 changes: 9 additions & 3 deletions ext/zip/php_zip_arginfo.h
View file
Open in desktop

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

25 changes: 25 additions & 0 deletions ext/zip/tests/ZipArchive_closeString_basic.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
--TEST--
ZipArchive::closeString() basic
--EXTENSIONS--
zip
--FILE--
<?php
$zip = new ZipArchive();
$zip->openString();
$zip->addFromString('test1', '1');
$zip->addFromString('test2', '2');
$contents = $zip->closeString();
echo $contents ? "OK\n" : "FAILED\n";

$zip = new ZipArchive();
$zip->openString($contents);
var_dump($zip->getFromName('test1'));
var_dump($zip->getFromName('test2'));
var_dump($zip->getFromName('nonexistent'));

?>
--EXPECT--
OK
string(1) "1"
string(1) "2"
bool(false)
47 changes: 47 additions & 0 deletions ext/zip/tests/ZipArchive_closeString_error.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
--TEST--
ZipArchive::closeString() error cases
--EXTENSIONS--
zip
--FILE--
<?php
echo "1.\n";
$zip = new ZipArchive();
$zip->openString();
var_dump($zip->open(__DIR__ . '/test.zip'));
try {
$zip->closeString();
} catch (Error $e) {
echo $e->getMessage() . "\n";
}

echo "2.\n";
$zip = new ZipArchive();
$zip->openString('...');
echo $zip->getStatusString() . "\n";
try {
$zip->closeString();
} catch (Error $e) {
echo $e->getMessage() . "\n";
}

echo "3.\n";
$zip = new ZipArchive();
$zip->openString(file_get_contents(__DIR__ . '/test.zip'));
echo gettype($zip->closeString()) . "\n";
try {
$zip->closeString();
} catch (Error $e) {
echo $e->getMessage() . "\n";
}

?>
--EXPECT--
1.
bool(true)
ZipArchive::closeString can only be called on an archive opened with ZipArchive::openString
2.
Not a zip archive
Invalid or uninitialized Zip object
3.
string
Invalid or uninitialized Zip object
22 changes: 22 additions & 0 deletions ext/zip/tests/ZipArchive_closeString_false.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
--TEST--
ZipArchive::closeString() false return
--EXTENSIONS--
zip
--FILE--
<?php
$zip = new ZipArchive();
// The "compressed size" fields are wrong, causing an error when reading the contents.
// The error is reported on close when we rewrite the member with setCompressionIndex().
// The error code is ER_DATA_LENGTH in libzip 1.10.0+ or ER_INCONS otherwise.
$input = file_get_contents(__DIR__ . '/wrong-file-size.zip');
var_dump($zip->openString($input));
$zip->setCompressionIndex(0, ZipArchive::CM_DEFLATE);
var_dump($zip->closeString());
echo $zip->getStatusString() . "\n";
?>
--EXPECTREGEX--
bool\(true\)

Warning: ZipArchive::closeString\(\): (Zip archive inconsistent|Unexpected length of data).*
bool\(false\)
(Zip archive inconsistent|Unexpected length of data)
39 changes: 39 additions & 0 deletions ext/zip/tests/ZipArchive_closeString_variation.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
--TEST--
ZipArchive::closeString() variations
Comment thread
DanielEScherzer marked this conversation as resolved.
--EXTENSIONS--
zip
--FILE--
<?php
echo "1. Empty archive creation\n";
$zip = new ZipArchive();
$zip->openString();
var_dump($zip->closeString());
echo $zip->getStatusString() . "\n";

echo "2. Update existing archive\n";
$input = file_get_contents(__DIR__ . '/test.zip');
$zip = new ZipArchive();
$zip->openString($input);
$zip->addFromString('entry1.txt', '');
$result = $zip->closeString();
echo gettype($result) . "\n";
var_dump($input !== $result);

echo "3. Unchanged existing archive\n";
$zip = new ZipArchive();
$zip->openString($input);
$result = $zip->closeString();
echo gettype($result) . "\n";
var_dump($input === $result);

?>
--EXPECT--
1. Empty archive creation
string(0) ""
No error
2. Update existing archive
string
bool(true)
3. Unchanged existing archive
string
bool(true)
Loading
Loading