Fix signal handling #110
Fix signal handling #110
Conversation
This reverts commit bdc1bdb. Neither free() nor exit() are safe to call in a signal handler. Signed-off-by: Ben Hutchings <[email protected]>
Currently we use a blocking netlink socket, which is not compatible with handling signals gracefully. To handle SIGINT properly: - Set the netlink socket to be non-blocking - Block SIGINT and create a signalfd() to receive it - Poll the netlink socket and signal fd in a loop Signed-off-by: Ben Hutchings <[email protected]>
systemd's default stop signal and the kill command's default signal are SIGTERM. Catch this as well as SIGINT. Signed-off-by: Ben Hutchings <[email protected]>
|
SIGINT can occur here only when tlshd is run at the command line, never when it is run as a daemon, as it normally is deployed. I can't find any immediate search results that explain how the parent exiting from a signal handler is in any way risky. I'm not convinced the risk is high enough to include this PR in a 1.2 minor release. But the current signal handler is throw-away code. |
Right. I added SIGTERM handling to provide a clean shutdown in the normal case.
Any Linux/Unix programming reference should explain this.
OK. |
I understand that is what the PR does. But it doesn't tell me why SIGTERM cleanup is necessary. The only purpose for the SIGINT handler was to clean up valgrind results so I can more easily catch memory leaks. There's no hard requirement that I can see for explicit clean up in other cases, since all processes, including the parent, are exiting. My fundamental qualms are about switching to using a polling event loop. I need to think about that. Would atexit() be a better approach for proper cleanup?
Searching again, I see claims that I shouldn't exit from a signal handler, but the suggestion is to use _exit(2) instead of exit(3). "man 7 signal" is referenced in some search results, but a quick browse doesn't find anything on point there either. I don't feel that I understand the risk yet. Even so, I will pull this into a 1.3 dev branch for more study. |
|
Manually pushed to ktls-utils-1.3-dev. |
|
The combined signal and netlink polling mechanism seems straightforward, and passes my testing. I'm re-opening this PR in order to pull the commits into the main branch. Sorry for the noise, I'm still getting used to GitHub's workflows. |
|
Hi Ben - would you be interested in adding a handler for SIGUSR1 (or SIGHUP) to cause tlshd to reload it's configuration? |
That would be good to have. I'm not sure if you're asking me to implement it or offering to do so...? In any case, |
I'm asking if you'd like to implement and contribute a SIGHUP handler for tlshd. |
2 participants
The signal handler added for SIGINT in commit bdc1bdb is unsafe, as it calls (indirectly) free() and exit() which are not signal-safe functions.
This reverts that change and switches to a poll()-based event loop that handles signals without using a signal handler.
It also adds handling of SIGTERM, which is the signal that systemd will send to stop the service.