diff --git a/NEWS b/NEWS index 63ac5db6b62..15453be636d 100644 --- a/NEWS +++ b/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 diff --git a/ext/phar/phar.c b/ext/phar/phar.c index 6f52fc714d6..dfdbbb8fb83 100644 --- a/ext/phar/phar.c +++ b/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; } diff --git a/ext/phar/phar_object.c b/ext/phar/phar_object.c index 470e1e8a993..98efcf701c6 100644 --- a/ext/phar/phar_object.c +++ b/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; } diff --git a/ext/phar/tests/gh17137.phpt b/ext/phar/tests/gh17137.phpt new file mode 100644 index 00000000000..b96036420e9 --- /dev/null +++ b/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-- +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