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

Request: forward all (well, most) signals to child process #240

Open
brettdh opened this issue Jun 19, 2023 · 4 comments · May be fixed by #242
Open

Request: forward all (well, most) signals to child process #240

brettdh opened this issue Jun 19, 2023 · 4 comments · May be fixed by #242

Comments

@brettdh
Copy link

brettdh commented Jun 19, 2023

I recently upgraded a Pyinstaller-built program to use Python 3.11. This resulted in the binary using a newer version of glibc than the target platform supported, which led me to discover staticx, without which this effort would be rather doomed. So first things first - thank you for building this. 🙂

Back when I first started using Pyinstaller, I noticed that most signals were not being forwarded to the child process. In particular, my program uses a somewhat arbitrary signal number to control the logging level of the child process. A more commonly-used example would be SIGHUP, which is often used to ask a daemon to reload its configuration from disk.

I made a change to the Pyinstaller bootloader back in 2018 whereby it forwards all signals (except SIGCLD/SIGCHLD, because they're special) to the child process. I think something similar would work for staticx's (grammar...? 😅 ) bootloader, but I haven't had a chance to try it out yet.

I might get some time to put together a PR for this, but I wanted to first open this issue so as to:

  1. get your feedback on the idea; and
  2. give you the chance to maybe get to it before I do 😁

Thanks again!

@JonathonReinhart
Copy link
Owner

Ah, you implemented that change to the PyInstaller bootloader after it had already influenced the staticx bootloader (167a585). No wonder it's not there! 😄

I agree, all signals should be forwarded, not just termination signals, with the exception of SIGCHLD as you indicate, and SIGKILL and SIGSTOP, since according to signal(7):

The signals SIGKILL and SIGSTOP cannot be caught, blocked, or ignored.

I also think there's a mistake in pyinstaller/pyinstaller@2ddb80d in the bounds of the for() loop: It should start at 1, not 0. And the upper bound is technically correct, but it is a bit misleading: I would use num_signals = 64, and use i <= num_signals.

I'm experimenting with an implementation now, but I'm seeing some strange differences between signal() and sigaction() on musl vs glibc... stay tuned.

@JonathonReinhart
Copy link
Owner

I've posted a draft PR in #242.

The "strange differences" I was referring to were the fact that SIGRTMIN starts at 35 on musl-libc (32-34 are reserved for pthreads implementation I believe), and thus sigaction() was failing.

I'm still not sure we want to forward all signals 1-31 with SIGCHLD, SIGKILL, and SIGSTOP in a denylist. If you read all of the signals listed on signal(7), I think there are plenty of others that we probably don't want to forward, too. Specifically ones that originate from hardware exceptions like SIGSEGV, SIGILL, SIGBUS, SIGFPE, etc.

I think we might consider instead an allowlist approach, including e.g.

  • SIGABRT (maybe)
  • SIGINT
  • SIGQUIT
  • SIGTERM
  • SIGUSR1
  • SIGUSR2

@brettdh
Copy link
Author

brettdh commented Jun 22, 2023

So long as the allow list is configurable when running staticx (with some reasonable defaults as you listed), that sounds just fine to me. Thanks for looking into it!

@brettdh
Copy link
Author

brettdh commented Jun 22, 2023

In my original case, I was wrapping a legacy application which arbitrarily used signal 42. (A lot has changed since 2018, though, and I could change this to something else now if needed.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants