Skip to content

Commit 9899601

Browse files
khaledalamkhaledalam
committed
Fix GH-22885: SEGV when setting the comment of a modified zip entry
1 parent 7ad79be commit 9899601

3 files changed

Lines changed: 253 additions & 17 deletions

File tree

‎NEWS‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,10 @@ PHP NEWS
7171
. Fixed bug GH-15836 (Use-after-free when a user stream filter accesses
7272
$this->stream during the close flush). (iliaal)
7373

74+
- Zip:
75+
. Fixed bug GH-22885 (SEGV when setting the comment of an entry that has been
76+
modified). (Khaled Alam)
77+
7478
16 Jul 2026, PHP 8.6.0alpha2
7579

7680
- Core:

‎ext/zip/php_zip.c‎

Lines changed: 173 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -79,19 +79,103 @@ static bool php_zip_set_file_comment(struct zip *za, zip_uint64_t index, const c
7979
}
8080
/* }}} */
8181

82+
/* {{{ php_zip_comment_detach / php_zip_comment_restore
83+
* Modifying an entry (replacing its data, renaming it, changing its compression,
84+
* ...) makes libzip clone the central directory record of that entry, and the
85+
* clone shares the comment string with the original record. Up to libzip 1.11.4
86+
* zip_file_set_comment() then frees that shared string unconditionally, leaving
87+
* the original record with a dangling pointer: a use-after-free, followed by a
88+
* double free when the archive is closed.
89+
* Detaching the comment before the record gets cloned makes the clone own its
90+
* comment, so that libzip only ever frees a string of its own. Both helpers are
91+
* no-ops for entries without a comment, and they leave the error state of the
92+
* archive untouched. */
93+
static zend_string *php_zip_comment_detach(struct zip *za, zip_int64_t index)
94+
{
95+
zip_error_t *err;
96+
int zip_err, sys_err;
97+
zip_uint32_t comment_len = 0;
98+
const char *comment;
99+
zend_string *saved;
100+
101+
if (index < 0) {
102+
return NULL;
103+
}
104+
105+
err = zip_get_error(za);
106+
zip_err = zip_error_code_zip(err);
107+
sys_err = zip_error_code_system(err);
108+
109+
comment = zip_file_get_comment(za, (zip_uint64_t) index, &comment_len, ZIP_FL_ENC_RAW);
110+
if (comment == NULL || comment_len == 0) {
111+
saved = NULL;
112+
} else {
113+
saved = zend_string_init(comment, comment_len, false);
114+
if (zip_file_set_comment(za, (zip_uint64_t) index, NULL, 0, 0) < 0) {
115+
zend_string_release_ex(saved, false);
116+
saved = NULL;
117+
}
118+
}
119+
120+
zip_error_set(err, zip_err, sys_err);
121+
122+
return saved;
123+
}
124+
125+
static zend_string *php_zip_comment_detach_name(struct zip *za, const char *name, zip_flags_t flags, zip_int64_t *index)
126+
{
127+
zip_error_t *err = zip_get_error(za);
128+
int zip_err = zip_error_code_zip(err);
129+
int sys_err = zip_error_code_system(err);
130+
131+
/* Only an overwritten entry can already be part of the archive. */
132+
*index = (flags & ZIP_FL_OVERWRITE) ? zip_name_locate(za, name, flags) : -1;
133+
134+
zip_error_set(err, zip_err, sys_err);
135+
136+
return php_zip_comment_detach(za, *index);
137+
}
138+
139+
static void php_zip_comment_restore(struct zip *za, zip_int64_t index, zend_string *comment)
140+
{
141+
zip_error_t *err;
142+
int zip_err, sys_err;
143+
144+
if (comment == NULL) {
145+
return;
146+
}
147+
148+
err = zip_get_error(za);
149+
zip_err = zip_error_code_zip(err);
150+
sys_err = zip_error_code_system(err);
151+
152+
zip_file_set_comment(za, (zip_uint64_t) index, ZSTR_VAL(comment), (zip_uint16_t) ZSTR_LEN(comment), 0);
153+
154+
zip_error_set(err, zip_err, sys_err);
155+
zend_string_release_ex(comment, false);
156+
}
157+
/* }}} */
158+
82159
# define add_ascii_assoc_string add_assoc_string
83160
# define add_ascii_assoc_long add_assoc_long
84161

85162
#ifdef HAVE_ENCRYPTION
86163
static bool php_zip_file_set_encryption(struct zip *intern, zend_long index, zend_long method, char *password) {
164+
zend_string *comment = php_zip_comment_detach(intern, (zip_int64_t)index);
165+
bool result;
166+
87167
// FIXME: is a workaround to reset/free the password in case of consecutive calls.
88168
// when libzip 1.11.5 is available, we can save this call in this case.
89169
if (UNEXPECTED(zip_file_set_encryption(intern, (zip_uint64_t)index, ZIP_EM_NONE, NULL) < 0)) {
170+
php_zip_comment_restore(intern, (zip_int64_t)index, comment);
90171
php_error_docref(NULL, E_WARNING, "password reset failed");
91172
return false;
92173
}
93174

94-
return (zip_file_set_encryption(intern, (zip_uint64_t)index, (zip_uint16_t)method, password) == 0);
175+
result = (zip_file_set_encryption(intern, (zip_uint64_t)index, (zip_uint16_t)method, password) == 0);
176+
php_zip_comment_restore(intern, (zip_int64_t)index, comment);
177+
178+
return result;
95179
}
96180
#endif
97181

@@ -298,6 +382,8 @@ static zend_result php_zip_add_file(ze_zip_object *obj, const char *filename, si
298382
struct zip_source *zs;
299383
char resolved_path[MAXPATHLEN];
300384
php_stream_statbuf ssb;
385+
zend_string *comment;
386+
zip_int64_t overwritten = -1;
301387

302388
if (ZIP_OPENBASEDIR_CHECKPATH(filename)) {
303389
return FAILURE;
@@ -333,15 +419,24 @@ static zend_result php_zip_add_file(ze_zip_object *obj, const char *filename, si
333419
}
334420
/* Replace */
335421
if (replace >= 0) {
336-
if (zip_file_replace(obj->za, replace, zs, flags) < 0) {
422+
int res;
423+
424+
comment = php_zip_comment_detach(obj->za, replace);
425+
res = zip_file_replace(obj->za, replace, zs, flags);
426+
php_zip_comment_restore(obj->za, replace, comment);
427+
428+
if (res < 0) {
337429
zip_source_free(zs);
338430
return FAILURE;
339431
}
340432
zip_error_clear(obj->za);
341433
return SUCCESS;
342434
}
343435
/* Add */
436+
comment = php_zip_comment_detach_name(obj->za, entry_name, flags, &overwritten);
437+
344438
obj->last_id = zip_file_add(obj->za, entry_name, zs, flags);
439+
php_zip_comment_restore(obj->za, overwritten, comment);
345440
if (obj->last_id < 0) {
346441
zip_source_free(zs);
347442
return FAILURE;
@@ -1751,6 +1846,8 @@ PHP_METHOD(ZipArchive, addEmptyDir)
17511846
char *dirname;
17521847
size_t dirname_len;
17531848
char *s;
1849+
zend_string *comment;
1850+
zip_int64_t overwritten = -1;
17541851
zend_long flags = 0;
17551852

17561853
if (zend_parse_parameters(ZEND_NUM_ARGS(), "s|l",
@@ -1770,13 +1867,17 @@ PHP_METHOD(ZipArchive, addEmptyDir)
17701867
s = dirname;
17711868
}
17721869

1870+
comment = php_zip_comment_detach_name(intern, (const char *)s, (zip_flags_t)flags, &overwritten);
1871+
17731872
if ((Z_ZIP_P(self)->last_id = zip_dir_add(intern, (const char *)s, flags)) == -1) {
17741873
RETVAL_FALSE;
17751874
} else {
17761875
zip_error_clear(intern);
17771876
RETVAL_TRUE;
17781877
}
17791878

1879+
php_zip_comment_restore(intern, overwritten, comment);
1880+
17801881
if (s != dirname) {
17811882
efree(s);
17821883
}
@@ -1983,6 +2084,8 @@ PHP_METHOD(ZipArchive, addFromString)
19832084
size_t name_len;
19842085
ze_zip_object *ze_obj;
19852086
struct zip_source *zs;
2087+
zend_string *comment;
2088+
zip_int64_t overwritten = -1;
19862089
zend_long flags = ZIP_FL_OVERWRITE;
19872090

19882091
if (zend_parse_parameters(ZEND_NUM_ARGS(), "sS|l",
@@ -1999,7 +2102,10 @@ PHP_METHOD(ZipArchive, addFromString)
19992102
RETURN_FALSE;
20002103
}
20012104

2105+
comment = php_zip_comment_detach_name(intern, name, (zip_flags_t)flags, &overwritten);
2106+
20022107
ze_obj->last_id = zip_file_add(intern, name, zs, flags);
2108+
php_zip_comment_restore(intern, overwritten, comment);
20032109
if (ze_obj->last_id == -1) {
20042110
zip_source_free(zs);
20052111
RETURN_FALSE;
@@ -2260,6 +2366,8 @@ PHP_METHOD(ZipArchive, setExternalAttributesName)
22602366
char *name;
22612367
zend_long flags=0, opsys, attr;
22622368
zip_int64_t idx;
2369+
zend_string *comment;
2370+
bool result;
22632371

22642372
if (zend_parse_parameters(ZEND_NUM_ARGS(), "sll|l",
22652373
&name, &name_len, &opsys, &attr, &flags) == FAILURE) {
@@ -2275,11 +2383,16 @@ PHP_METHOD(ZipArchive, setExternalAttributesName)
22752383

22762384
idx = zip_name_locate(intern, name, 0);
22772385

2278-
if (idx < 0 || zip_file_set_external_attributes(intern, idx, (zip_flags_t)flags,
2279-
(zip_uint8_t)(opsys&0xff), (zip_uint32_t)attr) < 0) {
2386+
if (idx < 0) {
22802387
RETURN_FALSE;
22812388
}
2282-
RETURN_TRUE;
2389+
2390+
comment = php_zip_comment_detach(intern, idx);
2391+
result = zip_file_set_external_attributes(intern, idx, (zip_flags_t)flags,
2392+
(zip_uint8_t)(opsys&0xff), (zip_uint32_t)attr) == 0;
2393+
php_zip_comment_restore(intern, idx, comment);
2394+
2395+
RETURN_BOOL(result);
22832396
}
22842397
/* }}} */
22852398

@@ -2290,6 +2403,8 @@ PHP_METHOD(ZipArchive, setExternalAttributesIndex)
22902403
zval *self = ZEND_THIS;
22912404
zend_long index, flags=0, opsys, attr;
22922405
struct zip_stat sb;
2406+
zend_string *comment;
2407+
bool result;
22932408

22942409
if (zend_parse_parameters(ZEND_NUM_ARGS(), "lll|l",
22952410
&index, &opsys, &attr, &flags) == FAILURE) {
@@ -2299,8 +2414,13 @@ PHP_METHOD(ZipArchive, setExternalAttributesIndex)
22992414
ZIP_FROM_OBJECT(intern, self);
23002415

23012416
PHP_ZIP_STAT_INDEX(intern, index, 0, sb);
2302-
RETURN_BOOL(zip_file_set_external_attributes(intern, (zip_uint64_t)index,
2303-
(zip_flags_t)flags, (zip_uint8_t)(opsys&0xff), (zip_uint32_t)attr) == 0);
2417+
2418+
comment = php_zip_comment_detach(intern, (zip_int64_t)index);
2419+
result = zip_file_set_external_attributes(intern, (zip_uint64_t)index,
2420+
(zip_flags_t)flags, (zip_uint8_t)(opsys&0xff), (zip_uint32_t)attr) == 0;
2421+
php_zip_comment_restore(intern, (zip_int64_t)index, comment);
2422+
2423+
RETURN_BOOL(result);
23042424
}
23052425
/* }}} */
23062426

@@ -2490,6 +2610,8 @@ PHP_METHOD(ZipArchive, setCompressionName)
24902610
char *name;
24912611
zip_int64_t idx;
24922612
zend_long comp_method, comp_flags = 0;
2613+
zend_string *comment;
2614+
bool result;
24932615

24942616
if (zend_parse_parameters(ZEND_NUM_ARGS(), "sl|l",
24952617
&name, &name_len, &comp_method, &comp_flags) == FAILURE) {
@@ -2519,8 +2641,12 @@ PHP_METHOD(ZipArchive, setCompressionName)
25192641
RETURN_FALSE;
25202642
}
25212643

2522-
RETURN_BOOL(zip_set_file_compression(intern, (zip_uint64_t)idx,
2523-
(zip_int32_t)comp_method, (zip_uint32_t)comp_flags) == 0);
2644+
comment = php_zip_comment_detach(intern, idx);
2645+
result = zip_set_file_compression(intern, (zip_uint64_t)idx,
2646+
(zip_int32_t)comp_method, (zip_uint32_t)comp_flags) == 0;
2647+
php_zip_comment_restore(intern, idx, comment);
2648+
2649+
RETURN_BOOL(result);
25242650
}
25252651
/* }}} */
25262652

@@ -2531,6 +2657,8 @@ PHP_METHOD(ZipArchive, setCompressionIndex)
25312657
zval *this = ZEND_THIS;
25322658
zend_long index;
25332659
zend_long comp_method, comp_flags = 0;
2660+
zend_string *comment;
2661+
bool result;
25342662

25352663
if (zend_parse_parameters(ZEND_NUM_ARGS(), "ll|l",
25362664
&index, &comp_method, &comp_flags) == FAILURE) {
@@ -2554,8 +2682,12 @@ PHP_METHOD(ZipArchive, setCompressionIndex)
25542682

25552683
ZIP_FROM_OBJECT(intern, this);
25562684

2557-
RETURN_BOOL(zip_set_file_compression(intern, (zip_uint64_t)index,
2558-
(zip_int32_t)comp_method, (zip_uint32_t)comp_flags) == 0);
2685+
comment = php_zip_comment_detach(intern, (zip_int64_t)index);
2686+
result = zip_set_file_compression(intern, (zip_uint64_t)index,
2687+
(zip_int32_t)comp_method, (zip_uint32_t)comp_flags) == 0;
2688+
php_zip_comment_restore(intern, (zip_int64_t)index, comment);
2689+
2690+
RETURN_BOOL(result);
25592691
}
25602692
/* }}} */
25612693

@@ -2568,6 +2700,8 @@ PHP_METHOD(ZipArchive, setMtimeName)
25682700
char *name;
25692701
zip_int64_t idx;
25702702
zend_long mtime, flags = 0;
2703+
zend_string *comment;
2704+
bool result;
25712705

25722706
if (zend_parse_parameters(ZEND_NUM_ARGS(), "sl|l",
25732707
&name, &name_len, &mtime, &flags) == FAILURE) {
@@ -2587,8 +2721,12 @@ PHP_METHOD(ZipArchive, setMtimeName)
25872721
RETURN_FALSE;
25882722
}
25892723

2590-
RETURN_BOOL(zip_file_set_mtime(intern, (zip_uint64_t)idx,
2591-
(time_t)mtime, (zip_uint32_t)flags) == 0);
2724+
comment = php_zip_comment_detach(intern, idx);
2725+
result = zip_file_set_mtime(intern, (zip_uint64_t)idx,
2726+
(time_t)mtime, (zip_uint32_t)flags) == 0;
2727+
php_zip_comment_restore(intern, idx, comment);
2728+
2729+
RETURN_BOOL(result);
25922730
}
25932731
/* }}} */
25942732

@@ -2599,6 +2737,8 @@ PHP_METHOD(ZipArchive, setMtimeIndex)
25992737
zval *this = ZEND_THIS;
26002738
zend_long index;
26012739
zend_long mtime, flags = 0;
2740+
zend_string *comment;
2741+
bool result;
26022742

26032743
if (zend_parse_parameters(ZEND_NUM_ARGS(), "ll|l",
26042744
&index, &mtime, &flags) == FAILURE) {
@@ -2607,8 +2747,12 @@ PHP_METHOD(ZipArchive, setMtimeIndex)
26072747

26082748
ZIP_FROM_OBJECT(intern, this);
26092749

2610-
RETURN_BOOL(zip_file_set_mtime(intern, (zip_uint64_t)index,
2611-
(time_t)mtime, (zip_uint32_t)flags) == 0);
2750+
comment = php_zip_comment_detach(intern, (zip_int64_t)index);
2751+
result = zip_file_set_mtime(intern, (zip_uint64_t)index,
2752+
(time_t)mtime, (zip_uint32_t)flags) == 0;
2753+
php_zip_comment_restore(intern, (zip_int64_t)index, comment);
2754+
2755+
RETURN_BOOL(result);
26122756
}
26132757
/* }}} */
26142758

@@ -2666,6 +2810,8 @@ PHP_METHOD(ZipArchive, renameIndex)
26662810
char *new_name;
26672811
size_t new_name_len;
26682812
zend_long index;
2813+
zend_string *comment;
2814+
bool result;
26692815

26702816
if (zend_parse_parameters(ZEND_NUM_ARGS(), "ls", &index, &new_name, &new_name_len) == FAILURE) {
26712817
RETURN_THROWS();
@@ -2682,7 +2828,11 @@ PHP_METHOD(ZipArchive, renameIndex)
26822828

26832829
ZIP_FROM_OBJECT(intern, self);
26842830

2685-
RETURN_BOOL(zip_file_rename(intern, index, (const char *)new_name, 0) == 0);
2831+
comment = php_zip_comment_detach(intern, (zip_int64_t)index);
2832+
result = zip_file_rename(intern, index, (const char *)new_name, 0) == 0;
2833+
php_zip_comment_restore(intern, (zip_int64_t)index, comment);
2834+
2835+
RETURN_BOOL(result);
26862836
}
26872837
/* }}} */
26882838

@@ -2694,6 +2844,8 @@ PHP_METHOD(ZipArchive, renameName)
26942844
struct zip_stat sb;
26952845
char *name, *new_name;
26962846
size_t name_len, new_name_len;
2847+
zend_string *comment;
2848+
bool result;
26972849

26982850
if (zend_parse_parameters(ZEND_NUM_ARGS(), "ss", &name, &name_len, &new_name, &new_name_len) == FAILURE) {
26992851
RETURN_THROWS();
@@ -2708,7 +2860,11 @@ PHP_METHOD(ZipArchive, renameName)
27082860

27092861
PHP_ZIP_STAT_PATH(intern, name, name_len, 0, sb);
27102862

2711-
RETURN_BOOL(zip_file_rename(intern, sb.index, (const char *)new_name, 0) == 0);
2863+
comment = php_zip_comment_detach(intern, (zip_int64_t)sb.index);
2864+
result = zip_file_rename(intern, sb.index, (const char *)new_name, 0) == 0;
2865+
php_zip_comment_restore(intern, (zip_int64_t)sb.index, comment);
2866+
2867+
RETURN_BOOL(result);
27122868
}
27132869
/* }}} */
27142870

0 commit comments

Comments
 (0)