Skip to content

Add next handler parameter to zend_observer_remove_begin/end_handler #13807

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

Merged
merged 1 commit into from
Apr 7, 2024

Conversation

bwoebi
Copy link
Member

@bwoebi bwoebi commented Mar 25, 2024

The usage of the current API within an observer handler leads to bugs like https://bugs.xdebug.org/view.php?id=2232. Given two observer handlers, next to each other. The first one is executed. It removes itself. The second observer handler gets moved to the place of the first. The first one returns. The handler in the second slot is fetched, but is now NULL, because the it's now in the slot of the first observer; i.e. the second handler is skipped and the begin/end symmetry guarantee is violated.

Providing the next handler to the caller is a zero-cost way to avoid any impact in the paths of zend_observe_fcall_begin/end.


I could not think of a both inexpensive and better way to implement this...

The usage of the current API within an observer handler leads to bugs like https://bugs.xdebug.org/view.php?id=2232.
Given two observer handlers, next to each other. The first one is executed. It removes itself. The second observer handler gets moved to the place of the first. The first one returns. The handler in the second slot is fetched, but is now NULL, because the it's now in the slot of the first observer; i.e. the second handler is skipped and the begin/end symmetry guarantee is violated.

Providing the next handler to the caller is a zero-cost way to avoid any impact in the paths of zend_observe_fcall_begin/end.

Signed-off-by: Bob Weinand <bobwei9@hotmail.com>
@iluuu1994
Copy link
Member

Would it be possible to check in _zend_observe_fcall_begin (and other places that loop over the handlers) whether the underlying handler has changed after invoking it, and in that case skipping the ++handler? I realize this may be slower, but not sure how much. This might be abstracted away in a macro that loops over the handlers.

@bwoebi
Copy link
Member Author

bwoebi commented Mar 27, 2024

@iluuu1994 Yes it would be possible. I'm just not sure if the overhead is worth it. Like note that for e.g. xdebug, which observes everything, it is overhead which is added on every single function call.

For the purposes of an observer which only sometimes observes, it's not that bad.

@iluuu1994
Copy link
Member

@bwoebi If this API is uncommon, then this caveat seems acceptable. Either approach seems ok to me.

@bwoebi
Copy link
Member Author

bwoebi commented Mar 27, 2024

This particular API which I'm changing here is uncommon to my knowledge.

@bwoebi bwoebi merged commit a22a872 into php:master Apr 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants