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

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 @@ static zend_object *phar_rename_archive(phar_archive_data **sphar, char *ext) /*
 	} 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 @@ static zend_object *phar_rename_archive(phar_archive_data **sphar, char *ext) /*
 		*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,15 @@ static zend_object *phar_rename_archive(phar_archive_data **sphar, char *ext) /*
 	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--;
+	}
+err_oldpath:
+	efree(oldpath);
+	return NULL;
 }
 /* }}} */
 
@@ -2747,7 +2756,7 @@ PHP_METHOD(Phar, setAlias)
 	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";
+?>
+--EXPECT--
+object(Phar)#1 (3) {
+  ["pathName":"SplFileInfo":private]=>
+  string(0) ""
+  ["glob":"DirectoryIterator":private]=>
+  bool(false)
+  ["subPathName":"RecursiveDirectoryIterator":private]=>
+  string(0) ""
+}
+object(Phar)#2 (3) {
+  ["pathName":"SplFileInfo":private]=>
+  string(0) ""
+  ["glob":"DirectoryIterator":private]=>
+  bool(false)
+  ["subPathName":"RecursiveDirectoryIterator":private]=>
+  string(0) ""
+}
+OK