From 4789e0ce398b1dbaa51a30acdd557a4b5191f276 Mon Sep 17 00:00:00 2001 From: "Su, Tao" Date: Tue, 14 Mar 2023 00:48:03 -0700 Subject: [PATCH 1/3] Fix GH-10755: Memory leak in phar_rename_archive() In phar_renmae_archive() context, added one reference but immediately destroyed another, so do not need to increase refcount. With removal of refcount++ line, PHP/Zend no longer reports memory leak. Updated bug69958.phpt test file accordingly. Testing (PASS on both Debug and Release build) Debug: ./configure --enable-debug Release: ./configure 1) Running selected tests. PASS Phar: bug #69958: Segfault in Phar::convertToData on invalid file [bug69958.phpt] 2) All tests under ext/phar/tests PASSED except skipped. ===================================================================== Number of tests : 530 515 Tests skipped : 15 ( 2.8%) -------- Tests warned : 0 ( 0.0%) ( 0.0%) Tests failed : 0 ( 0.0%) ( 0.0%) Tests passed : 515 ( 97.2%) (100.0%) --------------------------------------------------------------------- Time taken : 26 seconds ===================================================================== Signed-off-by: Su, Tao --- ext/phar/phar_object.c | 3 ++- ext/phar/tests/bug69958.phpt | 2 -- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/ext/phar/phar_object.c b/ext/phar/phar_object.c index e32b530b82297..fea27e2847c06 100644 --- a/ext/phar/phar_object.c +++ b/ext/phar/phar_object.c @@ -2114,7 +2114,8 @@ static zend_object *phar_rename_archive(phar_archive_data **sphar, char *ext) /* phar_destroy_phar_data(phar); *sphar = NULL; phar = pphar; - phar->refcount++; + /* FIX: GH-10755 Memory leak in phar_rename_archive() */ + /* phar->refcount++; */ newpath = oldpath; goto its_ok; } diff --git a/ext/phar/tests/bug69958.phpt b/ext/phar/tests/bug69958.phpt index b53c76a104951..577b96ce94acb 100644 --- a/ext/phar/tests/bug69958.phpt +++ b/ext/phar/tests/bug69958.phpt @@ -1,7 +1,5 @@ --TEST-- Phar: bug #69958: Segfault in Phar::convertToData on invalid file ---XFAIL-- -Still has memory leaks, see https://bugs.php.net/bug.php?id=70005 --EXTENSIONS-- phar --FILE-- From 09addda8b796a2f386902a2d608fcafbf514a4d1 Mon Sep 17 00:00:00 2001 From: Tony Su Date: Wed, 15 Mar 2023 13:58:29 +0800 Subject: [PATCH 2/3] Fix GH-10755: Make bug69958.phpt support Linux and Windows According to Niels Dossche, remove slash character (/) in bug69958.php test file in order to support testing on both Linux and Windows. Link: https://github.com/php/php-src/pull/10848 Signed-off-by: Tony Su --- ext/phar/tests/bug69958.phpt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ext/phar/tests/bug69958.phpt b/ext/phar/tests/bug69958.phpt index 577b96ce94acb..447efce57db7c 100644 --- a/ext/phar/tests/bug69958.phpt +++ b/ext/phar/tests/bug69958.phpt @@ -8,8 +8,8 @@ $tarphar = new PharData(__DIR__.'/bug69958.tar'); $phar = $tarphar->convertToData(Phar::TAR); ?> --EXPECTF-- -Fatal error: Uncaught BadMethodCallException: phar "%s/bug69958.tar" exists and must be unlinked prior to conversion in %s/bug69958.php:%d +Fatal error: Uncaught BadMethodCallException: phar "%sbug69958.tar" exists and must be unlinked prior to conversion in %sbug69958.php:%d Stack trace: -#0 %s/bug69958.php(%d): PharData->convertToData(%d) +#0 %sbug69958.php(%d): PharData->convertToData(%d) #1 {main} - thrown in %s/bug69958.php on line %d + thrown in %sbug69958.php on line %d From a3cf48afaba8af8bfcf29ceb1a1c591ca22237b5 Mon Sep 17 00:00:00 2001 From: Tony Su Date: Sat, 18 Mar 2023 09:23:35 +0800 Subject: [PATCH 3/3] FIX GH-10755: Double-free of 'alias' caught by ASAN check After having fixed memory leak, ASAN check reported double-free of phar->alias (char*) string. We found the root cause and documented it at https://github.com/php/php-src/pull/10856 Solution is provided by Ilija Tovilo (iluuu1994). Signed-off-by: Tony Su Reviewed-by: Ilija Tovilo Tested-by: Tony Su --- ext/phar/phar_object.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ext/phar/phar_object.c b/ext/phar/phar_object.c index fea27e2847c06..13fc3079320b4 100644 --- a/ext/phar/phar_object.c +++ b/ext/phar/phar_object.c @@ -2111,6 +2111,9 @@ 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 */ phar_destroy_phar_data(phar); *sphar = NULL; phar = pphar;