Proper fix

The copy function does two things wrong:
- The error recovery logic is a hack that temporarily moves the fp
  pointer to cfp, even though it's not compressed. The respective error
  recovery it talks about is not present in the code, nor is it
  necessary. This is the direct cause of the double free in the original
  reproducer. Fixing this makes it crash in another location though.
- The link following logic is inconsistent and illogical. It cannot be a
  link at this point.

The root cause, after fixing the above issues, is that the file pointers
are not reset properly for the copy. The file pointer need to be the
original ones to perform the copy from the right source, but after that
they need to be set properly to NULL (because fp_type == PHAR_FP).

Author: nielsdos

ext/phar/phar_object.c

@@ -1926,7 +1926,8 @@ static int phar_copy_file_contents(phar_entry_info *entry, php_stream *fp) /* {{
 {
 	char *error;
 	zend_off_t offset;
-	phar_entry_info *link;
+
+	ZEND_ASSERT(!entry->link);
 
 	if (FAILURE == phar_open_entry_fp(entry, &error, 1)) {
 		if (error) {
@@ -1941,26 +1942,14 @@ static int phar_copy_file_contents(phar_entry_info *entry, php_stream *fp) /* {{
 	}
 
 	/* copy old contents in entirety */
-	phar_seek_efp(entry, 0, SEEK_SET, 0, 1);
+	phar_seek_efp(entry, 0, SEEK_SET, 0, 0);
 	offset = php_stream_tell(fp);
-	link = phar_get_link_source(entry);
-
-	if (!link) {
-		link = entry;
-	}
-
-	if (SUCCESS != php_stream_copy_to_stream_ex(phar_get_efp(link, 0), fp, link->uncompressed_filesize, NULL)) {
+	if (SUCCESS != php_stream_copy_to_stream_ex(phar_get_efp(entry, 0), fp, entry->uncompressed_filesize, NULL)) {
 		zend_throw_exception_ex(spl_ce_UnexpectedValueException, 0,
 			"Cannot convert phar archive \"%s\", unable to copy entry \"%s\" contents", entry->phar->fname, entry->filename);
 		return FAILURE;
 	}
 
-	if (entry->fp_type == PHAR_MOD) {
-		/* save for potential restore on error */
-		entry->cfp = entry->fp;
-		entry->fp = NULL;
-	}
-
 	/* set new location of file contents */
 	entry->fp_type = PHAR_FP;
 	entry->offset = offset;
@@ -2298,14 +2287,11 @@ static zend_object *phar_convert_to_other(phar_archive_data *source, int convert
 			/* exception already thrown */
 			return NULL;
 		}
-
-		if (newentry.fp == NULL) {
-			/* entry->fp may be moved to cfp in phar_copy_file_contents
-			 * and be free'd later in phar_flush_ex
-			 * If so, zero fp to avoid double free in rshutdown. */
-			entry->fp = NULL;
-		}
 no_copy:
+		/* Reset file pointers, they have to be reset here such that if a copy happens the original
+		 * source fp can be accessed. */
+		newentry.fp = NULL;
+		newentry.cfp = NULL;
 		newentry.filename = estrndup(newentry.filename, newentry.filename_len);
 
 		phar_metadata_tracker_clone(&newentry.metadata_tracker);

ext/phar/tests/gh18953.phpt

@@ -1,45 +1,43 @@
 --TEST--
-Phar: Stream double free
+GH-18953 (Phar: Stream double free)
 --EXTENSIONS--
 phar
 --INI--
 phar.readonly=0
---ENV--
-USE_ZEND_ALLOC=0
 --FILE--
 <?php
 
-declare(strict_types=1);
-
-require __DIR__ . '/gh18953/autoload.inc';
-
-// cleaning
-@unlink("gh18953.phar");
-@unlink("gh18953.phar.gz");
-
-// create a phar
-$phar = new Phar("gh18953.phar");
-$phar->startBuffering();
-// add any dir
+$phar = new Phar(__DIR__ . "/gh18953.phar");
+$phar->addFromString("file", str_repeat("123", random_int(1, 1)));
 $phar->addEmptyDir("dir");
-$phar->stopBuffering();
-// compress
-$phar->compress(Phar::GZ);
+$phar2 = $phar->compress(Phar::GZ);
+
+var_dump($phar["dir"]);
+var_dump($phar2["dir"]);
+var_dump($phar["file"]->openFile()->fread(100));
+var_dump($phar2["file"]->openFile()->fread(100));
 
-// this increases the chance of reproducing the problem
-// even with this, it's not always reproducible
-$obj1 = new NS1\Class1();
-$obj2 = new NS1\Class1();
-$obj2 = new NS1\Class1();
-$obj2 = new NS1\Class1();
-$obj2 = new NS1\Class1();
+unset($file);
+unset($phar);
 
-echo "Done" . PHP_EOL;
 ?>
 --CLEAN--
 <?php
-@unlink("gh18953.phar");
-@unlink("gh18953.phar.gz");
+@unlink(__DIR__ . "/gh18953.phar");
+@unlink(__DIR__ . "/gh18953.phar.gz");
 ?>
---EXPECT--
-Done
+--EXPECTF--
+object(PharFileInfo)#%d (2) {
+  ["pathName":"SplFileInfo":private]=>
+  string(%d) "%sphar%sdir"
+  ["fileName":"SplFileInfo":private]=>
+  string(3) "dir"
+}
+object(PharFileInfo)#%d (2) {
+  ["pathName":"SplFileInfo":private]=>
+  string(%d) "%sphar.gz%sdir"
+  ["fileName":"SplFileInfo":private]=>
+  string(3) "dir"
+}
+string(3) "123"
+string(3) "123"