Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #80284: Return Value Not Checked from Function Call #6412

Open
wants to merge 1 commit into
base: PHP-7.4
Choose a base branch
from

Conversation

cmb69
Copy link
Contributor

@cmb69 cmb69 commented Nov 9, 2020

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.

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.
@nikic
Copy link
Member

nikic commented Nov 9, 2020

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
Copy link
Contributor Author

cmb69 commented Nov 9, 2020

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:

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?

@ramsey ramsey added the Bug label Jun 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants