-
Notifications
You must be signed in to change notification settings - Fork 7.9k
sapi/phpdbg: Grab the function pointer directly #17370
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
Conversation
sapi/phpdbg/phpdbg_prompt.c
Outdated
HashTable *params_ht = NULL; | ||
zend_function *user_fn = zend_hash_str_find_ptr(EG(function_table), lc_name, name->len); | ||
if (user_fn == NULL) { | ||
// TODO Debug for unknown function? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leak on lc_name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah indeed, I don't really know how to write PHPDBG tests, but that would be a good one to cover
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See sapi/phpdbg/tests, it has a PHPDBG section. https://qa.php.net/phpt_details.php#phpdbg_section
9c81fb7
to
4e36429
Compare
I "fixed" the leak in a better way digging at the code, but this whole thing is broken as just registering a function will cause a use after free, moreover, triggering the actual call to the registered function is still eliding me. Considering the |
Internal function won't need their refcount increased as they outlive the debugger session, and userland functions won't be unloaded either. So no refcount management is necessary for registered functions.
Allocating a string zval to let zend_call_function() do the same thing is slightly pointless
4e36429
to
3034f07
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One nit, ok otherwise LGTM
I propose that when this is merged "fix-15981" gets backported to 8.3+. |
Agreed, that makes sense to me too |
Allocating a string zval to let zend_call_function() do the same thing is slightly pointless