Aggressively use actual function parameters in `php_verror` by NattyNarwhal · Pull Request #12276 · php/php-src

NattyNarwhal · 2023-09-22T18:18:11Z

PHP errors used to not show parameter info consistently. Make it so that it uses a backtrace to get function info, similar to how exceptions work.

This makes the docref error functions' parameter argument mostly vestigal, being used only if allocation fails basically. The parameter argument may be useful in the case it is more verbose than the actual function args (is there a case?).

This is an INI option, so that this behaviour can be turned off. We do so for tests, as to avoid rewriting most EXPECTF sections. This can be changed, of course.

See GH-12048. (Updated in 2026-03-04.)

NattyNarwhal · 2023-09-22T18:20:57Z

The new hotness:

calvin@anika php-src % sapi/cli/php ~/src/chmod.php

Warning: unlink('/tmp'): Operation not permitted in /Users/calvin/src/chmod.php on line 3

Warning: chown('/', 'calvin'): Operation not permitted in /Users/calvin/src/chmod.php on line 4

Warning: chmod('/', 511): Operation not permitted in /Users/calvin/src/chmod.php on line 5

Versus the old busted stuff:

calvin@anika php-src % /opt/calvin/php/bin//php ~/src/chmod.php

Warning: unlink(/tmp): Operation not permitted in /Users/calvin/src/chmod.php on line 3

Warning: chown(): Operation not permitted in /Users/calvin/src/chmod.php on line 4

Warning: chmod(): Operation not permitted in /Users/calvin/src/chmod.php on line 5

Girgias

Thanks for looking into this!

NattyNarwhal · 2023-09-25T19:48:28Z

The big awful annoyance is going to be rewriting pretty much every test file. I know there's been other wide sweeping commits that changed a lot of stuff before though. Were they doing anything automated to clean those up?

iluuu1994 · 2023-09-25T20:23:00Z

@NattyNarwhal run-tests.php --bless

NattyNarwhal · 2023-09-26T17:42:05Z

That works, although paths are kinda annoying - all the absolute paths are there, and truncated, i.e. Warning: symlink('/Users/calvin/s...', '../bad/./symlin...'):. At least it fixed the resource IDs. I'm considering changing bless to make this easier to deal with.

Girgias · 2023-09-26T18:13:44Z

That works, although paths are kinda annoying - all the absolute paths are there, and truncated, i.e. Warning: symlink('/Users/calvin/s...', '../bad/./symlin...'):. At least it fixed the resource IDs. I'm considering changing bless to make this easier to deal with.

Changing bless probably makes sense as part of this PR yes, ideally it should replace those with %s

NattyNarwhal · 2023-09-26T18:25:28Z

So my change to bless as below:

diff --git a/scripts/dev/bless_tests.php b/scripts/dev/bless_tests.php
index fa49647fcf..2422512880 100755
--- a/scripts/dev/bless_tests.php
+++ b/scripts/dev/bless_tests.php
@@ -72,6 +72,8 @@ function normalizeOutput(string $out): string {
         'Resource ID#%d used as offset, casting to integer (%d)',
         $out);
     $out = preg_replace('/string\(\d+\) "([^"]*%d)/', 'string(%d) "$1', $out);
+    // Replace absolute paths, particularly those truncated; they're likely to have your homedir in it
+    $out = preg_replace("/'\\/.*\.\\.\\.'/", "'%s'", $out);
     $out = str_replace("\0", '%0', $out);
     return $out;
 }

...does work, like so:

-Warning: fileperms(): stat failed for /no/such/file/dir in %s on line %d
+Warning: fileperms('%s'): stat failed for /no/such/file/dir in %s on line %d

Of course, bless is also a little insensitive, so you do have to manually postprocess these (unless there's a better way to do it?)

-Warning: chmod(): %s in %s on line %d
+Warning: chmod('/etc/passwd', 511): Operation not permitted in %s on line %d

To speak nothing of the tests I can't run because i.e. Windows.

Girgias · 2023-09-26T18:31:00Z

One thing that I just thought about is that if the parameter is going to be displayed it should be suppressed if the SensitiveParam attribute is used.

NattyNarwhal · 2023-09-26T18:37:37Z

One thing that I just thought about is that if the parameter is going to be displayed it should be suppressed if the SensitiveParam attribute is used.

The code that prints the arguments in errors is shared with code that prints backtraces i.e. on exceptions. Both cases are handled, so you'll get Warning: odbc_connect('bogusdsn', 'user', Object(SensitiveParameterValue)): SQL error: [unixODBC][Driver Manager]Data source name not found and no default driver specified, SQL state IM002 in SQLConnect and Stack trace: #0 /Users/calvin/src/chmod.php(10): sensitive(Object(SensitiveParameterValue)).

NattyNarwhal · 2023-09-27T17:42:00Z

One thing that came to mind was turning this into an INI option, and it could be off for just the test suite or by default in general if the new output is intrusive. That said, I don't think configurability is a good idea (in terms of making the option used, and the PHP stance on introducing new options in general), but bringing it up anyways.

Girgias · 2023-09-27T19:38:25Z

Yeah not a fan about the INI setting :D

NattyNarwhal · 2023-09-28T17:47:33Z

I've pushed changes to ext/standard/tests/file, mostly because it would likely have a ton of absolute paths in the new parameters, and a small enough set to check bless output without getting overwhelmed. Unfortunately, I still had to change a lot of the test outputs back (or further manually) in cases where bless was overzealous, ignorant (still a lot of absolute paths in error messages), or a bit naive (i.e. fscanf test putting format strings like %d in paramters, which confuses EXPECTF). The question is if there's a better way to scale this up to all the other tests.

NattyNarwhal · 2024-12-12T18:29:44Z

I've rebased this onto master and added an INI option to gate this behind. I don't like adding INI options, but it might be a lesser evil than touching every PHPT file.

bwoebi · 2024-12-13T03:37:15Z

I don't think we should do this. Like, adding configs, just because it avoids updating some tests. What we were talking about over in the other PR is displaying the stacktrace, which anyway already has an ini.

bukka · 2024-12-14T16:02:23Z

I thought about this and the INI actually makes sense here. The reason is that this is more an operational thing. It's really just not about avoiding the tests updates. The thing is that for some legacy projects where warnings happening quite often (I saw quite a few such projects in past), this can lead not only to a significant increase of the log space but to the potential compliance issues. An example of that might be a function receiving email addresses and compliance with GDPR or similar. I realise that there is such potential with stack traces already but those are usually less common than warnings in the logs. So for some users it might be convenient to disable logging of parameters so having such INI seems reasonable to me.

NattyNarwhal · 2024-12-14T18:57:58Z

I'm not sure if you're talking about GH-17056, this PR, or both. Sorry for any confusion by mentioning that PR on a commit here; the discussion in that PR was motivating me to revive this PR I had.

bukka · 2024-12-14T23:59:37Z

I was talking about this PR but it could apply to both. But this one can especially increase the log size as fatal errors are not that common. However if you have lots regular warning in logs and they suddenly get all params logged, then it can increase the size and cost. So having an option to disable is a good think IMHO.

DanielEScherzer · 2026-02-26T23:54:51Z

@NattyNarwhal happy to review this once rebased

DanielEScherzer

please

add some tests to show the new behavior when this is enabled
update the example INI files php.ini-development and php.ini-production

DanielEScherzer · 2026-03-03T17:47:27Z

-		origin_len = spprintf(&origin, 0, "%s%s%s(%s)", class_name, space, function, params);
+		zend_string *dynamic_params = NULL;
+		/*
+		 * By default, this is disabled because it requires rewriting


I suggest taking a look at https://wiki.php.net/rfc/error_backtraces_v2 and its discussion - you can have the default be true generally, and then false for tests

NattyNarwhal · 2026-03-03T21:40:28Z

Made the suggested changes. I'm also going to write to internals@ soonish and almost certainly prepare an RFC. When I originally wrote this, I knew much less about the process (and the v2 backtrace RFC wasn't around yet).

TimWolla · 2026-03-06T09:41:35Z

+; Default Value: Off
+; Development Value: On
+; Production Value: On
+display_error_function_args = On


Related to Daniel's comment: Please avoid further INIs where the builtin default differs from the recommended “production default”.

Relatedly, we should probably finally drop zend.exception_ignore_args=Off from php.ini-production /cc @bwoebi.

Do you mean zend.exception_ignore_args=On? Because that's what currently is in php.ini-production if I'm not mistaken. Do you think the information disclosure is no longer an issue?

Do you mean zend.exception_ignore_args=On?

Yes, thank you.

Do you think the information disclosure is no longer an issue?

display_errors=Off should remain and for parameters in logs there is #[\SensitiveParameter] and “just changing the INI”. The Docker images also don't load any INI by default, thus for them the builtin default is already being used.

Going back to the original comment of avoiding defaults which are different from production recommendations: Is there a consensus that the default should be production settings?

Because some of the main settings like display_errors and error_reporting have Development as a default and I probably missed the discussion on whether a default configuration is aimed at development or production, i.e. prioritize providing as much information to the developers as possible or try to prevent information leakage on production systems as much as possible.

I can see arguments for both and don't have a strong opinion, I'm just pointing out that I'm unaware of a clear decision.

This will be resolved however the RFC vote goes, whenever that happens.

TimWolla · 2026-03-31T07:16:59Z

                '%s %s "%s" %s',
-                PHP_BINARY, $ini ? "-n -c $ini" : "",
+                // XXX: TEST_PHP_EXTRA_ARGS for run-test values won't work here?
+                PHP_BINARY, $ini ? "-n -c $ini -d error_ignore_args=1" : "",


Is this expected to emit errors during regular operation? Otherwise I would find it okay to leave out that option here and fix a (small) number of tests.

One test (sapi/cli/tests/gh18582.phpt) does so. I set it here to make them consistent across all tests in case more are added; if not appropriate, it can be moved to that test with the cmd_args parameter on the server function.

NattyNarwhal · 2026-04-02T22:28:02Z

test failures unrelated, having the same on master

NattyNarwhal · 2026-05-15T21:12:18Z

RFC was approved; INI subvote indicates it should be set to 0 in the default INIs and in tests.

TimWolla · 2026-05-16T12:42:53Z

+ZEND_API zend_string *zend_trace_function_args_to_string(const HashTable *frame) {
+	zval *tmp;
+	smart_str str = {0};
+	/* init because ASan will complain */


Complain about what?

Been a while, but pretty sure it complained if the smart_str never had anything initialized, which is fairly obvious.

smart_str str = {0}; will initialize the entire struct to zero. If it would not, appending an empty string would also be unsafe, because it works on garbage data.

TimWolla · 2026-05-16T12:44:21Z

+	smart_str_0(&str);
+	return str.s ? str.s : ZSTR_EMPTY_ALLOC();


Suggested change

smart_str_0(&str);

return str.s ? str.s : ZSTR_EMPTY_ALLOC();

return smart_str_extract(&str);

Good idea; I cribbed the pattern from other functions in this file, so probably should file a PR to fix those up too...

Cleaning up the other functions would be nice indeed!

PHP errors used to not show parameter info consistently. Make it so that it uses a backtrace to get function info, similar to how exceptions work. This makes the docref error functions' parameter argument mostly vestigal, being used only if allocation fails basically. Several tests will fail from the fact we include function params. One annoyance is that _build_trace_args truncates strings according to exception_string_param_max_len. See phpGH-12048

This is a useful feature, but enabling it by default requires rewriting every PHPT file's output section. Since that would be a hellish diff to make and to review, I think the best option is unfortunately, another INI option. We can enable this for prod/dev recommended INIs, but make sure it's disabled for the test runner. This takes some inspiration from the discussion in phpGH-17056, which has similar problems to this PR.

If this is not enabled by default for tests (like the fatal error backtrace RFC), then at least test for it.

Per feedback from Tim on the RFC. Also rationalize the default vs. recommended INI settings. This does invert the semantics for the option; the if is changed accordingly.

Almost certainly a better way to do this...

Avoid a negative which is harder to reason about; matches RFC change

RFC is going towards adding this, but disabling it by default.

TimWolla · 2026-05-17T08:57:27Z

+	if (Z_TYPE(backtrace) != IS_ARRAY) {
+		return NULL;
+	}
+	zval *first_frame = zend_hash_index_find(Z_ARRVAL(backtrace), 0);
+	if (first_frame) {
+		dynamic_params = zend_trace_function_args_to_string(Z_ARRVAL_P(first_frame));
+	}


With the return NULL; for “not an array” you are leaking memory if a non-array refcounted value is returned (for whatever reason). I suggest:

Suggested change

if (Z_TYPE(backtrace) != IS_ARRAY) {

return NULL;

}

zval *first_frame = zend_hash_index_find(Z_ARRVAL(backtrace), 0);

if (first_frame) {

dynamic_params = zend_trace_function_args_to_string(Z_ARRVAL_P(first_frame));

}

if (Z_TYPE(backtrace) == IS_ARRAY) {

zval *first_frame = zend_hash_index_find(Z_ARRVAL(backtrace), 0);

if (first_frame) {

dynamic_params = zend_trace_function_args_to_string(Z_ARRVAL_P(first_frame));

}

}

Since dynamic_params is initialized to NULL this should just work.

TimWolla · 2026-05-17T08:59:05Z

+ZEND_API zend_string *zend_trace_function_args_to_string(const HashTable *frame) {
+	zval *tmp;
+	smart_str str = {0};
+	/* init because ASan will complain */


smart_str str = {0}; will initialize the entire struct to zero. If it would not, appending an empty string would also be unsafe, because it works on garbage data.

TimWolla · 2026-05-17T08:59:24Z

+/* {{{ Gets the function arguments printed as a string from a backtrace frame. */
+ZEND_API zend_string *zend_trace_function_args_to_string(const HashTable *frame) {
+	smart_str str = {0};
+	smart_str_appends(&str, "");


This does nothing (useful):

Suggested change

smart_str_appends(&str, "");

TimWolla · 2026-05-17T09:00:37Z

 }
 /* }}} */

+static void _build_trace_args_list(zval *tmp, smart_str *str) /* {{{ */


Suggested change

static void _build_trace_args_list(zval *tmp, smart_str *str) /* {{{ */

static void build_trace_args_list(zval *tmp, smart_str *str) /* {{{ */

Identifiers starting with an underscore are reserved. The static is sufficient to make the function private.

TimWolla · 2026-05-17T09:02:08Z

+static void _build_trace_args_list(zval *tmp, smart_str *str) /* {{{ */
+{
+	if (EXPECTED(Z_TYPE_P(tmp) == IS_ARRAY)) {
+		size_t last_len = ZSTR_LEN(str->s);


I know you just moved the code, but this should be smart_str_get_len() (which safely handles the case where no allocation has been made yet (which might be the reason you got the ASAN issues?).

A simpler solution might also be to append the , within the loop and just have a boolean value “first” or “not first”. This avoids manually touching the length of the backing string of the smart_str.

NattyNarwhal requested review from bukka and iluuu1994 as code owners September 22, 2023 18:18

github-actions Bot added the Category: Engine label Sep 22, 2023

NattyNarwhal marked this pull request as draft September 22, 2023 18:18

Girgias reviewed Sep 23, 2023

View reviewed changes

Comment thread Zend/zend_exceptions.c Outdated

github-actions Bot added the Category: Build System label Sep 27, 2023

github-actions Bot added the Extension: standard label Sep 28, 2023

NattyNarwhal force-pushed the verror-consistent-params branch from 902de2c to 44b8dcd Compare December 12, 2024 18:29

github-actions Bot added the ABI break label Dec 12, 2024

NattyNarwhal requested a review from DanielEScherzer February 26, 2026 22:34

NattyNarwhal force-pushed the verror-consistent-params branch from 90ec7b5 to 7434069 Compare March 3, 2026 16:07

DanielEScherzer reviewed Mar 3, 2026

View reviewed changes

TimWolla reviewed Mar 6, 2026

View reviewed changes

iluuu1994 added the RFC label Mar 6, 2026

NattyNarwhal force-pushed the verror-consistent-params branch from 41d448e to 7a81b0e Compare March 30, 2026 20:29

github-actions Bot added SAPI: cli Extension: openssl labels Mar 30, 2026

TimWolla reviewed Mar 31, 2026

View reviewed changes

NattyNarwhal force-pushed the verror-consistent-params branch from b289770 to 3c58215 Compare May 12, 2026 21:29

NattyNarwhal marked this pull request as ready for review May 16, 2026 03:15

TimWolla requested changes May 16, 2026

View reviewed changes

NattyNarwhal mentioned this pull request May 16, 2026

Change bless to clean up absolute paths #22067

Open

NattyNarwhal and others added 8 commits May 16, 2026 18:49

Add test for display_error_function_args

060d971

If this is not enabled by default for tests (like the fatal error backtrace RFC), then at least test for it.

Rename option to error_ignore_args

83a6868

Per feedback from Tim on the RFC. Also rationalize the default vs. recommended INI settings. This does invert the semantics for the option; the if is changed accordingly.

Fix this test since it calls PHP itself

3f0e8cf

Almost certainly a better way to do this...

Quick hack to placate Windows OpenSSL tests w/ new INI opt

6e88792

Rename to error_include_args

8e0e56b

Avoid a negative which is harder to reason about; matches RFC change

Default error_include_args to Off

9e866be

RFC is going towards adding this, but disabling it by default.

NattyNarwhal force-pushed the verror-consistent-params branch from 9676ead to 9e866be Compare May 16, 2026 21:50

TimWolla requested changes May 17, 2026

View reviewed changes

		smart_str_0(&str);
		return str.s ? str.s : ZSTR_EMPTY_ALLOC();

	smart_str_0(&str);
	return str.s ? str.s : ZSTR_EMPTY_ALLOC();
	return smart_str_extract(&str);

	static void _build_trace_args_list(zval tmp, smart_str str) /* {{{ */
	static void build_trace_args_list(zval tmp, smart_str str) /* {{{ */

Conversation

NattyNarwhal commented Sep 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NattyNarwhal commented Sep 22, 2023

Uh oh!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NattyNarwhal commented Sep 25, 2023

Uh oh!

iluuu1994 commented Sep 25, 2023

Uh oh!

NattyNarwhal commented Sep 26, 2023

Uh oh!

Girgias commented Sep 26, 2023

Uh oh!

NattyNarwhal commented Sep 26, 2023

Uh oh!

Girgias commented Sep 26, 2023

Uh oh!

NattyNarwhal commented Sep 26, 2023

Uh oh!

NattyNarwhal commented Sep 27, 2023

Uh oh!

Girgias commented Sep 27, 2023

Uh oh!

NattyNarwhal commented Sep 28, 2023

Uh oh!

NattyNarwhal commented Dec 12, 2024

Uh oh!

bwoebi commented Dec 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bukka commented Dec 14, 2024

Uh oh!

NattyNarwhal commented Dec 14, 2024

Uh oh!

bukka commented Dec 14, 2024

Uh oh!

DanielEScherzer commented Feb 26, 2026

Uh oh!

DanielEScherzer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NattyNarwhal commented Mar 3, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NattyNarwhal commented Apr 2, 2026

Uh oh!

NattyNarwhal commented May 15, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NattyNarwhal commented Sep 22, 2023 •

edited

Loading

bwoebi commented Dec 13, 2024 •

edited

Loading