diff --git a/NEWS b/NEWS index 94c80419b91..e01a9549663 100644 --- a/NEWS +++ b/NEWS @@ -40,6 +40,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 b50142914ec..a4a9b7d2139 100644 --- a/ext/phar/phar.c +++ b/ext/phar/phar.c @@ -1506,6 +1506,7 @@ zend_result phar_create_or_parse_filename(char *fname, size_t fname_len, char *a } } + 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 25f0d7d9fa6..dbc3f91b983 100644 --- a/ext/phar/phar_object.c +++ b/ext/phar/phar_object.c @@ -2120,9 +2120,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))) { @@ -2134,41 +2133,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); @@ -2178,20 +2178,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_ex(phar, NULL, 1, &error); @@ -2201,8 +2202,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); @@ -2225,6 +2225,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; } /* }}} */ @@ -2754,8 +2764,13 @@ valid_alias: oldalias_len = phar_obj->archive->alias_len; old_temp = phar_obj->archive->is_temporary_alias; - phar_obj->archive->alias = estrndup(ZSTR_VAL(new_alias), ZSTR_LEN(new_alias)); phar_obj->archive->alias_len = ZSTR_LEN(new_alias); + if (phar_obj->archive->alias_len) { + phar_obj->archive->alias = pestrndup(ZSTR_VAL(new_alias), ZSTR_LEN(new_alias), phar_obj->archive->is_persistent); + } else { + phar_obj->archive->alias = NULL; + } + phar_obj->archive->is_temporary_alias = 0; phar_flush(phar_obj->archive, &error); 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