Skip to content

Properly forward the signal to the original handler if TSRM is shutdown#10219

Merged
bwoebi merged 1 commit intophp:PHP-8.2from
bwoebi:fix-fpm-sigquit
Mar 18, 2024
Merged

Properly forward the signal to the original handler if TSRM is shutdown#10219
bwoebi merged 1 commit intophp:PHP-8.2from
bwoebi:fix-fpm-sigquit

Conversation

@bwoebi
Copy link
Member

@bwoebi bwoebi commented Jan 4, 2023

I've occasionally been observing stacktraces like:

#0  raise (sig=3) at ../sysdeps/unix/sysv/linux/raise.c:50
#1  0x0000aaaab305e0f8 in zend_signal_handler (signo=3, siginfo=0xffffe6eafa20, context=0xffffe6eafaa0) at /usr/local/src/php/Zend/zend_signal.c:201
#2  0x0000aaaab305cf34 in zend_signal_handler_defer (signo=3, siginfo=0xffffe6eafa20, context=0xffffe6eafaa0) at /usr/local/src/php/Zend/zend_signal.c:91
#3  <signal handler called>
#4  0x0000ffff96344be8 in sched_yield () at ../sysdeps/unix/syscall-template.S:78
#5  0x0000ffff96f51fdc in ?? () from /usr/lib/aarch64-linux-gnu/libasan.so.5
#6  0x0000ffff96f59b9c in ?? () from /usr/lib/aarch64-linux-gnu/libasan.so.5
#7  0x0000ffff96393ae4 in __GI___dl_iterate_phdr (callback=0xffff96f59b90, data=0xffffe6eb0e20) at dl-iteratephdr.c:75
#8  0x0000ffff96f59ea8 in ?? () from /usr/lib/aarch64-linux-gnu/libasan.so.5
#9  0x0000ffff96f59438 in ?? () from /usr/lib/aarch64-linux-gnu/libasan.so.5
#10 0x0000ffff96f596a4 in ?? () from /usr/lib/aarch64-linux-gnu/libasan.so.5
#11 0x0000ffff962c0440 in __cxa_finalize (d=0xffff96fc05b0) at cxa_finalize.c:83
#12 0x0000ffff96e85bb0 in ?? () from /usr/lib/aarch64-linux-gnu/libasan.so.5
#13 0x0000ffff97cf7e1c in _dl_fini () at dl-fini.c:138
#14 0x0000ffff962bfe38 in __run_exit_handlers (status=0, listp=0xffff963f65c8 <__exit_funcs>, run_list_atexit=run_list_atexit@entry=true, run_dtors=run_dtors@entry=true) at exit.c:108
#15 0x0000ffff962bff9c in __GI_exit (status=<optimized out>) at exit.c:139
#16 0x0000ffff962abda8 in __libc_start_main (main=0xaaaab326f2a0 <main>, argc=2, argv=0xffffe6eb13a8, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=<optimized out>) at ../csu/libc-start.c:342
#17 0x0000aaaab1d05804 in _start ()

(Asan builds incidentally make that particular issue a little bit more command as it has more overhead on process end.)

Coredumps were being created despite SIGQUIT being officially a signal for graceful handling in php-fpm, as signals are in ZTS builds not forwarded to the original handler if TSRM has already been shutdown.

This ensures proper handling of SIGQUIT in ZTS fpm builds outside of active requests.

I'm not sure whether that's truly the correct fix, but it looks sensible to me...

@bwoebi bwoebi changed the base branch from master to PHP-8.1 January 4, 2023 14:29
@cmb69
Copy link
Member

cmb69 commented Jan 4, 2023

See also #10193 (seems that ZTS and signals don't play well together anyway).

@bukka
Copy link
Member

bukka commented Dec 3, 2023

@bwoebi I just spent some time to go through this, all those ZTS signal issues and signal code itself. I think this fix makes sense. From what I see, the Zend Signal code overwrite FPM SIGQUIT handler (sig_soft_quit. in fmp_signal.c). That should be stored global_orig_handlers so it should be used after TSRM shutdown. I assume that it should be fine to do it in general for all signals as I assume that sigaction for not handled actions sets oldact to action containing handler with SIG_DFL. I haven't checked that though so might be worth to double check that. But if that's the case, I think it should be fine.

Otherwise it needs rebase as there is a conflict.

@arnaud-lb could you please also double this if you also think it makes sense.

@bukka bukka mentioned this pull request Dec 3, 2023
Open
@arnaud-lb
Copy link
Member

arnaud-lb commented Dec 9, 2023

This makes sense to me as well 👍

@bukka
Copy link
Member

bukka commented Jan 11, 2024

@bwoebi Do you plan to merge this?

@bwoebi
Properly forward the signal to the original handler if TSRM is shutdown.
1f91fd3 
This ensures proper handling of SIGQUIT in ZTS fpm builds outside of active requests.
@bwoebi bwoebi changed the base branch from PHP-8.1 to PHP-8.2 March 17, 2024 02:12
@bwoebi bwoebi requested a review from iluuu1994 as a code owner March 17, 2024 02:12
@bwoebi
Copy link
Member Author

bwoebi commented Mar 17, 2024

@bukka I missed that - rebased it now. So you're saying the patch is fine as is? Then I'll merge it.

@bukka
Copy link
Member

bukka commented Mar 17, 2024

Yeah I think it's fine as is. I can't really think about any reasonable test for this.

@bwoebi bwoebi merged commit 059bf33 into php:PHP-8.2 Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bukka bukka bukka approved these changes

@iluuu1994 iluuu1994 Awaiting requested review from iluuu1994

Assignees

No one assigned

Labels

Category: Engine SAPI: fpm

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bwoebi @cmb69 @bukka @arnaud-lb