Fix GH-17137: Segmentation fault ext/phar/phar.c

Commit edae2431 attempted to fix a leak and double free, but didn't
properly understand what was going on, causing a reference count mistake
and subsequent segfault in this case.

The first mistake of that commit is that the reference count should've
been increased because we're reusing a phar object. The error handling
path should've gotten changed instead to undo this refcount increase
instead of not refcounting at all (root cause of this bug).

The second mistake is that the alias isn't supposed to be transferred or
whatever, that just doesn't make sense. The reason the test
bug69958.phpt originally leaked is because in the non-reuse case we
borrowed the alias and otherwise we own the alias. If we own the alias
the alias information shouldn't get deleted anyway as that would desync
the alias map.

Fixing these will reveal a third issue in which the alias memory is not
always properly in sync with the persistence-ness of the phar, fix this
as well.

Closes GH-17150.

NEWS

@@ -25,6 +25,9 @@ PHP                                                                        NEWS
   . Fixed bug GH-17158 (pg_fetch_result Shows Incorrect ArgumentCountError
     Message when Called With 1 Argument). (nielsdos)
 
+- Phar:
+  . Fixed bug GH-17137 (Segmentation fault ext/phar/phar.c). (nielsdos)
+
 - SimpleXML:
   . Fixed bug GH-17040 (SimpleXML's unset can break DOM objects). (nielsdos)
   . Fixed bug GH-17153 (SimpleXML crash when using autovivification on

ext/phar/phar.c

@@ -1504,6 +1504,7 @@ int phar_create_or_parse_filename(char *fname, size_t fname_len, char *alias, si
 			}
 		}
 
+		ZEND_ASSERT(!mydata->is_persistent);
 		mydata->alias = alias ? estrndup(alias, alias_len) : estrndup(mydata->fname, fname_len);
 		mydata->alias_len = alias ? alias_len : fname_len;
 	}

ext/phar/phar_object.c

@@ -2112,9 +2112,8 @@ static zend_object *phar_rename_archive(phar_archive_data **sphar, char *ext) /*
 	efree(newname);
 
 	if (PHAR_G(manifest_cached) && NULL != (pphar = zend_hash_str_find_ptr(&cached_phars, newpath, phar->fname_len))) {
-		efree(oldpath);
 		zend_throw_exception_ex(spl_ce_BadMethodCallException, 0, "Unable to add newly converted phar \"%s\" to the list of phars, new phar name is in phar.cache_list", phar->fname);
-		return NULL;
+		goto err_oldpath;
 	}
 
 	if (NULL != (pphar = zend_hash_str_find_ptr(&(PHAR_G(phar_fname_map)), newpath, phar->fname_len))) {
@@ -2126,41 +2125,42 @@ static zend_object *phar_rename_archive(phar_archive_data **sphar, char *ext) /*
 				pphar->flags = phar->flags;
 				pphar->fp = phar->fp;
 				phar->fp = NULL;
-				/* FIX: GH-10755 Double-free issue caught by ASAN check */
-				pphar->alias = phar->alias; /* Transfer alias to pphar to */
-				phar->alias = NULL;         /* avoid being free'd twice   */
+				/* The alias is not owned by the phar, so set it to NULL to avoid freeing it. */
+				phar->alias = NULL;
 				phar_destroy_phar_data(phar);
 				*sphar = NULL;
 				phar = pphar;
+				/* NOTE: this phar is now reused, so the refcount must be increased. */
+				phar->refcount++;
 				newpath = oldpath;
 				goto its_ok;
 			}
 		}
 
-		efree(oldpath);
 		zend_throw_exception_ex(spl_ce_BadMethodCallException, 0, "Unable to add newly converted phar \"%s\" to the list of phars, a phar with that name already exists", phar->fname);
-		return NULL;
+		goto err_oldpath;
 	}
 its_ok:
 	if (SUCCESS == php_stream_stat_path(newpath, &ssb)) {
 		zend_throw_exception_ex(spl_ce_BadMethodCallException, 0, "phar \"%s\" exists and must be unlinked prior to conversion", newpath);
-		efree(oldpath);
-		return NULL;
+		goto err_reused_oldpath;
 	}
 	if (!phar->is_data) {
 		if (SUCCESS != phar_detect_phar_fname_ext(newpath, phar->fname_len, (const char **) &(phar->ext), &ext_len, 1, 1, 1)) {
-			efree(oldpath);
 			zend_throw_exception_ex(spl_ce_BadMethodCallException, 0, "phar \"%s\" has invalid extension %s", phar->fname, ext);
-			return NULL;
+			goto err_reused_oldpath;
 		}
 		phar->ext_len = ext_len;
 
-		if (phar->alias) {
+		/* If we are reusing a phar, then the aliases should be already set up correctly,
+		 * and so we should not clear out the alias information.
+		 * This would also leak memory because, unlike the non-reuse path, we actually own the alias memory. */
+		if (phar->alias && phar != pphar) {
 			if (phar->is_temporary_alias) {
 				phar->alias = NULL;
 				phar->alias_len = 0;
 			} else {
-				phar->alias = estrndup(newpath, strlen(newpath));
+				phar->alias = pestrndup(newpath, strlen(newpath), phar->is_persistent);
 				phar->alias_len = strlen(newpath);
 				phar->is_temporary_alias = 1;
 				zend_hash_str_update_ptr(&(PHAR_G(phar_alias_map)), newpath, phar->fname_len, phar);
@@ -2170,20 +2170,21 @@ its_ok:
 	} else {
 
 		if (SUCCESS != phar_detect_phar_fname_ext(newpath, phar->fname_len, (const char **) &(phar->ext), &ext_len, 0, 1, 1)) {
-			efree(oldpath);
 			zend_throw_exception_ex(spl_ce_BadMethodCallException, 0, "data phar \"%s\" has invalid extension %s", phar->fname, ext);
-			return NULL;
+			goto err_reused_oldpath;
 		}
 		phar->ext_len = ext_len;
 
-		phar->alias = NULL;
-		phar->alias_len = 0;
+		/* See comment in other branch. */
+		if (phar != pphar) {
+			phar->alias = NULL;
+			phar->alias_len = 0;
+		}
 	}
 
 	if ((!pphar || phar == pphar) && NULL == zend_hash_str_update_ptr(&(PHAR_G(phar_fname_map)), newpath, phar->fname_len, phar)) {
-		efree(oldpath);
 		zend_throw_exception_ex(spl_ce_BadMethodCallException, 0, "Unable to add newly converted phar \"%s\" to the list of phars", phar->fname);
-		return NULL;
+		goto err_oldpath;
 	}
 
 	phar_flush(phar, 0, 0, 1, &error);
@@ -2193,8 +2194,7 @@ its_ok:
 		*sphar = NULL;
 		zend_throw_exception_ex(spl_ce_BadMethodCallException, 0, "%s", error);
 		efree(error);
-		efree(oldpath);
-		return NULL;
+		goto err_oldpath;
 	}
 
 	efree(oldpath);
@@ -2217,6 +2217,16 @@ its_ok:
 	zend_call_known_instance_method_with_1_params(ce->constructor, Z_OBJ(ret), NULL, &arg1);
 	zval_ptr_dtor(&arg1);
 	return Z_OBJ(ret);
+
+err_reused_oldpath:
+	if (pphar == phar) {
+		/* NOTE: we know it's not the last reference because the phar is reused. */
+		phar->refcount--;
+	}
+	/* fallthrough */
+err_oldpath:
+	efree(oldpath);
+	return NULL;
 }
 /* }}} */
 
@@ -2747,7 +2757,7 @@ valid_alias:
 	old_temp = phar_obj->archive->is_temporary_alias;
 
 	if (alias_len) {
-		phar_obj->archive->alias = estrndup(alias, alias_len);
+		phar_obj->archive->alias = pestrndup(alias, alias_len, phar_obj->archive->is_persistent);
 	} else {
 		phar_obj->archive->alias = NULL;
 	}

ext/phar/tests/gh17137.phpt

@@ -0,0 +1,31 @@
+--TEST--
+GH-17137 (Segmentation fault ext/phar/phar.c)
+--EXTENSIONS--
+phar
+--INI--
+phar.readonly=0
+--FILE--
+<?php
+$file = __DIR__ . DIRECTORY_SEPARATOR . 'gh17137.phar';
+$phar = new Phar($file);
+var_dump($phar, $phar->decompress());
+echo "OK\n";
+?>
+--EXPECTF--
+object(Phar)#%d (3) {
+  ["pathName":"SplFileInfo":private]=>
+  string(0) ""
+  ["glob":"DirectoryIterator":private]=>
+  bool(false)
+  ["subPathName":"RecursiveDirectoryIterator":private]=>
+  string(0) ""
+}
+object(Phar)#%d (3) {
+  ["pathName":"SplFileInfo":private]=>
+  string(0) ""
+  ["glob":"DirectoryIterator":private]=>
+  bool(false)
+  ["subPathName":"RecursiveDirectoryIterator":private]=>
+  string(0) ""
+}
+OK