[RFC] [WIP] 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

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.

TODO: Add additional tests of edge cases writing phars after setMetadata

Author: TysonAndre

ext/phar/phar.c

@@ -229,20 +229,7 @@ void phar_destroy_phar_data(phar_archive_data *phar) /* {{{ */
 		HT_INVALIDATE(&phar->virtual_dirs);
 	}
 
-	if (Z_TYPE(phar->metadata) != IS_UNDEF) {
-		if (phar->is_persistent) {
-			if (phar->metadata_len) {
-				/* for zip comments that are strings */
-				free(Z_PTR(phar->metadata));
-			} else {
-				zval_internal_ptr_dtor(&phar->metadata);
-			}
-		} else {
-			zval_ptr_dtor(&phar->metadata);
-		}
-		phar->metadata_len = 0;
-		ZVAL_UNDEF(&phar->metadata);
-	}
+	phar_metadata_tracker_free(&phar->metadata_tracker);
 
 	if (phar->fp) {
 		php_stream_close(phar->fp);
@@ -383,25 +370,7 @@ void destroy_phar_manifest_entry_int(phar_entry_info *entry) /* {{{ */
 		entry->fp = 0;
 	}
 
-	if (Z_TYPE(entry->metadata) != IS_UNDEF) {
-		if (entry->is_persistent) {
-			if (entry->metadata_len) {
-				/* for zip comments that are strings */
-				free(Z_PTR(entry->metadata));
-			} else {
-				zval_internal_ptr_dtor(&entry->metadata);
-			}
-		} else {
-			zval_ptr_dtor(&entry->metadata);
-		}
-		entry->metadata_len = 0;
-		ZVAL_UNDEF(&entry->metadata);
-	}
-
-	if (entry->metadata_str.s) {
-		smart_str_free(&entry->metadata_str);
-		entry->metadata_str.s = NULL;
-	}
+	phar_metadata_tracker_free(&entry->metadata_tracker);
 
 	pefree(entry->filename, entry->is_persistent);
 
@@ -600,49 +569,135 @@ int phar_open_parsed_phar(char *fname, size_t fname_len, char *alias, size_t ali
 /* }}}*/
 
 /**
- * Parse out metadata from the manifest for a single file
- *
- * Meta-data is in this format:
- * [len32][data...]
- *
- * data is the serialized zval
+ * Attempt to serialize the data.
+ * Callers are responsible for handling EG(exception) if one occurs.
  */
-int phar_parse_metadata(char **buffer, zval *metadata, uint32_t zip_metadata_len) /* {{{ */
+void phar_metadata_tracker_try_ensure_has_serialized_data(phar_metadata_tracker *tracker, int persistent) /* {{{ */
 {
-	php_unserialize_data_t var_hash;
+	php_serialize_data_t metadata_hash;
+	smart_str metadata_str = {0};
+	if (tracker->str || Z_ISUNDEF(tracker->val)) {
+		/* Already has serialized the value or there is no value */
+		return;
+	}
+	PHP_VAR_SERIALIZE_INIT(metadata_hash);
+	php_var_serialize(&metadata_str, &tracker->val, &metadata_hash);
+	PHP_VAR_SERIALIZE_DESTROY(metadata_hash);
+	if (!metadata_str.s) {
+		return;
+	}
+	if (persistent) {
+		tracker->str = zend_string_init(ZSTR_VAL(metadata_str.s), ZSTR_LEN(metadata_str.s), persistent);
+		zend_string_release(metadata_str.s);
+	} else {
+		tracker->str = metadata_str.s;
+	}
+}
+/* }}} */
 
-	if (zip_metadata_len) {
-		const unsigned char *p;
-		unsigned char *p_buff = (unsigned char *)estrndup(*buffer, zip_metadata_len);
-		p = p_buff;
+/**
+ * Parse out metadata when phar_metadata_tracker_has_data is true
+ */
+int phar_metadata_tracker_unserialize_or_copy(const phar_metadata_tracker *tracker, zval *metadata, int persistent) /* {{{ */
+{
+	if (Z_TYPE(tracker->val) == IS_UNDEF || persistent) {
+		const unsigned char *start;
+		php_unserialize_data_t var_hash;
+		/* precondition: This has data */
+		ZEND_ASSERT(tracker->str != NULL);
 		ZVAL_NULL(metadata);
 		PHP_VAR_UNSERIALIZE_INIT(var_hash);
+		start = (const unsigned char*)ZSTR_VAL(tracker->str);
 
-		if (!php_var_unserialize(metadata, &p, p + zip_metadata_len, &var_hash)) {
-			efree(p_buff);
+		if (!php_var_unserialize(metadata, &start, start + ZSTR_LEN(tracker->str), &var_hash)) {
 			PHP_VAR_UNSERIALIZE_DESTROY(var_hash);
 			zval_ptr_dtor(metadata);
 			ZVAL_UNDEF(metadata);
 			return FAILURE;
 		}
-		efree(p_buff);
 		PHP_VAR_UNSERIALIZE_DESTROY(var_hash);
-
-		if (PHAR_G(persist)) {
-			/* lazy init metadata */
-			zval_ptr_dtor(metadata);
-			Z_PTR_P(metadata) = pemalloc(zip_metadata_len, 1);
-			memcpy(Z_PTR_P(metadata), *buffer, zip_metadata_len);
-			return SUCCESS;
-		}
+		return SUCCESS;
 	} else {
-		ZVAL_UNDEF(metadata);
+		/* TODO: what is the current/expected behavior when fetching an object with getMetadata() and modifying a property? Previously, it was underdefined, and probably unimportant to support  */
+		ZVAL_COPY(metadata, &tracker->val);
 	}
 
 	return SUCCESS;
 }
 /* }}}*/
 
+/**
+ * Check if this has any data, serialized or as a raw value.
+ */
+zend_bool phar_metadata_tracker_has_data(const phar_metadata_tracker *tracker) /* {{{ */
+{
+	return Z_TYPE(tracker->val) != IS_UNDEF || tracker->str != NULL;
+}
+/* }}} */
+
+/**
+ * Free memory used to track the metadata and set all fields to be null/undef.
+ */
+void phar_metadata_tracker_free(phar_metadata_tracker *tracker) /* {{{ */
+{
+	if (Z_TYPE(tracker->val) != IS_UNDEF) {
+		zval_ptr_dtor(&tracker->val);
+		ZVAL_UNDEF(&tracker->val);
+	}
+	if (tracker->str) {
+		zend_string_release(tracker->str);
+		tracker->str = NULL;
+	}
+}
+/* }}} */
+
+/**
+ * Free memory used to track the metadata and set all fields to be null/undef.
+ */
+void phar_metadata_tracker_copy(phar_metadata_tracker *dest, const phar_metadata_tracker *source) /* {{{ */
+{
+	ZEND_ASSERT(dest != source);
+	phar_metadata_tracker_free(dest);
+
+	if (Z_TYPE(source->val) != IS_UNDEF) {
+		ZVAL_COPY(&dest->val, &source->val);
+	}
+	if (source->str) {
+		dest->str = zend_string_copy(source->str);
+	}
+}
+/* }}} */
+
+/**
+ * Increment reference counts after a metadata entry was copied
+ */
+void phar_metadata_tracker_clone(phar_metadata_tracker *tracker) /* {{{ */
+{
+	zval_copy_ctor(&tracker->val);
+	if (tracker->str) {
+		tracker->str = zend_string_copy(tracker->str);
+	}
+}
+/* }}} */
+
+/**
+ * Parse out metadata from the manifest for a single file, saving it into a Z_PTR (or setting metadata to Z_UNDEF)
+ *
+ * Meta-data is in this format:
+ * [len32][data...]
+ *
+ * data is the serialized zval
+ */
+void phar_parse_metadata_lazy(const char *buffer, phar_metadata_tracker *tracker, uint32_t zip_metadata_len, int persistent) /* {{{ */
+{
+	phar_metadata_tracker_free(tracker);
+	if (zip_metadata_len) {
+		/* lazy init metadata */
+		tracker->str = zend_string_init(buffer, zip_metadata_len, persistent);
+	}
+}
+/* }}}*/
+
 /**
  * Size of fixed fields in the manifest.
  * See: http://php.net/manual/en/phar.fileformat.phar.php
@@ -1028,7 +1083,6 @@ static int phar_parse_pharfile(php_stream *fp, char *fname, size_t fname_len, ch
 	/* check whether we have meta data, zero check works regardless of byte order */
 	SAFE_PHAR_GET_32(buffer, endbuffer, len);
 	if (mydata->is_persistent) {
-		mydata->metadata_len = len;
 		if (!len) {
 			/* FIXME: not sure why this is needed but removing it breaks tests */
 			SAFE_PHAR_GET_32(buffer, endbuffer, len);
@@ -1037,9 +1091,9 @@ static int phar_parse_pharfile(php_stream *fp, char *fname, size_t fname_len, ch
 	if(len > (size_t)(endbuffer - buffer)) {
 		MAPPHAR_FAIL("internal corruption of phar \"%s\" (trying to read past buffer end)");
 	}
-	if (phar_parse_metadata(&buffer, &mydata->metadata, len) == FAILURE) {
-		MAPPHAR_FAIL("unable to read phar metadata in .phar file \"%s\"");
-	}
+	/* Don't implicitly call unserialize() on potentially untrusted input unless getMetadata() is called directly. */
+	// XXX where is the raw data to unserialize being set
+	phar_parse_metadata_lazy(buffer, &mydata->metadata_tracker, len, mydata->is_persistent);
 	buffer += len;
 
 	/* set up our manifest */
@@ -1112,19 +1166,15 @@ static int phar_parse_pharfile(php_stream *fp, char *fname, size_t fname_len, ch
 		}
 
 		PHAR_GET_32(buffer, len);
-		if (entry.is_persistent) {
-			entry.metadata_len = len;
-		} else {
-			entry.metadata_len = 0;
-		}
 		if (len > (size_t)(endbuffer - buffer)) {
 			pefree(entry.filename, entry.is_persistent);
 			MAPPHAR_FAIL("internal corruption of phar \"%s\" (truncated manifest entry)");
 		}
-		if (phar_parse_metadata(&buffer, &entry.metadata, len) == FAILURE) {
-			pefree(entry.filename, entry.is_persistent);
-			MAPPHAR_FAIL("unable to read file metadata in .phar file \"%s\"");
-		}
+		/* Don't implicitly call unserialize() on potentially untrusted input unless getMetadata() is called directly. */
+		/* The same local variable entry is reused in a loop, so reset the state before reading data. */
+		ZVAL_UNDEF(&entry.metadata_tracker.val);
+		entry.metadata_tracker.str = NULL;
+		phar_parse_metadata_lazy(buffer, &entry.metadata_tracker, len, entry.is_persistent);
 		buffer += len;
 
 		entry.offset = entry.offset_abs = offset;
@@ -1133,39 +1183,21 @@ static int phar_parse_pharfile(php_stream *fp, char *fname, size_t fname_len, ch
 		switch (entry.flags & PHAR_ENT_COMPRESSION_MASK) {
 			case PHAR_ENT_COMPRESSED_GZ:
 				if (!PHAR_G(has_zlib)) {
-					if (Z_TYPE(entry.metadata) != IS_UNDEF) {
-						if (entry.is_persistent) {
-							free(Z_PTR(entry.metadata));
-						} else {
-							zval_ptr_dtor(&entry.metadata);
-						}
-					}
+					phar_metadata_tracker_free(&entry.metadata_tracker);
 					pefree(entry.filename, entry.is_persistent);
 					MAPPHAR_FAIL("zlib extension is required for gz compressed .phar file \"%s\"");
 				}
 				break;
 			case PHAR_ENT_COMPRESSED_BZ2:
 				if (!PHAR_G(has_bz2)) {
-					if (Z_TYPE(entry.metadata) != IS_UNDEF) {
-						if (entry.is_persistent) {
-							free(Z_PTR(entry.metadata));
-						} else {
-							zval_ptr_dtor(&entry.metadata);
-						}
-					}
+					phar_metadata_tracker_free(&entry.metadata_tracker);
 					pefree(entry.filename, entry.is_persistent);
 					MAPPHAR_FAIL("bz2 extension is required for bzip2 compressed .phar file \"%s\"");
 				}
 				break;
 			default:
 				if (entry.uncompressed_filesize != entry.compressed_filesize) {
-					if (Z_TYPE(entry.metadata) != IS_UNDEF) {
-						if (entry.is_persistent) {
-							free(Z_PTR(entry.metadata));
-						} else {
-							zval_ptr_dtor(&entry.metadata);
-						}
-					}
+					phar_metadata_tracker_free(&entry.metadata_tracker);
 					pefree(entry.filename, entry.is_persistent);
 					MAPPHAR_FAIL("internal corruption of phar \"%s\" (compressed and uncompressed size does not match for uncompressed entry)");
 				}
@@ -2673,9 +2705,11 @@ int phar_flush(phar_archive_data *phar, char *user_stub, zend_long len, int conv
 
 	/* compress as necessary, calculate crcs, serialize meta-data, manifest size, and file sizes */
 	main_metadata_str.s = NULL;
-	if (Z_TYPE(phar->metadata) != IS_UNDEF) {
+	if (phar->metadata_tracker.str) {
+		smart_str_appendl(&main_metadata_str, ZSTR_VAL(phar->metadata_tracker.str), ZSTR_LEN(phar->metadata_tracker.str));
+	} else if (!Z_ISUNDEF(phar->metadata_tracker.val)) {
 		PHP_VAR_SERIALIZE_INIT(metadata_hash);
-		php_var_serialize(&main_metadata_str, &phar->metadata, &metadata_hash);
+		php_var_serialize(&main_metadata_str, &phar->metadata_tracker.val, &metadata_hash);
 		PHP_VAR_SERIALIZE_DESTROY(metadata_hash);
 	}
 	new_manifest_count = 0;
@@ -2710,23 +2744,17 @@ int phar_flush(phar_archive_data *phar, char *user_stub, zend_long len, int conv
 			/* we use this to calculate API version, 1.1.1 is used for phars with directories */
 			has_dirs = 1;
 		}
-		if (Z_TYPE(entry->metadata) != IS_UNDEF) {
-			if (entry->metadata_str.s) {
-				smart_str_free(&entry->metadata_str);
-			}
-			entry->metadata_str.s = NULL;
+		if (Z_TYPE(entry->metadata_tracker.val) != IS_UNDEF && !entry->metadata_tracker.str) {
+			/* Assume serialization will succeed. TODO: Set error and throw if EG(exception) != NULL */
+			smart_str buf = {0};
 			PHP_VAR_SERIALIZE_INIT(metadata_hash);
-			php_var_serialize(&entry->metadata_str, &entry->metadata, &metadata_hash);
+			php_var_serialize(&buf, &entry->metadata_tracker.val, &metadata_hash);
 			PHP_VAR_SERIALIZE_DESTROY(metadata_hash);
-		} else {
-			if (entry->metadata_str.s) {
-				smart_str_free(&entry->metadata_str);
-			}
-			entry->metadata_str.s = NULL;
+			entry->metadata_tracker.str = buf.s;
 		}
 
 		/* 32 bits for filename length, length of filename, manifest + metadata, and add 1 for trailing / if a directory */
-		offset += 4 + entry->filename_len + sizeof(entry_buffer) + (entry->metadata_str.s ? ZSTR_LEN(entry->metadata_str.s) : 0) + (entry->is_dir ? 1 : 0);
+		offset += 4 + entry->filename_len + sizeof(entry_buffer) + (entry->metadata_tracker.str ? ZSTR_LEN(entry->metadata_tracker.str) : 0) + (entry->is_dir ? 1 : 0);
 
 		/* compress and rehash as necessary */
 		if ((oldfile && !entry->is_modified) || entry->is_dir) {
@@ -2916,6 +2944,7 @@ int phar_flush(phar_archive_data *phar, char *user_stub, zend_long len, int conv
 
 	/* now write the manifest */
 	ZEND_HASH_FOREACH_PTR(&phar->manifest, entry) {
+		const zend_string *metadata_str;
 		if (entry->is_deleted || entry->is_mounted) {
 			/* remove this from the new phar if deleted, ignore if mounted */
 			continue;
@@ -2960,11 +2989,12 @@ int phar_flush(phar_archive_data *phar, char *user_stub, zend_long len, int conv
 		phar_set_32(entry_buffer+8, entry->compressed_filesize);
 		phar_set_32(entry_buffer+12, entry->crc32);
 		phar_set_32(entry_buffer+16, entry->flags);
-		phar_set_32(entry_buffer+20, entry->metadata_str.s ? ZSTR_LEN(entry->metadata_str.s) : 0);
+		metadata_str = entry->metadata_tracker.str;
+		phar_set_32(entry_buffer+20, metadata_str ? ZSTR_LEN(metadata_str) : 0);
 
 		if (sizeof(entry_buffer) != php_stream_write(newfile, entry_buffer, sizeof(entry_buffer))
-		|| (entry->metadata_str.s &&
-		    ZSTR_LEN(entry->metadata_str.s) != php_stream_write(newfile, ZSTR_VAL(entry->metadata_str.s), ZSTR_LEN(entry->metadata_str.s)))) {
+		|| (metadata_str &&
+		    ZSTR_LEN(metadata_str) != php_stream_write(newfile, ZSTR_VAL(metadata_str), ZSTR_LEN(metadata_str)))) {
 			if (closeoldfile) {
 				php_stream_close(oldfile);
 			}