Improve robustness of Phar*->setMetadata()

Add sanity checks for edge cases freeing metadata, when destructors
or serializers modify the phar recursively.

Typical use cases of php have read-only phars and would not be affected.

Author: TysonAndre

ext/phar/phar.c

@@ -652,15 +652,20 @@ zend_bool phar_metadata_tracker_has_data(const phar_metadata_tracker *tracker, i
  */
 void phar_metadata_tracker_free(phar_metadata_tracker *tracker, int persistent) /* {{{ */
 {
-	if (!Z_ISUNDEF(tracker->val)) {
-		ZEND_ASSERT(!persistent);
-		zval_ptr_dtor(&tracker->val);
-		ZVAL_UNDEF(&tracker->val);
-	}
+	/* Free the string before the zval in case the zval's destructor modifies the metadata */
 	if (tracker->str) {
 		zend_string_release(tracker->str);
 		tracker->str = NULL;
 	}
+	if (!Z_ISUNDEF(tracker->val)) {
+		/* Here, copy the original zval to a different pointer without incrementing the refcount in case something uses the original while it's being freed. */
+		zval zval_copy;
+
+		ZEND_ASSERT(!persistent);
+		ZVAL_COPY_VALUE(&zval_copy, &tracker->val);
+		ZVAL_UNDEF(&tracker->val);
+		zval_ptr_dtor(&zval_copy);
+	}
 }
 /* }}} */
 

ext/phar/phar_object.c

@@ -3963,13 +3963,42 @@ PHP_METHOD(Phar, getMetadata)
 }
 /* }}} */
 
+/* {{{ Modifies the phar metadata or throws */
+static int serialize_metadata_or_throw(phar_metadata_tracker *tracker, int persistent, zval *metadata)
+{
+	php_serialize_data_t metadata_hash;
+	smart_str main_metadata_str = {0};
+
+	PHP_VAR_SERIALIZE_INIT(metadata_hash);
+	php_var_serialize(&main_metadata_str, metadata, &metadata_hash);
+	PHP_VAR_SERIALIZE_DESTROY(metadata_hash);
+	if (EG(exception)) {
+		/* Serialization can throw. Don't overwrite the original value or original string. */
+		return FAILURE;
+	}
+
+	phar_metadata_tracker_free(tracker, persistent);
+	if (EG(exception)) {
+		/* Destructor can throw. */
+		zend_string_release(main_metadata_str.s);
+		return FAILURE;
+	}
+
+	if (tracker->str) {
+		zend_throw_exception_ex(phar_ce_PharException, 0, "Metadata unexpectedly changed during setMetadata()");
+		zend_string_release(main_metadata_str.s);
+		return FAILURE;
+	}
+	ZVAL_COPY(&tracker->val, metadata);
+	tracker->str = main_metadata_str.s;
+	return SUCCESS;
+}
+
 /* {{{ Sets the global metadata of the phar */
 PHP_METHOD(Phar, setMetadata)
 {
 	char *error;
 	zval *metadata;
-	php_serialize_data_t metadata_hash;
-	smart_str main_metadata_str = {0};
 
 	PHAR_ARCHIVE_OBJECT();
 
@@ -3988,22 +4017,10 @@ PHP_METHOD(Phar, setMetadata)
 	}
 
 	ZEND_ASSERT(!phar_obj->archive->is_persistent); /* Should no longer be persistent */
-
-	phar_metadata_tracker_free(&phar_obj->archive->metadata_tracker, phar_obj->archive->is_persistent);
-	if (EG(exception)) {
-		/* Destructor can throw. */
-		return;
+	if (serialize_metadata_or_throw(&phar_obj->archive->metadata_tracker, phar_obj->archive->is_persistent, metadata) != SUCCESS) {
+		RETURN_THROWS();
 	}
 
-	PHP_VAR_SERIALIZE_INIT(metadata_hash);
-	php_var_serialize(&main_metadata_str, metadata, &metadata_hash);
-	PHP_VAR_SERIALIZE_DESTROY(metadata_hash);
-	if (EG(exception)) {
-		/* Serialization can throw. Don't overwrite the original string. */
-		return;
-	}
-	ZVAL_COPY(&phar_obj->archive->metadata_tracker.val, metadata);
-	phar_obj->archive->metadata_tracker.str = main_metadata_str.s;
 	phar_obj->archive->is_modified = 1;
 	phar_flush(phar_obj->archive, 0, 0, 0, &error);
 
@@ -4683,10 +4700,12 @@ PHP_METHOD(PharFileInfo, setMetadata)
 		}
 		/* re-populate after copy-on-write */
 		entry_obj->entry = zend_hash_str_find_ptr(&phar->manifest, entry_obj->entry->filename, entry_obj->entry->filename_len);
+		ZEND_ASSERT(!entry_obj->entry->is_persistent); /* Should no longer be persistent */
 	}
-	phar_metadata_tracker_free(&entry_obj->entry->metadata_tracker, entry_obj->entry->is_persistent);
 
-	ZVAL_COPY(&entry_obj->entry->metadata_tracker.val, metadata);
+	if (serialize_metadata_or_throw(&entry_obj->entry->metadata_tracker, entry_obj->entry->is_persistent, metadata) != SUCCESS) {
+		RETURN_THROWS();
+	}
 
 	entry_obj->entry->is_modified = 1;
 	entry_obj->entry->phar->is_modified = 1;

ext/phar/tests/bug69958.phpt

@@ -9,7 +9,7 @@ Still has memory leaks, see https://bugs.php.net/bug.php?id=70005
 $tarphar = new PharData(__DIR__.'/bug69958.tar');
 $phar = $tarphar->convertToData(Phar::TAR);
 --EXPECTF--
-Fatal error: Uncaught exception 'BadMethodCallException' with message 'phar "%s/bug69958.tar" exists and must be unlinked prior to conversion' in %s/bug69958.php:%d
+Fatal error: Uncaught BadMethodCallException: phar "%s/bug69958.tar" exists and must be unlinked prior to conversion in %s/bug69958.php:%d
 Stack trace:
 #0 %s/bug69958.php(%d): PharData->convertToData(%d)
 #1 {main}

ext/phar/tests/phar_metadata_write4.phpt

@@ -11,7 +11,10 @@ phar.readonly=0
 <?php
 class EchoesOnWakeup {
     public function __wakeup() {
-        echo "In wakeup\n";
+        echo "In __wakeup " . spl_object_id($this) . "\n";
+    }
+    public function __destruct() {
+        echo "In __destruct " . spl_object_id($this) . "\n";
     }
 }
 class ThrowsOnSerialize {
@@ -30,6 +33,7 @@ include 'files/phar_test.inc';
 foreach($files as $name => $cont) {
     var_dump(file_get_contents($pname.'/'.$name));
 }
+unset($files);
 
 $phar = new Phar($fname);
 echo "Loading metadata for 'a' without allowed_classes\n";
@@ -46,12 +50,18 @@ echo "Loading metadata from 'a' from the new phar\n";
 var_dump($phar['a']->getMetadata());
 echo "Loading metadata from 'a' from the new phar with unserialize options\n";
 var_dump($phar['a']->getMetadata(['allowed_classes' => true]));
+// PharEntry->setMetaData will do the following:
+// 1. serialize, checking for exceptions
+// 2. free the original data, checking for exceptions or the data getting set from destructors or error handlers.
+// 3. set the new data.
 try {
     var_dump($phar['a']->setMetadata(new ThrowsOnSerialize()));
 } catch (RuntimeException $e) {
     echo "Caught {$e->getMessage()} at {$e->getFile()}:{$e->getLine()}\n";
+    unset($e);
 }
 var_dump($phar['a']->getMetadata([]));
+var_dump($phar['a']->getMetadata(['allowed_classes' => false]));
 
 ?>
 --CLEAN--
@@ -60,24 +70,34 @@ unlink(__DIR__ . '/' . basename(__FILE__, '.clean.php') . '.phar.php');
 unlink(__DIR__ . '/' . basename(__FILE__, '.clean.php') . '.phar.php.copy.php');
 ?>
 --EXPECTF--
+In __destruct 1
 string(1) "a"
 Loading metadata for 'a' without allowed_classes
 object(__PHP_Incomplete_Class)#3 (1) {
   ["__PHP_Incomplete_Class_Name"]=>
   string(14) "EchoesOnWakeup"
 }
 Loading metadata for 'a' with allowed_classes
-In wakeup
+In __wakeup 2
 object(EchoesOnWakeup)#2 (0) {
 }
+In __destruct 2
 Loading metadata from 'a' from the new phar
-In wakeup
+In __wakeup 3
 object(EchoesOnWakeup)#3 (0) {
 }
+In __destruct 3
 Loading metadata from 'a' from the new phar with unserialize options
-In wakeup
+In __wakeup 2
 object(EchoesOnWakeup)#2 (0) {
 }
-Caught In sleep at %sphar_metadata_write4.php:9
-object(ThrowsOnSerialize)#3 (0) {
+In __destruct 2
+Caught In sleep at %sphar_metadata_write4.php:12
+In __wakeup 3
+object(EchoesOnWakeup)#3 (0) {
+}
+In __destruct 3
+object(__PHP_Incomplete_Class)#4 (1) {
+  ["__PHP_Incomplete_Class_Name"]=>
+  string(14) "EchoesOnWakeup"
 }