From 71aa83b0166bcbae0f3d829f898d4b8ad182488d Mon Sep 17 00:00:00 2001 From: Yun Dou Date: Fri, 27 Jun 2025 11:05:33 +0800 Subject: [PATCH 1/3] Fix stream double free in phar --- ext/phar/phar_object.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/ext/phar/phar_object.c b/ext/phar/phar_object.c index bd724b1036949..854794b79a00f 100644 --- a/ext/phar/phar_object.c +++ b/ext/phar/phar_object.c @@ -2298,6 +2298,13 @@ 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: newentry.filename = estrndup(newentry.filename, newentry.filename_len); From d29a5400b41a5acf01267a613b33aee6d343c6ee Mon Sep 17 00:00:00 2001 From: Yun Dou Date: Fri, 27 Jun 2025 11:40:38 +0800 Subject: [PATCH 2/3] Add test for fix-phar-stream-double-free --- ext/phar/tests/gh18953.phpt | 45 +++++++++++++++++++ ext/phar/tests/gh18953/autoload.inc | 10 +++++ ext/phar/tests/gh18953/src/NS1/Class1.inc | 11 +++++ ext/phar/tests/gh18953/src/NS2/Interface1.inc | 9 ++++ 4 files changed, 75 insertions(+) create mode 100644 ext/phar/tests/gh18953.phpt create mode 100644 ext/phar/tests/gh18953/autoload.inc create mode 100644 ext/phar/tests/gh18953/src/NS1/Class1.inc create mode 100644 ext/phar/tests/gh18953/src/NS2/Interface1.inc diff --git a/ext/phar/tests/gh18953.phpt b/ext/phar/tests/gh18953.phpt new file mode 100644 index 0000000000000..53860edfa3a52 --- /dev/null +++ b/ext/phar/tests/gh18953.phpt @@ -0,0 +1,45 @@ +--TEST-- +Phar: Stream double free +--EXTENSIONS-- +phar +--INI-- +phar.readonly=0 +--ENV-- +USE_ZEND_ALLOC=0 +--FILE-- +startBuffering(); +// add any dir +$phar->addEmptyDir("dir"); +$phar->stopBuffering(); +// compress +$phar->compress(Phar::GZ); + +// 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(); + +echo "Done" . PHP_EOL; +?> +--CLEAN-- + +--EXPECT-- +Done diff --git a/ext/phar/tests/gh18953/autoload.inc b/ext/phar/tests/gh18953/autoload.inc new file mode 100644 index 0000000000000..bf00622767c1e --- /dev/null +++ b/ext/phar/tests/gh18953/autoload.inc @@ -0,0 +1,10 @@ + Date: Sat, 5 Jul 2025 01:59:14 +0200 Subject: [PATCH 3/3] 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). --- ext/phar/phar_object.c | 30 +++------- ext/phar/tests/gh18953.phpt | 55 +++++++++---------- ext/phar/tests/gh18953/autoload.inc | 10 ---- ext/phar/tests/gh18953/src/NS1/Class1.inc | 11 ---- ext/phar/tests/gh18953/src/NS2/Interface1.inc | 9 --- 5 files changed, 33 insertions(+), 82 deletions(-) delete mode 100644 ext/phar/tests/gh18953/autoload.inc delete mode 100644 ext/phar/tests/gh18953/src/NS1/Class1.inc delete mode 100644 ext/phar/tests/gh18953/src/NS2/Interface1.inc diff --git a/ext/phar/phar_object.c b/ext/phar/phar_object.c index 854794b79a00f..cb032095c392d 100644 --- a/ext/phar/phar_object.c +++ b/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); diff --git a/ext/phar/tests/gh18953.phpt b/ext/phar/tests/gh18953.phpt index 53860edfa3a52..52bbace2406d8 100644 --- a/ext/phar/tests/gh18953.phpt +++ b/ext/phar/tests/gh18953.phpt @@ -1,45 +1,40 @@ --TEST-- -Phar: Stream double free +GH-18953 (Phar: Stream double free) --EXTENSIONS-- phar --INI-- phar.readonly=0 ---ENV-- -USE_ZEND_ALLOC=0 --FILE-- 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); -// 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(); +var_dump($phar["dir"]); +var_dump($phar2["dir"]); +var_dump($phar["file"]->openFile()->fread(100)); +var_dump($phar2["file"]->openFile()->fread(100)); -echo "Done" . PHP_EOL; ?> --CLEAN-- ---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" diff --git a/ext/phar/tests/gh18953/autoload.inc b/ext/phar/tests/gh18953/autoload.inc deleted file mode 100644 index bf00622767c1e..0000000000000 --- a/ext/phar/tests/gh18953/autoload.inc +++ /dev/null @@ -1,10 +0,0 @@ -