bug#70144: system* affects signal handlers

bug#70144: system* affects signal handlers

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 544 bytes --]

I've encountered a situation where signal handlers don't seem to
run. With the following program, sending it SIGINT won't trigger the
handler, however if you remove the system* call, then the handler will
run.

  (use-modules (ice-9 threads))

  (call-with-new-thread
   (lambda ()
     ;; Remove the following system* call to fix the handler
     (system* "echo" "foo")))

  (sigaction SIGINT
    (lambda (sig)
      (peek "SIGINT handler")
      (exit 1)))

  (for-each
   (lambda _
     (sleep 1))
   (iota 30))

  (display "normal exit\n")

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 987 bytes --]

bug#70144: system* affects signal handlers

[Mikael Djurfeldt]

[-- Attachment #1: Type: text/plain, Size: 1919 bytes --]

system* temporarily re-binds signal handlers to prevent the child process
from killing the parent. Thus, it is not thread safe with regard to SIGINT
(or SIGQUIT if available). So, your code has a race condition with respect
to the signal handler. This common resource can, in principle, be handled
the usual way by, for example, utilizing a mutex:

(use-modules (ice-9 threads))

(define sigint-mutex (make-mutex))

(define thread
  (call-with-new-thread
   (lambda ()
     (with-mutex sigint-mutex
       (system* "echo" "foo")))))

(with-mutex sigint-mutex
  (sigaction SIGINT
    (lambda (sig)
      (peek "SIGINT handler")
      (exit 1)))

  (for-each
   (lambda _
     (sleep 1))
   (iota 30)))

(join-thread thread)

(display "normal exit\n")

But if this was real code, another way would be to make sure that the code
blocks are run in the order that you wish (which you did not want here
since your very purpose was to provoke the collision of resources).

I'm leaving this bug open. *Should* system* re-bind the signal handlers?
Should it really protect itself from the child? If so, we should probably
document this behaviour in the reference manual.

Best regards,
Mikael

On Tue, Apr 2, 2024 at 4:28 PM Christopher Baines <mail@cbaines.net> wrote:

> I've encountered a situation where signal handlers don't seem to
> run. With the following program, sending it SIGINT won't trigger the
> handler, however if you remove the system* call, then the handler will
> run.
>
>   (use-modules (ice-9 threads))
>
>   (call-with-new-thread
>    (lambda ()
>      ;; Remove the following system* call to fix the handler
>      (system* "echo" "foo")))
>
>   (sigaction SIGINT
>     (lambda (sig)
>       (peek "SIGINT handler")
>       (exit 1)))
>
>   (for-each
>    (lambda _
>      (sleep 1))
>    (iota 30))
>
>   (display "normal exit\n")
>

[-- Attachment #2: Type: text/html, Size: 2540 bytes --]

bug#70144: system* affects signal handlers

[Ludovic Courtès]

Hi,

Mikael Djurfeldt <mikael@djurfeldt.com> skribis:

> system* temporarily re-binds signal handlers to prevent the child process
> from killing the parent. Thus, it is not thread safe with regard to SIGINT
> (or SIGQUIT if available). So, your code has a race condition with respect
> to the signal handler. This common resource can, in principle, be handled
> the usual way by, for example, utilizing a mutex:

[...]

> I'm leaving this bug open. *Should* system* re-bind the signal handlers?
> Should it really protect itself from the child? If so, we should probably
> document this behaviour in the reference manual.

Unless I’m mistaken, we can remove the ‘scm_dynwind_sigaction’ calls
from ‘scm_system_star’: now that we use ‘posix_spawn’, this is all taken
care of.

This can be seen by running:

  strace -o /tmp/log.strace -f guile -c '(system* "/bin/sh" "-c" "echo foo $$")'

… which shows pre-fork signal blocking (this is
‘internal_signal_block_all’ in spawni.c in glibc) followed, in the child
(PID 28592 here), by a long series of ‘sigaction’ calls to reset
handlers to their default behavior:

--8<---------------cut here---------------start------------->8---
28586 rt_sigprocmask(SIG_BLOCK, ~[], [], 8) = 0
28586 clone3({flags=CLONE_VM|CLONE_VFORK, exit_signal=SIGCHLD, stack=0x7f73b39b2000, stack_size=0x9000}, 88 <unfinished ...>
28592 rt_sigprocmask(SIG_BLOCK, NULL, ~[KILL STOP], 8) = 0
28592 rt_sigaction(SIGHUP, NULL, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=0}, 8) = 0
28592 rt_sigaction(SIGHUP, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=SA_RESTORER, sa_restorer=0x7f73b432d2a0}, NULL, 8) = 0
28592 rt_sigaction(SIGINT, NULL, {sa_handler=SIG_IGN, sa_mask=[], sa_flags=SA_RESTORER, sa_restorer=0x7f73b432d2a0}, 8) = 0
28592 rt_sigaction(SIGQUIT, NULL, {sa_handler=SIG_IGN, sa_mask=[], sa_flags=SA_RESTORER, sa_restorer=0x7f73b432d2a0}, 8) = 0
28592 rt_sigaction(SIGILL, NULL, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=0}, 8) = 0
28592 rt_sigaction(SIGILL, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=SA_RESTORER, sa_restorer=0x7f73b432d2a0}, NULL, 8) = 0
28592 rt_sigaction(SIGTRAP, NULL, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=0}, 8) = 0
28592 rt_sigaction(SIGTRAP, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=SA_RESTORER, sa_restorer=0x7f73b432d2a0}, NULL, 8) = 0
--8<---------------cut here---------------end--------------->8---

Josselin, can you confirm?

Thanks,
Ludo’.

bug#70144: system* affects signal handlers

[Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language]

[-- Attachment #1: Type: text/plain, Size: 2066 bytes --]

Hi Ludo,

Ludovic Courtès <ludo@gnu.org> writes:

> Unless I’m mistaken, we can remove the ‘scm_dynwind_sigaction’ calls
> from ‘scm_system_star’: now that we use ‘posix_spawn’, this is all taken
> care of.
>
> This can be seen by running:
>
>   strace -o /tmp/log.strace -f guile -c '(system* "/bin/sh" "-c" "echo foo $$")'
>
> … which shows pre-fork signal blocking (this is
> ‘internal_signal_block_all’ in spawni.c in glibc) followed, in the child
> (PID 28592 here), by a long series of ‘sigaction’ calls to reset
> handlers to their default behavior:
>
> --8<---------------cut here---------------start------------->8---
> 28586 rt_sigprocmask(SIG_BLOCK, ~[], [], 8) = 0
> 28586 clone3({flags=CLONE_VM|CLONE_VFORK, exit_signal=SIGCHLD, stack=0x7f73b39b2000, stack_size=0x9000}, 88 <unfinished ...>
> 28592 rt_sigprocmask(SIG_BLOCK, NULL, ~[KILL STOP], 8) = 0
> 28592 rt_sigaction(SIGHUP, NULL, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=0}, 8) = 0
> 28592 rt_sigaction(SIGHUP, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=SA_RESTORER, sa_restorer=0x7f73b432d2a0}, NULL, 8) = 0
> 28592 rt_sigaction(SIGINT, NULL, {sa_handler=SIG_IGN, sa_mask=[], sa_flags=SA_RESTORER, sa_restorer=0x7f73b432d2a0}, 8) = 0
> 28592 rt_sigaction(SIGQUIT, NULL, {sa_handler=SIG_IGN, sa_mask=[], sa_flags=SA_RESTORER, sa_restorer=0x7f73b432d2a0}, 8) = 0
> 28592 rt_sigaction(SIGILL, NULL, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=0}, 8) = 0
> 28592 rt_sigaction(SIGILL, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=SA_RESTORER, sa_restorer=0x7f73b432d2a0}, NULL, 8) = 0
> 28592 rt_sigaction(SIGTRAP, NULL, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=0}, 8) = 0
> 28592 rt_sigaction(SIGTRAP, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=SA_RESTORER, sa_restorer=0x7f73b432d2a0}, NULL, 8) = 0
> --8<---------------cut here---------------end--------------->8---
>
> Josselin, can you confirm?

Yes, I believe this is all taken care of by our use of posix_spawn
(which was the point in the first place :) ).

Best,
-- 
Josselin Poiret

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 682 bytes --]

bug#70144: system* affects signal handlers

[Ludovic Courtès]

Hi,

Josselin Poiret <dev@jpoiret.xyz> skribis:

> Yes, I believe this is all taken care of by our use of posix_spawn
> (which was the point in the first place :) ).

Yup!  I pushed the fix as 4ae33f76d6b33ea0bedfa36050d44c88d08c2823.

Thanks,
Ludo’.