Skip to content

Commit 2aa66a1

Browse files
TysonAndreTysonAndre
committed
[RFC] Only unserialize Phar metadata when getMetadata() is called
In other words, don't automatically unserialize when the magic phar:// stream wrappers are used. RFC: https://wiki.php.net/rfc/phar_stop_autoloading_metadata Also, change the signature from `getMetadata()` to `getMetadata(array $unserialize_options = [])`. Start throwing earlier if setMetadata() is called and serialization threw. See https://externals.io/message/110856 and https://bugs.php.net/bug.php?id=76774 This was refactored to add a phar_metadata_tracker for the following reasons: - The way to properly copy a zval was previously implicit and undocumented (e.g. is it a pointer to a raw string or an actual value) - Avoid unnecessary serialization and unserialization in the most common case - If a metadata value is serialized once while saving a new/modified phar file, this allows reusing the same serialized string. - Have as few ways to copy/clone/lazily parse metadata (etc.) as possible, so that code changes can be limited to only a few places in the future. - Performance is hopefully not a concern - copying a string should be faster than unserializing a value, and metadata should be rare in most cases. Remove unnecessary skip in a test(Compression's unused) Add additional assertions about usage of persistent phars
1 parent 9250abf commit 2aa66a1

17 files changed

+574
-324
lines changed

‎ext/phar/phar.c

Lines changed: 168 additions & 111 deletions
Large diffs are not rendered by default.

‎ext/phar/phar_internal.h

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -212,6 +212,17 @@ enum phar_fp_type {
212212
PHAR_TMP
213213
};
214214

215+
/*
216+
* Represents the metadata of the phar file or a file entry within the phar.
217+
* Can contain any combination of serialized data and the value as needed.
218+
*/
219+
typedef struct _phar_metadata_tracker {
220+
/* Can be IS_UNDEF or a regular value */
221+
zval val;
222+
/* Nullable string with the serialized value, if the serialization was performed or read from a file. */
223+
zend_string *str;
224+
} phar_metadata_tracker;
225+
215226
/* entry for one file in a phar file */
216227
typedef struct _phar_entry_info {
217228
/* first bytes are exactly as in file */
@@ -223,8 +234,7 @@ typedef struct _phar_entry_info {
223234
/* remainder */
224235
/* when changing compression, save old flags in case fp is NULL */
225236
uint32_t old_flags;
226-
zval metadata;
227-
uint32_t metadata_len; /* only used for cached manifests */
237+
phar_metadata_tracker metadata_tracker;
228238
uint32_t filename_len;
229239
char *filename;
230240
enum phar_fp_type fp_type;
@@ -239,7 +249,6 @@ typedef struct _phar_entry_info {
239249
int fp_refcount;
240250
char *tmp;
241251
phar_archive_data *phar;
242-
smart_str metadata_str;
243252
char *link; /* symbolic link to another file */
244253
char tar_type;
245254
/* position in the manifest */
@@ -291,8 +300,7 @@ struct _phar_archive_data {
291300
uint32_t sig_flags;
292301
uint32_t sig_len;
293302
char *signature;
294-
zval metadata;
295-
uint32_t metadata_len; /* only used for cached manifests */
303+
phar_metadata_tracker metadata_tracker;
296304
uint32_t phar_pos;
297305
/* if 1, then this alias was manually specified by the user and is not a permanent alias */
298306
uint32_t is_temporary_alias:1;
@@ -544,7 +552,16 @@ int phar_mount_entry(phar_archive_data *phar, char *filename, size_t filename_le
544552
zend_string *phar_find_in_include_path(char *file, size_t file_len, phar_archive_data **pphar);
545553
char *phar_fix_filepath(char *path, size_t *new_len, int use_cwd);
546554
phar_entry_info * phar_open_jit(phar_archive_data *phar, phar_entry_info *entry, char **error);
547-
int phar_parse_metadata(char **buffer, zval *metadata, uint32_t zip_metadata_len);
555+
void phar_parse_metadata_lazy(const char *buffer, phar_metadata_tracker *tracker, uint32_t zip_metadata_len, int persistent);
556+
zend_bool phar_metadata_tracker_has_data(const phar_metadata_tracker* tracker, int persistent);
557+
/* If this has data, free it and set all values to undefined. */
558+
void phar_metadata_tracker_free(phar_metadata_tracker* val, int persistent);
559+
void phar_metadata_tracker_copy(phar_metadata_tracker* dest, const phar_metadata_tracker *source, int persistent);
560+
void phar_metadata_tracker_clone(phar_metadata_tracker* tracker);
561+
void phar_metadata_tracker_try_ensure_has_serialized_data(phar_metadata_tracker* tracker, int persistent);
562+
int phar_metadata_tracker_unserialize_or_copy(phar_metadata_tracker* tracker, zval *value, int persistent, zval *unserialize_options, const char* method_name);
563+
void phar_release_entry_metadata(phar_entry_info *entry);
564+
void phar_release_archive_metadata(phar_archive_data *phar);
548565
void destroy_phar_manifest_entry(zval *zv);
549566
int phar_seek_efp(phar_entry_info *entry, zend_off_t offset, int whence, zend_off_t position, int follow_links);
550567
php_stream *phar_get_efp(phar_entry_info *entry, int follow_links);

‎ext/phar/phar_object.c

Lines changed: 56 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -2266,10 +2266,7 @@ static zend_object *phar_convert_to_other(phar_archive_data *source, int convert
22662266
phar->is_temporary_alias = source->is_temporary_alias;
22672267
phar->alias = source->alias;
22682268

2269-
if (Z_TYPE(source->metadata) != IS_UNDEF) {
2270-
ZVAL_DUP(&phar->metadata, &source->metadata);
2271-
phar->metadata_len = 0;
2272-
}
2269+
phar_metadata_tracker_copy(&phar->metadata_tracker, &source->metadata_tracker, phar->is_persistent);
22732270

22742271
/* first copy each file's uncompressed contents to a temporary file and set per-file flags */
22752272
ZEND_HASH_FOREACH_PTR(&source->manifest, entry) {
@@ -2286,8 +2283,6 @@ static zend_object *phar_convert_to_other(phar_archive_data *source, int convert
22862283
goto no_copy;
22872284
}
22882285

2289-
newentry.metadata_str.s = NULL;
2290-
22912286
if (FAILURE == phar_copy_file_contents(&newentry, phar->fp)) {
22922287
zend_hash_destroy(&(phar->manifest));
22932288
php_stream_close(phar->fp);
@@ -2298,10 +2293,7 @@ static zend_object *phar_convert_to_other(phar_archive_data *source, int convert
22982293
no_copy:
22992294
newentry.filename = estrndup(newentry.filename, newentry.filename_len);
23002295

2301-
if (Z_TYPE(newentry.metadata) != IS_UNDEF) {
2302-
zval_copy_ctor(&newentry.metadata);
2303-
newentry.metadata_str.s = NULL;
2304-
}
2296+
phar_metadata_tracker_clone(&newentry.metadata_tracker);
23052297

23062298
newentry.is_zip = phar->is_zip;
23072299
newentry.is_tar = phar->is_tar;
@@ -3444,10 +3436,7 @@ PHP_METHOD(Phar, copy)
34443436

34453437
memcpy((void *) &newentry, oldentry, sizeof(phar_entry_info));
34463438

3447-
if (Z_TYPE(newentry.metadata) != IS_UNDEF) {
3448-
zval_copy_ctor(&newentry.metadata);
3449-
newentry.metadata_str.s = NULL;
3450-
}
3439+
phar_metadata_tracker_clone(&newentry.metadata_tracker);
34513440

34523441
newentry.filename = estrndup(newfile, newfile_len);
34533442
newentry.filename_len = newfile_len;
@@ -3951,28 +3940,25 @@ PHP_METHOD(Phar, hasMetadata)
39513940
RETURN_THROWS();
39523941
}
39533942

3954-
RETURN_BOOL(Z_TYPE(phar_obj->archive->metadata) != IS_UNDEF);
3943+
RETURN_BOOL(phar_metadata_tracker_has_data(&phar_obj->archive->metadata_tracker, phar_obj->archive->is_persistent));
39553944
}
39563945
/* }}} */
39573946

39583947
/* {{{ Returns the global metadata of the phar */
39593948
PHP_METHOD(Phar, getMetadata)
39603949
{
3950+
zval *unserialize_options = NULL;
3951+
phar_metadata_tracker *tracker;
39613952
PHAR_ARCHIVE_OBJECT();
39623953

3963-
if (zend_parse_parameters_none() == FAILURE) {
3964-
RETURN_THROWS();
3965-
}
3954+
ZEND_PARSE_PARAMETERS_START(0, 1)
3955+
Z_PARAM_OPTIONAL
3956+
Z_PARAM_ARRAY(unserialize_options)
3957+
ZEND_PARSE_PARAMETERS_END();
39663958

3967-
if (Z_TYPE(phar_obj->archive->metadata) != IS_UNDEF) {
3968-
if (phar_obj->archive->is_persistent) {
3969-
char *buf = estrndup((char *) Z_PTR(phar_obj->archive->metadata), phar_obj->archive->metadata_len);
3970-
/* assume success, we would have failed before */
3971-
phar_parse_metadata(&buf, return_value, phar_obj->archive->metadata_len);
3972-
efree(buf);
3973-
} else {
3974-
ZVAL_COPY(return_value, &phar_obj->archive->metadata);
3975-
}
3959+
tracker = &phar_obj->archive->metadata_tracker;
3960+
if (phar_metadata_tracker_has_data(tracker, phar_obj->archive->is_persistent)) {
3961+
phar_metadata_tracker_unserialize_or_copy(tracker, return_value, phar_obj->archive->is_persistent, unserialize_options, "Phar::getMetadata");
39763962
}
39773963
}
39783964
/* }}} */
@@ -3982,6 +3968,8 @@ PHP_METHOD(Phar, setMetadata)
39823968
{
39833969
char *error;
39843970
zval *metadata;
3971+
php_serialize_data_t metadata_hash;
3972+
smart_str main_metadata_str = {0};
39853973

39863974
PHAR_ARCHIVE_OBJECT();
39873975

@@ -3998,12 +3986,24 @@ PHP_METHOD(Phar, setMetadata)
39983986
zend_throw_exception_ex(phar_ce_PharException, 0, "phar \"%s\" is persistent, unable to copy on write", phar_obj->archive->fname);
39993987
RETURN_THROWS();
40003988
}
4001-
if (Z_TYPE(phar_obj->archive->metadata) != IS_UNDEF) {
4002-
zval_ptr_dtor(&phar_obj->archive->metadata);
4003-
ZVAL_UNDEF(&phar_obj->archive->metadata);
3989+
3990+
ZEND_ASSERT(!phar_obj->archive->is_persistent); /* Should no longer be persistent */
3991+
3992+
phar_metadata_tracker_free(&phar_obj->archive->metadata_tracker, phar_obj->archive->is_persistent);
3993+
if (EG(exception)) {
3994+
/* Destructor can throw. */
3995+
return;
40043996
}
40053997

4006-
ZVAL_COPY(&phar_obj->archive->metadata, metadata);
3998+
PHP_VAR_SERIALIZE_INIT(metadata_hash);
3999+
php_var_serialize(&main_metadata_str, metadata, &metadata_hash);
4000+
PHP_VAR_SERIALIZE_DESTROY(metadata_hash);
4001+
if (EG(exception)) {
4002+
/* Serialization can throw. Don't overwrite the original string. */
4003+
return;
4004+
}
4005+
ZVAL_COPY(&phar_obj->archive->metadata_tracker.val, metadata);
4006+
phar_obj->archive->metadata_tracker.str = main_metadata_str.s;
40074007
phar_obj->archive->is_modified = 1;
40084008
phar_flush(phar_obj->archive, 0, 0, 0, &error);
40094009

@@ -4030,20 +4030,18 @@ PHP_METHOD(Phar, delMetadata)
40304030
RETURN_THROWS();
40314031
}
40324032

4033-
if (Z_TYPE(phar_obj->archive->metadata) != IS_UNDEF) {
4034-
zval_ptr_dtor(&phar_obj->archive->metadata);
4035-
ZVAL_UNDEF(&phar_obj->archive->metadata);
4036-
phar_obj->archive->is_modified = 1;
4037-
phar_flush(phar_obj->archive, 0, 0, 0, &error);
4033+
if (!phar_metadata_tracker_has_data(&phar_obj->archive->metadata_tracker, phar_obj->archive->is_persistent)) {
4034+
RETURN_TRUE;
4035+
}
40384036

4039-
if (error) {
4040-
zend_throw_exception_ex(phar_ce_PharException, 0, "%s", error);
4041-
efree(error);
4042-
RETURN_THROWS();
4043-
} else {
4044-
RETURN_TRUE;
4045-
}
4037+
phar_metadata_tracker_free(&phar_obj->archive->metadata_tracker, phar_obj->archive->is_persistent);
4038+
phar_obj->archive->is_modified = 1;
4039+
phar_flush(phar_obj->archive, 0, 0, 0, &error);
40464040

4041+
if (error) {
4042+
zend_throw_exception_ex(phar_ce_PharException, 0, "%s", error);
4043+
efree(error);
4044+
RETURN_THROWS();
40474045
} else {
40484046
RETURN_TRUE;
40494047
}
@@ -4630,28 +4628,25 @@ PHP_METHOD(PharFileInfo, hasMetadata)
46304628
RETURN_THROWS();
46314629
}
46324630

4633-
RETURN_BOOL(Z_TYPE(entry_obj->entry->metadata) != IS_UNDEF);
4631+
RETURN_BOOL(phar_metadata_tracker_has_data(&entry_obj->entry->metadata_tracker, entry_obj->entry->is_persistent));
46344632
}
46354633
/* }}} */
46364634

46374635
/* {{{ Returns the metadata of the entry */
46384636
PHP_METHOD(PharFileInfo, getMetadata)
46394637
{
4638+
zval *unserialize_options = NULL;
4639+
phar_metadata_tracker *tracker;
46404640
PHAR_ENTRY_OBJECT();
46414641

4642-
if (zend_parse_parameters_none() == FAILURE) {
4643-
RETURN_THROWS();
4644-
}
4642+
ZEND_PARSE_PARAMETERS_START(0, 1)
4643+
Z_PARAM_OPTIONAL
4644+
Z_PARAM_ARRAY(unserialize_options)
4645+
ZEND_PARSE_PARAMETERS_END();
46454646

4646-
if (Z_TYPE(entry_obj->entry->metadata) != IS_UNDEF) {
4647-
if (entry_obj->entry->is_persistent) {
4648-
char *buf = estrndup((char *) Z_PTR(entry_obj->entry->metadata), entry_obj->entry->metadata_len);
4649-
/* assume success, we would have failed before */
4650-
phar_parse_metadata(&buf, return_value, entry_obj->entry->metadata_len);
4651-
efree(buf);
4652-
} else {
4653-
ZVAL_COPY(return_value, &entry_obj->entry->metadata);
4654-
}
4647+
tracker = &entry_obj->entry->metadata_tracker;
4648+
if (phar_metadata_tracker_has_data(tracker, entry_obj->entry->is_persistent)) {
4649+
phar_metadata_tracker_unserialize_or_copy(tracker, return_value, entry_obj->entry->is_persistent, unserialize_options, "PharFileInfo::getMetadata");
46554650
}
46564651
}
46574652
/* }}} */
@@ -4689,12 +4684,9 @@ PHP_METHOD(PharFileInfo, setMetadata)
46894684
/* re-populate after copy-on-write */
46904685
entry_obj->entry = zend_hash_str_find_ptr(&phar->manifest, entry_obj->entry->filename, entry_obj->entry->filename_len);
46914686
}
4692-
if (Z_TYPE(entry_obj->entry->metadata) != IS_UNDEF) {
4693-
zval_ptr_dtor(&entry_obj->entry->metadata);
4694-
ZVAL_UNDEF(&entry_obj->entry->metadata);
4695-
}
4687+
phar_metadata_tracker_free(&entry_obj->entry->metadata_tracker, entry_obj->entry->is_persistent);
46964688

4697-
ZVAL_COPY(&entry_obj->entry->metadata, metadata);
4689+
ZVAL_COPY(&entry_obj->entry->metadata_tracker.val, metadata);
46984690

46994691
entry_obj->entry->is_modified = 1;
47004692
entry_obj->entry->phar->is_modified = 1;
@@ -4729,7 +4721,7 @@ PHP_METHOD(PharFileInfo, delMetadata)
47294721
RETURN_THROWS();
47304722
}
47314723

4732-
if (Z_TYPE(entry_obj->entry->metadata) != IS_UNDEF) {
4724+
if (phar_metadata_tracker_has_data(&entry_obj->entry->metadata_tracker, entry_obj->entry->is_persistent)) {
47334725
if (entry_obj->entry->is_persistent) {
47344726
phar_archive_data *phar = entry_obj->entry->phar;
47354727

@@ -4740,8 +4732,8 @@ PHP_METHOD(PharFileInfo, delMetadata)
47404732
/* re-populate after copy-on-write */
47414733
entry_obj->entry = zend_hash_str_find_ptr(&phar->manifest, entry_obj->entry->filename, entry_obj->entry->filename_len);
47424734
}
4743-
zval_ptr_dtor(&entry_obj->entry->metadata);
4744-
ZVAL_UNDEF(&entry_obj->entry->metadata);
4735+
/* multiple values may reference the metadata */
4736+
phar_metadata_tracker_free(&entry_obj->entry->metadata_tracker, entry_obj->entry->is_persistent);
47454737
entry_obj->entry->is_modified = 1;
47464738
entry_obj->entry->phar->is_modified = 1;
47474739

‎ext/phar/phar_object.stub.php

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ public function getAlias() {}
6767
public function getPath() {}
6868

6969
/** @return mixed */
70-
public function getMetadata() {}
70+
public function getMetadata(array $unserialize_options = []) {}
7171

7272
/** @return bool */
7373
public function getModified() {}
@@ -303,7 +303,7 @@ public function getPath() {}
303303
* @return mixed
304304
* @alias Phar::getMetadata
305305
*/
306-
public function getMetadata() {}
306+
public function getMetadata(array $unserialize_options = []) {}
307307

308308
/**
309309
* @return bool
@@ -510,7 +510,7 @@ public function getCRC32() {}
510510
public function getContent() {}
511511

512512
/** @return mixed */
513-
public function getMetadata() {}
513+
public function getMetadata(array $unserialize_options = []) {}
514514

515515
/** @return int */
516516
public function getPharFlags() {}

‎ext/phar/phar_object_arginfo.h

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/* This is a generated file, edit the .stub.php file instead.
2-
* Stub hash: fd4f05b74248e4f7efb234cac8e3a90e17037ee0 */
2+
* Stub hash: 586c79f097e9153b70f6c6e208daa08acc0ce1d4 */
33

44
ZEND_BEGIN_ARG_INFO_EX(arginfo_class_Phar___construct, 0, 0, 1)
55
ZEND_ARG_TYPE_INFO(0, filename, IS_STRING, 0)
@@ -82,7 +82,9 @@ ZEND_END_ARG_INFO()
8282

8383
#define arginfo_class_Phar_getPath arginfo_class_Phar___destruct
8484

85-
#define arginfo_class_Phar_getMetadata arginfo_class_Phar___destruct
85+
ZEND_BEGIN_ARG_INFO_EX(arginfo_class_Phar_getMetadata, 0, 0, 0)
86+
ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, unserialize_options, IS_ARRAY, 0, "[]")
87+
ZEND_END_ARG_INFO()
8688

8789
#define arginfo_class_Phar_getModified arginfo_class_Phar___destruct
8890

@@ -252,7 +254,7 @@ ZEND_END_ARG_INFO()
252254

253255
#define arginfo_class_PharData_getPath arginfo_class_Phar___destruct
254256

255-
#define arginfo_class_PharData_getMetadata arginfo_class_Phar___destruct
257+
#define arginfo_class_PharData_getMetadata arginfo_class_Phar_getMetadata
256258

257259
#define arginfo_class_PharData_getModified arginfo_class_Phar___destruct
258260

@@ -346,7 +348,7 @@ ZEND_END_ARG_INFO()
346348

347349
#define arginfo_class_PharFileInfo_getContent arginfo_class_Phar___destruct
348350

349-
#define arginfo_class_PharFileInfo_getMetadata arginfo_class_Phar___destruct
351+
#define arginfo_class_PharFileInfo_getMetadata arginfo_class_Phar_getMetadata
350352

351353
#define arginfo_class_PharFileInfo_getPharFlags arginfo_class_Phar___destruct
352354

‎ext/phar/stream.c

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -223,13 +223,10 @@ static php_stream * phar_wrapper_open_url(php_stream_wrapper *wrapper, const cha
223223
idata->internal_file->flags |= Z_LVAL_P(pzoption);
224224
}
225225
if ((pzoption = zend_hash_str_find(pharcontext, "metadata", sizeof("metadata")-1)) != NULL) {
226-
if (Z_TYPE(idata->internal_file->metadata) != IS_UNDEF) {
227-
zval_ptr_dtor(&idata->internal_file->metadata);
228-
ZVAL_UNDEF(&idata->internal_file->metadata);
229-
}
226+
phar_metadata_tracker_free(&idata->internal_file->metadata_tracker, idata->internal_file->is_persistent);
230227

231228
metadata = pzoption;
232-
ZVAL_COPY_DEREF(&idata->internal_file->metadata, metadata);
229+
ZVAL_COPY_DEREF(&idata->internal_file->metadata_tracker.val, metadata);
233230
idata->phar->is_modified = 1;
234231
}
235232
}
@@ -846,7 +843,7 @@ static int phar_wrapper_rename(php_stream_wrapper *wrapper, const char *url_from
846843
/* mark the old one for deletion */
847844
entry->is_deleted = 1;
848845
entry->fp = NULL;
849-
ZVAL_UNDEF(&entry->metadata);
846+
ZVAL_UNDEF(&entry->metadata_tracker.val);
850847
entry->link = entry->tmp = NULL;
851848
source = entry;
852849

0 commit comments

Comments
 (0)