From aafa6ea386fc7334e73a4e9ce60e940683116896 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Mon, 9 Dec 2024 12:13:35 +0100 Subject: [PATCH 1/5] Don't run proc_open_cmd.phpt in parallel with other tests This test puts a fake cmd.exe in the CWD and removes it only after the test has finished. We need to avoid that other tests are running while that fake cmd.exe is there, because they may use it instead of the proper cmd.exe. We also unlink the fake cmd.exe as soon as possible, regardless of the test result. Fixes GH-17098. Closes GH-17090. --- ext/standard/tests/general_functions/proc_open_cmd.phpt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ext/standard/tests/general_functions/proc_open_cmd.phpt b/ext/standard/tests/general_functions/proc_open_cmd.phpt index 6c80871d47df8..2dcc207bfe6ec 100644 --- a/ext/standard/tests/general_functions/proc_open_cmd.phpt +++ b/ext/standard/tests/general_functions/proc_open_cmd.phpt @@ -1,5 +1,7 @@ --TEST-- Harden against cmd.exe hijacking +--CONFLICTS-- +all --SKIPIF-- --EXPECTF-- resource(%d) of type (process) From 142f85e2e18a9574eb1ce941953beb16e679082c Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Fri, 13 Dec 2024 21:19:57 +0100 Subject: [PATCH 2/5] Fix GH-17137: Segmentation fault ext/phar/phar.c Commit edae2431 attempted to fix a leak and double free, but didn't properly understand what was going on, causing a reference count mistake and subsequent segfault in this case. The first mistake of that commit is that the reference count should've been increased because we're reusing a phar object. The error handling path should've gotten changed instead to undo this refcount increase instead of not refcounting at all (root cause of this bug). The second mistake is that the alias isn't supposed to be transferred or whatever, that just doesn't make sense. The reason the test bug69958.phpt originally leaked is because in the non-reuse case we borrowed the alias and otherwise we own the alias. If we own the alias the alias information shouldn't get deleted anyway as that would desync the alias map. Fixing these will reveal a third issue in which the alias memory is not always properly in sync with the persistence-ness of the phar, fix this as well. Closes GH-17150. --- NEWS | 3 +++ ext/phar/phar.c | 1 + ext/phar/phar_object.c | 54 ++++++++++++++++++++++--------------- ext/phar/tests/gh17137.phpt | 31 +++++++++++++++++++++ 4 files changed, 67 insertions(+), 22 deletions(-) create mode 100644 ext/phar/tests/gh17137.phpt diff --git a/NEWS b/NEWS index 63ac5db6b62fc..15453be636dcf 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 6f52fc714d6b7..dfdbbb8fb8390 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 470e1e8a993be..98efcf701c6c4 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 @@ 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,16 @@ 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--; + } + /* fallthrough */ +err_oldpath: + efree(oldpath); + return NULL; } /* }}} */ @@ -2747,7 +2757,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; } diff --git a/ext/phar/tests/gh17137.phpt b/ext/phar/tests/gh17137.phpt new file mode 100644 index 0000000000000..b96036420e931 --- /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 From 0a3442fbe6401e3c53edf91b84c8eaebbd2554d5 Mon Sep 17 00:00:00 2001 From: David Carlier Date: Sun, 15 Dec 2024 10:50:51 +0000 Subject: [PATCH 3/5] ext/pgsql fixing further calls with flexible arguments number. continuation of GH-17161 close GH-17165 --- NEWS | 2 ++ ext/pgsql/pgsql.c | 40 +++++++++++++++++----- ext/pgsql/tests/gh17165.phpt | 65 ++++++++++++++++++++++++++++++++++++ 3 files changed, 99 insertions(+), 8 deletions(-) create mode 100644 ext/pgsql/tests/gh17165.phpt diff --git a/NEWS b/NEWS index 15453be636dcf..29a3290378de8 100644 --- a/NEWS +++ b/NEWS @@ -24,6 +24,8 @@ PHP NEWS - PgSql: . Fixed bug GH-17158 (pg_fetch_result Shows Incorrect ArgumentCountError Message when Called With 1 Argument). (nielsdos) + . Fixed further ArgumentCountError for calls with flexible + number of arguments. (David Carlier) - Phar: . Fixed bug GH-17137 (Segmentation fault ext/phar/phar.c). (nielsdos) diff --git a/ext/pgsql/pgsql.c b/ext/pgsql/pgsql.c index 1db33e5b18225..e955eed0928c8 100644 --- a/ext/pgsql/pgsql.c +++ b/ext/pgsql/pgsql.c @@ -984,12 +984,15 @@ PHP_FUNCTION(pg_query) } link = FETCH_DEFAULT_LINK(); CHECK_DEFAULT_LINK(link); - } else { + } else if (ZEND_NUM_ARGS() == 2) { if (zend_parse_parameters(ZEND_NUM_ARGS(), "Os", &pgsql_link, pgsql_link_ce, &query, &query_len) == FAILURE) { RETURN_THROWS(); } link = Z_PGSQL_LINK_P(pgsql_link); CHECK_PGSQL_LINK(link); + } else { + zend_wrong_parameters_count_error(1, 2); + RETURN_THROWS(); } pgsql = link->conn; @@ -1078,12 +1081,15 @@ PHP_FUNCTION(pg_query_params) } link = FETCH_DEFAULT_LINK(); CHECK_DEFAULT_LINK(link); - } else { + } else if (ZEND_NUM_ARGS() == 3) { if (zend_parse_parameters(ZEND_NUM_ARGS(), "Osa", &pgsql_link, pgsql_link_ce, &query, &query_len, &pv_param_arr) == FAILURE) { RETURN_THROWS(); } link = Z_PGSQL_LINK_P(pgsql_link); CHECK_PGSQL_LINK(link); + } else { + zend_wrong_parameters_count_error(2, 3); + RETURN_THROWS(); } pgsql = link->conn; @@ -1183,12 +1189,15 @@ PHP_FUNCTION(pg_prepare) } link = FETCH_DEFAULT_LINK(); CHECK_DEFAULT_LINK(link); - } else { + } else if (ZEND_NUM_ARGS() == 3) { if (zend_parse_parameters(ZEND_NUM_ARGS(), "Oss", &pgsql_link, pgsql_link_ce, &stmtname, &stmtname_len, &query, &query_len) == FAILURE) { RETURN_THROWS(); } link = Z_PGSQL_LINK_P(pgsql_link); CHECK_PGSQL_LINK(link); + } else { + zend_wrong_parameters_count_error(2, 3); + RETURN_THROWS(); } pgsql = link->conn; @@ -1264,12 +1273,15 @@ PHP_FUNCTION(pg_execute) } link = FETCH_DEFAULT_LINK(); CHECK_DEFAULT_LINK(link); - } else { + } else if (ZEND_NUM_ARGS() == 3) { if (zend_parse_parameters(ZEND_NUM_ARGS(), "Osa", &pgsql_link, pgsql_link_ce, &stmtname, &stmtname_len, &pv_param_arr) == FAILURE) { RETURN_THROWS(); } link = Z_PGSQL_LINK_P(pgsql_link); CHECK_PGSQL_LINK(link); + } else { + zend_wrong_parameters_count_error(2, 3); + RETURN_THROWS(); } pgsql = link->conn; @@ -2024,7 +2036,7 @@ static void php_pgsql_data_info(INTERNAL_FUNCTION_PARAMETERS, int entry_type, bo Z_PARAM_OBJECT_OF_CLASS(result, pgsql_result_ce) Z_PARAM_STR_OR_LONG(field_name, field_offset) ZEND_PARSE_PARAMETERS_END(); - } else { + } else if (ZEND_NUM_ARGS() == 3) { ZEND_PARSE_PARAMETERS_START(3, 3) Z_PARAM_OBJECT_OF_CLASS(result, pgsql_result_ce) if (nullable_row) { @@ -2034,6 +2046,9 @@ static void php_pgsql_data_info(INTERNAL_FUNCTION_PARAMETERS, int entry_type, bo } Z_PARAM_STR_OR_LONG(field_name, field_offset) ZEND_PARSE_PARAMETERS_END(); + } else { + zend_wrong_parameters_count_error(2, 3); + RETURN_THROWS(); } pg_result = Z_PGSQL_RESULT_P(result); @@ -2849,12 +2864,15 @@ PHP_FUNCTION(pg_set_error_verbosity) } link = FETCH_DEFAULT_LINK(); CHECK_DEFAULT_LINK(link); - } else { + } else if (ZEND_NUM_ARGS() == 2) { if (zend_parse_parameters(ZEND_NUM_ARGS(), "Ol", &pgsql_link, pgsql_link_ce, &verbosity) == FAILURE) { RETURN_THROWS(); } link = Z_PGSQL_LINK_P(pgsql_link); CHECK_PGSQL_LINK(link); + } else { + zend_wrong_parameters_count_error(1, 2); + RETURN_THROWS(); } pgsql = link->conn; @@ -2907,12 +2925,15 @@ PHP_FUNCTION(pg_set_client_encoding) } link = FETCH_DEFAULT_LINK(); CHECK_DEFAULT_LINK(link); - } else { + } else if (ZEND_NUM_ARGS() == 2) { if (zend_parse_parameters(ZEND_NUM_ARGS(), "Os", &pgsql_link, pgsql_link_ce, &encoding, &encoding_len) == FAILURE) { RETURN_THROWS(); } link = Z_PGSQL_LINK_P(pgsql_link); CHECK_PGSQL_LINK(link); + } else { + zend_wrong_parameters_count_error(1, 2); + RETURN_THROWS(); } pgsql = link->conn; @@ -2996,12 +3017,15 @@ PHP_FUNCTION(pg_put_line) } link = FETCH_DEFAULT_LINK(); CHECK_DEFAULT_LINK(link); - } else { + } else if (ZEND_NUM_ARGS() == 2) { if (zend_parse_parameters(ZEND_NUM_ARGS(), "Os", &pgsql_link, pgsql_link_ce, &query, &query_len) == FAILURE) { RETURN_THROWS(); } link = Z_PGSQL_LINK_P(pgsql_link); CHECK_PGSQL_LINK(link); + } else { + zend_wrong_parameters_count_error(1, 2); + RETURN_THROWS(); } pgsql = link->conn; diff --git a/ext/pgsql/tests/gh17165.phpt b/ext/pgsql/tests/gh17165.phpt new file mode 100644 index 0000000000000..2fe3f2ad18add --- /dev/null +++ b/ext/pgsql/tests/gh17165.phpt @@ -0,0 +1,65 @@ +--TEST-- +Fix pg_query()/pg_query_params()/pg_prepare()/pg_execute()/pg_set_error_verbosity()/pg_set_client_encoding()/pg_put_line() pg field infos calls ArgumentCountError message. +--EXTENSIONS-- +pgsql +--SKIPIF-- + +--FILE-- +getMessage(), PHP_EOL; +} + +try { + pg_query_params($db, "b", array(), "d"); +} catch (ArgumentCountError $e) { + echo $e->getMessage(), PHP_EOL; +} + +try { + pg_prepare($db, "a", "b", "c"); +} catch (ArgumentCountError $e) { + echo $e->getMessage(), PHP_EOL; +} + +try { + pg_set_error_verbosity($db, 0, PHP_INT_MAX); +} catch (ArgumentCountError $e) { + echo $e->getMessage(), PHP_EOL; +} + +try { + pg_set_client_encoding($db, "foo", "bar"); +} catch (ArgumentCountError $e) { + echo $e->getMessage(), PHP_EOL; +} + +try { + pg_put_line($db, "my", "data"); +} catch (ArgumentCountError $e) { + echo $e->getMessage(), PHP_EOL; +} + +try { + pg_field_is_null($db, false, "myfield", new stdClass()); +} catch (ArgumentCountError $e) { + echo $e->getMessage(), PHP_EOL; +} +?> +--EXPECT-- +pg_query() expects at most 2 arguments, 3 given +pg_query_params() expects at most 3 arguments, 4 given +pg_prepare() expects at most 3 arguments, 4 given +pg_set_error_verbosity() expects at most 2 arguments, 3 given +pg_set_client_encoding() expects at most 2 arguments, 3 given +pg_put_line() expects at most 2 arguments, 3 given +pg_field_is_null() expects at most 3 arguments, 4 given From 1bb4bd657fc6918d1575158dc68bce6eeb660992 Mon Sep 17 00:00:00 2001 From: David Carlier Date: Sun, 15 Dec 2024 15:22:35 +0000 Subject: [PATCH 4/5] fix new pgsql test --- ext/pgsql/tests/gh17165.phpt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ext/pgsql/tests/gh17165.phpt b/ext/pgsql/tests/gh17165.phpt index 2fe3f2ad18add..4fd0189d96443 100644 --- a/ext/pgsql/tests/gh17165.phpt +++ b/ext/pgsql/tests/gh17165.phpt @@ -4,7 +4,7 @@ Fix pg_query()/pg_query_params()/pg_prepare()/pg_execute()/pg_set_error_verbosit pgsql --SKIPIF-- --FILE-- Date: Sun, 15 Dec 2024 16:05:10 +0000 Subject: [PATCH 5/5] fix pgsql config inclusion --- ext/pgsql/tests/gh17165.phpt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ext/pgsql/tests/gh17165.phpt b/ext/pgsql/tests/gh17165.phpt index 4fd0189d96443..b29f822f21c47 100644 --- a/ext/pgsql/tests/gh17165.phpt +++ b/ext/pgsql/tests/gh17165.phpt @@ -10,7 +10,7 @@ include("inc/skipif.inc");