Fix #80284: Return Value Not Checked from Function Call

[cmb69]

We assert that retval IS_UNDEF if call_user_function(), to catch
a potentially uninitialized retval. This might also help static code
analyzers to avoid raising false positives.

---

I don't see any issues with the current code, but still it might be a good idea to assert this condition. Probably PHP-8.0 or the "master" branch would be more appropriate targets for this change.

[nikic]

I don't think this change makes sense. This is just asserting the contract of the function.

Longer term, we should probably make call_user_function() effectively infallible, by throwing if the function cannot be called.

[cmb69]

I think we should at least document such contracts by adding respective comments, since there appears to be some confusion, e.g. no need to ZVAL_FALSE(&retval) here:

php-src/ext/standard/assert.c

Lines 179 to 189 in b6a93c3

	ZVAL_FALSE(&retval);
	if (description_str) {
	ZVAL_STR(&args[3], description_str);
	call_user_function(NULL, NULL, &ASSERTG(callback), &retval, 4, args);
	} else {
	call_user_function(NULL, NULL, &ASSERTG(callback), &retval, 3, args);
	}
	zval_ptr_dtor(&args[0]);
	zval_ptr_dtor(&retval);

Making _call_user_function_impl() infallible is fine for me; or maybe add assertions to the function?

[krakjoe]

Closing this, it was opened against a branch we no longer support a very long time ago.

There was some confusion/disagreement about whether the change is worth it, or correct.

If you'd like to revisit the topic, please open a pull against a supported branch.

On a side note, it never really looked necessary to me to make any change, you should check the return value of zend_call_function, when you need too; if it makes no difference what the return value is in the branch that called it, then it looks like a pointless additional branch to me.