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

Introduce a special FPM event pid callback #10510

Closed
wants to merge 1 commit into from

Conversation

bukka
Copy link
Member

@bukka bukka commented Feb 5, 2023

This is an attempt to fix #10461 - needs OP verification

@bukka bukka changed the base branch from master to PHP-8.1 February 5, 2023 12:07
@@ -480,11 +480,14 @@ void fpm_event_loop(int err) /* {{{ */

void fpm_event_fire(struct fpm_event_s *ev) /* {{{ */
{
if (!ev || !ev->callback) {
if (!ev || !ev->ptr_callback) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ptr_callback or pid_callback should be checked below, after dispatch based on pid.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's union so the same memory is used so if one is NULL, then the other one is NULL as well.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course this is true, but this is not explicitly apparent when reading this code and could be easily forgotten if the structure is modified.

union {
void (*ptr_callback)(struct fpm_event_s *, short, void *);
void (*pid_callback)(struct fpm_event_s *, short, pid_t pid);
};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Propose to document in a comment how the proper field should be selected (pid != 0).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah will add a comment if this is confirmed working :)

@bukka
Copy link
Member Author

bukka commented Apr 15, 2023

Passing pid should not be actually necessary and the problem was more about accessing the event after free. So closing this in favour of #11084 .

@bukka bukka closed this Apr 15, 2023
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.

PHP-FPM segfault due to after free usage of child->ev_std(out|err)
4 participants