Fix stream double free in phar

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).

Closes GH-19035.

Co-authored-by: Yun Dou <[email protected]>

Author: ndossche

NEWS

@@ -37,6 +37,9 @@ PHP                                                                        NEWS
   . Fixed bug GH-18958 (Fatal error during shutdown after pcntl_rfork() or
     pcntl_forkx() with zend-max-execution-timers). (Arnaud)
 
+- Phar:
+  . Fix stream double free in phar. (nielsdos, dixyes)
+
 - SOAP:
   . Fixed bug GH-18990, bug #81029, bug #47314 (SOAP HTTP socket not closing
     on object destruction). (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;
@@ -2299,6 +2288,10 @@ static zend_object *phar_convert_to_other(phar_archive_data *source, int convert
 			return 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

@@ -0,0 +1,40 @@
+--TEST--
+GH-18953 (Phar: Stream double free)
+--EXTENSIONS--
+phar
+--INI--
+phar.readonly=0
+--FILE--
+<?php
+
+$phar = new Phar(__DIR__ . "/gh18953.phar");
+$phar->addFromString("file", str_repeat("123", random_int(1, 1)));
+$phar->addEmptyDir("dir");
+$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));
+
+?>
+--CLEAN--
+<?php
+@unlink(__DIR__ . "/gh18953.phar");
+@unlink(__DIR__ . "/gh18953.phar.gz");
+?>
+--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"