Skip to content

Commit c8b9e32

Browse files
khaledalamkhaledalam
committed
Fix GH-22885: SEGV when setting the comment of a modified zip entry
1 parent 68d605f commit c8b9e32

4 files changed

Lines changed: 237 additions & 8 deletions

File tree

‎NEWS‎

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

26+
- Zip:
27+
. Fixed bug GH-22885 (SEGV when setting the comment of an entry that has been
28+
modified). (Khaled Alam)
29+
2630
30 Jul 2026, PHP 8.4.24
2731

2832
- Calendar:

‎ext/zip/php_zip.c‎

Lines changed: 128 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -64,25 +64,106 @@ static int le_zip_entry;
6464
}
6565
/* }}} */
6666

67+
/* {{{ Bookkeeping of the entries libzip has copied the central directory record of.
68+
* Modifying an entry makes libzip copy its central directory record, and the copy
69+
* shares the comment string with the record of the entry. As zip_file_set_comment()
70+
* frees that string unconditionally, setting the comment of a modified entry leaves
71+
* the entry with a dangling comment: a use-after-free, and a double free when the
72+
* archive is closed. The comment of such an entry cannot be replaced from the
73+
* outside either, because libzip only keeps an unchanged comment as the shared
74+
* string, so ext/zip tracks the modified entries and refuses instead.
75+
* Fixed upstream by nih-at/libzip@17f10b9, unreleased as of libzip 1.11.4; this
76+
* can go once that is the minimum supported version. */
77+
static void php_zip_mark_entry_modified(ze_zip_object *obj, zip_int64_t index)
78+
{
79+
if (index < 0) {
80+
return;
81+
}
82+
83+
if (!obj->modified_entries) {
84+
ALLOC_HASHTABLE(obj->modified_entries);
85+
zend_hash_init(obj->modified_entries, 0, NULL, NULL, false);
86+
}
87+
88+
zend_hash_index_add_empty_element(obj->modified_entries, (zend_ulong) index);
89+
}
90+
91+
static void php_zip_unmark_entry_modified(ze_zip_object *obj, zip_int64_t index)
92+
{
93+
if (obj->modified_entries && index >= 0) {
94+
zend_hash_index_del(obj->modified_entries, (zend_ulong) index);
95+
}
96+
}
97+
98+
static void php_zip_forget_modified_entries(ze_zip_object *obj)
99+
{
100+
if (obj->modified_entries) {
101+
zend_hash_destroy(obj->modified_entries);
102+
FREE_HASHTABLE(obj->modified_entries);
103+
obj->modified_entries = NULL;
104+
}
105+
}
106+
107+
/* Whether the comment of the entry is the string shared with the copy of its record. */
108+
static bool php_zip_entry_shares_comment(ze_zip_object *obj, zip_uint64_t index)
109+
{
110+
zip_error_t *err;
111+
int zip_err, sys_err;
112+
zip_uint32_t comment_len = 0;
113+
bool shared;
114+
115+
if (!obj->modified_entries || !zend_hash_index_exists(obj->modified_entries, (zend_ulong) index)) {
116+
return false;
117+
}
118+
119+
/* Only a comment the entry was read with is shared. */
120+
err = zip_get_error(obj->za);
121+
zip_err = zip_error_code_zip(err);
122+
sys_err = zip_error_code_system(err);
123+
124+
shared = zip_file_get_comment(obj->za, index, &comment_len, ZIP_FL_UNCHANGED | ZIP_FL_ENC_RAW) != NULL
125+
&& comment_len > 0;
126+
127+
zip_error_set(err, zip_err, sys_err);
128+
129+
return shared;
130+
}
131+
/* }}} */
132+
67133
/* {{{ php_zip_set_file_comment */
68-
static bool php_zip_set_file_comment(struct zip *za, zip_uint64_t index, const char *comment, size_t comment_len)
134+
static bool php_zip_set_file_comment(ze_zip_object *obj, zip_uint64_t index, const char *comment, size_t comment_len)
69135
{
70136
zip_uint32_t current_comment_len;
71-
const char *current_comment = zip_file_get_comment(za, index, &current_comment_len, ZIP_FL_ENC_RAW);
137+
const char *current_comment = zip_file_get_comment(obj->za, index, &current_comment_len, ZIP_FL_ENC_RAW);
72138

73-
/* Avoid a libzip use-after-free when resetting an unchanged inherited comment. */
139+
/* Setting the comment an entry already has is a no-op. */
74140
if (current_comment && current_comment_len == comment_len
75141
&& memcmp(current_comment, comment, comment_len) == 0) {
76142
return true;
77143
}
78144

145+
if (php_zip_entry_shares_comment(obj, index)) {
146+
php_error_docref(NULL, E_WARNING, "Cannot set the comment of an entry that has been modified, "
147+
"set the comment before modifying the entry");
148+
return false;
149+
}
150+
79151
if (comment_len == 0) {
80152
/* Passing NULL removes the existing comment. */
81-
return zip_file_set_comment(za, index, NULL, 0, 0) == 0;
153+
if (zip_file_set_comment(obj->za, index, NULL, 0, 0) != 0) {
154+
return false;
155+
}
156+
} else {
157+
ZEND_ASSERT(comment_len <= 0xffff);
158+
if (zip_file_set_comment(obj->za, index, comment, (zip_uint16_t) comment_len, 0) != 0) {
159+
return false;
160+
}
82161
}
83162

84-
ZEND_ASSERT(comment_len <= 0xffff);
85-
return zip_file_set_comment(za, index, comment, (zip_uint16_t) comment_len, 0) == 0;
163+
/* The comment of the entry is no longer shared with the copy of its record. */
164+
php_zip_unmark_entry_modified(obj, (zip_int64_t) index);
165+
166+
return true;
86167
}
87168
/* }}} */
88169

@@ -338,6 +419,7 @@ static int php_zip_add_file(ze_zip_object *obj, const char *filename, size_t fil
338419
zip_source_free(zs);
339420
return -1;
340421
}
422+
php_zip_mark_entry_modified(obj, replace);
341423
zip_error_clear(obj->za);
342424
return 1;
343425
}
@@ -347,6 +429,7 @@ static int php_zip_add_file(ze_zip_object *obj, const char *filename, size_t fil
347429
zip_source_free(zs);
348430
return -1;
349431
}
432+
php_zip_mark_entry_modified(obj, obj->last_id);
350433
zip_error_clear(obj->za);
351434
return 1;
352435
}
@@ -1072,6 +1155,9 @@ static void php_zip_object_free_storage(zend_object *object) /* {{{ */
10721155
if (!intern) {
10731156
return;
10741157
}
1158+
1159+
php_zip_forget_modified_entries(intern);
1160+
10751161
if (intern->za) {
10761162
if (zip_close(intern->za) != 0) {
10771163
php_error_docref(NULL, E_WARNING, "Cannot destroy the zip context: %s", zip_strerror(intern->za));
@@ -1501,6 +1587,8 @@ PHP_METHOD(ZipArchive, open)
15011587
RETURN_FALSE;
15021588
}
15031589

1590+
php_zip_forget_modified_entries(ze_obj);
1591+
15041592
if (ze_obj->za) {
15051593
/* we already have an opened zip, free it */
15061594
if (zip_close(ze_obj->za) != 0) {
@@ -1588,6 +1676,8 @@ PHP_METHOD(ZipArchive, close)
15881676

15891677
ze_obj = Z_ZIP_P(self);
15901678

1679+
php_zip_forget_modified_entries(ze_obj);
1680+
15911681
err = zip_close(intern);
15921682
if (err) {
15931683
php_error_docref(NULL, E_WARNING, "%s", zip_strerror(intern));
@@ -1740,6 +1830,7 @@ PHP_METHOD(ZipArchive, addEmptyDir)
17401830
if ((Z_ZIP_P(self)->last_id = zip_dir_add(intern, (const char *)s, flags)) == -1) {
17411831
RETVAL_FALSE;
17421832
} else {
1833+
php_zip_mark_entry_modified(Z_ZIP_P(self), Z_ZIP_P(self)->last_id);
17431834
zip_error_clear(intern);
17441835
RETVAL_TRUE;
17451836
}
@@ -1999,6 +2090,7 @@ PHP_METHOD(ZipArchive, addFromString)
19992090
zip_source_free(zs);
20002091
RETURN_FALSE;
20012092
} else {
2093+
php_zip_mark_entry_modified(ze_obj, ze_obj->last_id);
20022094
zip_error_clear(intern);
20032095
RETURN_TRUE;
20042096
}
@@ -2217,7 +2309,7 @@ PHP_METHOD(ZipArchive, setCommentName)
22172309
if (idx < 0) {
22182310
RETURN_FALSE;
22192311
}
2220-
RETURN_BOOL(php_zip_set_file_comment(intern, (zip_uint64_t) idx, comment, comment_len));
2312+
RETURN_BOOL(php_zip_set_file_comment(Z_ZIP_P(self), (zip_uint64_t) idx, comment, comment_len));
22212313
}
22222314
/* }}} */
22232315

@@ -2247,7 +2339,7 @@ PHP_METHOD(ZipArchive, setCommentIndex)
22472339
PHP_ZIP_STAT_INDEX(intern, index, 0, sb);
22482340
idx = (zip_uint64_t) index;
22492341

2250-
RETURN_BOOL(php_zip_set_file_comment(intern, idx, comment, comment_len));
2342+
RETURN_BOOL(php_zip_set_file_comment(Z_ZIP_P(self), idx, comment, comment_len));
22512343
}
22522344
/* }}} */
22532345

@@ -2285,6 +2377,8 @@ PHP_METHOD(ZipArchive, setExternalAttributesName)
22852377
(zip_uint8_t)(opsys&0xff), (zip_uint32_t)attr) < 0) {
22862378
RETURN_FALSE;
22872379
}
2380+
2381+
php_zip_mark_entry_modified(Z_ZIP_P(self), idx);
22882382
RETURN_TRUE;
22892383
}
22902384
/* }}} */
@@ -2309,6 +2403,8 @@ PHP_METHOD(ZipArchive, setExternalAttributesIndex)
23092403
(zip_flags_t)flags, (zip_uint8_t)(opsys&0xff), (zip_uint32_t)attr) < 0) {
23102404
RETURN_FALSE;
23112405
}
2406+
2407+
php_zip_mark_entry_modified(Z_ZIP_P(self), (zip_int64_t)index);
23122408
RETURN_TRUE;
23132409
}
23142410
/* }}} */
@@ -2418,6 +2514,8 @@ PHP_METHOD(ZipArchive, setEncryptionName)
24182514
if (zip_file_set_encryption(intern, idx, (zip_uint16_t)method, password)) {
24192515
RETURN_FALSE;
24202516
}
2517+
2518+
php_zip_mark_entry_modified(Z_ZIP_P(self), idx);
24212519
RETURN_TRUE;
24222520
}
24232521
/* }}} */
@@ -2446,6 +2544,8 @@ PHP_METHOD(ZipArchive, setEncryptionIndex)
24462544
if (zip_file_set_encryption(intern, index, (zip_uint16_t)method, password)) {
24472545
RETURN_FALSE;
24482546
}
2547+
2548+
php_zip_mark_entry_modified(Z_ZIP_P(self), (zip_int64_t)index);
24492549
RETURN_TRUE;
24502550
}
24512551
/* }}} */
@@ -2541,6 +2641,8 @@ PHP_METHOD(ZipArchive, setCompressionName)
25412641
(zip_int32_t)comp_method, (zip_uint32_t)comp_flags) != 0) {
25422642
RETURN_FALSE;
25432643
}
2644+
2645+
php_zip_mark_entry_modified(Z_ZIP_P(this), idx);
25442646
RETURN_TRUE;
25452647
}
25462648
/* }}} */
@@ -2564,6 +2666,8 @@ PHP_METHOD(ZipArchive, setCompressionIndex)
25642666
(zip_int32_t)comp_method, (zip_uint32_t)comp_flags) != 0) {
25652667
RETURN_FALSE;
25662668
}
2669+
2670+
php_zip_mark_entry_modified(Z_ZIP_P(this), (zip_int64_t)index);
25672671
RETURN_TRUE;
25682672
}
25692673
/* }}} */
@@ -2601,6 +2705,8 @@ PHP_METHOD(ZipArchive, setMtimeName)
26012705
(time_t)mtime, (zip_uint32_t)flags) != 0) {
26022706
RETURN_FALSE;
26032707
}
2708+
2709+
php_zip_mark_entry_modified(Z_ZIP_P(this), idx);
26042710
RETURN_TRUE;
26052711
}
26062712
/* }}} */
@@ -2624,6 +2730,8 @@ PHP_METHOD(ZipArchive, setMtimeIndex)
26242730
(time_t)mtime, (zip_uint32_t)flags) != 0) {
26252731
RETURN_FALSE;
26262732
}
2733+
2734+
php_zip_mark_entry_modified(Z_ZIP_P(this), (zip_int64_t)index);
26272735
RETURN_TRUE;
26282736
}
26292737
/* }}} */
@@ -2709,6 +2817,8 @@ PHP_METHOD(ZipArchive, renameIndex)
27092817
RETURN_FALSE;
27102818
}
27112819

2820+
php_zip_mark_entry_modified(Z_ZIP_P(self), (zip_int64_t)index);
2821+
27122822
RETURN_TRUE;
27132823
}
27142824
/* }}} */
@@ -2739,6 +2849,8 @@ PHP_METHOD(ZipArchive, renameName)
27392849
RETURN_FALSE;
27402850
}
27412851

2852+
php_zip_mark_entry_modified(Z_ZIP_P(self), (zip_int64_t)sb.index);
2853+
27422854
RETURN_TRUE;
27432855
}
27442856
/* }}} */
@@ -2760,6 +2872,8 @@ PHP_METHOD(ZipArchive, unchangeIndex)
27602872
RETURN_FALSE;
27612873
}
27622874

2875+
php_zip_unmark_entry_modified(Z_ZIP_P(self), (zip_int64_t)index);
2876+
27632877
if (zip_unchange(intern, index) != 0) {
27642878
RETURN_FALSE;
27652879
} else {
@@ -2789,6 +2903,8 @@ PHP_METHOD(ZipArchive, unchangeName)
27892903

27902904
PHP_ZIP_STAT_PATH(intern, name, name_len, 0, sb);
27912905

2906+
php_zip_unmark_entry_modified(Z_ZIP_P(self), (zip_int64_t)sb.index);
2907+
27922908
if (zip_unchange(intern, sb.index) != 0) {
27932909
RETURN_FALSE;
27942910
} else {
@@ -2809,6 +2925,8 @@ PHP_METHOD(ZipArchive, unchangeAll)
28092925

28102926
ZIP_FROM_OBJECT(intern, self);
28112927

2928+
php_zip_forget_modified_entries(Z_ZIP_P(self));
2929+
28122930
if (zip_unchange_all(intern) != 0) {
28132931
RETURN_FALSE;
28142932
} else {
@@ -2829,6 +2947,8 @@ PHP_METHOD(ZipArchive, unchangeArchive)
28292947

28302948
ZIP_FROM_OBJECT(intern, self);
28312949

2950+
php_zip_forget_modified_entries(Z_ZIP_P(self));
2951+
28322952
if (zip_unchange_archive(intern) != 0) {
28332953
RETURN_FALSE;
28342954
} else {

‎ext/zip/php_zip.h‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,11 @@ typedef struct _ze_zip_object {
7474
int filename_len;
7575
int buffers_cnt;
7676
zip_int64_t last_id;
77+
/* Indices of the entries libzip has copied the central directory record of,
78+
* because they were modified. Such a copy shares its comment with the record
79+
* of the entry, which makes setting the comment unsafe. Lazily allocated,
80+
* see php_zip_set_file_comment(). */
81+
HashTable *modified_entries;
7782
int err_zip;
7883
int err_sys;
7984
#ifdef HAVE_PROGRESS_CALLBACK

0 commit comments

Comments
 (0)