Skip to content

CRT: make termination signal handler async signal safe #1839

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.

Sign up for GitHub

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

Open
DeltachangeOG wants to merge 2 commits into
base: main
Choose a base branch
from
+23 −3
Open

CRT: make termination signal handler async signal safe #1839

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c4861c9
CRT: make termination signal handler async signal safe
DeltachangeOG Dec 17, 2025
b7b16b2
Reset signal handling after initial signal handled
DeltachangeOG Dec 17, 2025
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions CRT.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,9 @@ const char* const* CRT_treeStr = CRT_treeStrAscii;

static const Settings* CRT_settings;

static volatile sig_atomic_t CRT_terminateRequested = 0;
static volatile sig_atomic_t CRT_terminateSignal = 0;
Copy link
Contributor

@Explorer09 Explorer09 Dec 17, 2025

Choose a reason for hiding this comment

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

I have a small idea that I'm not sure you would experiment on.

The CRT_terminateSignal variable and CRT_terminateRequested flag may be merged to one variable. Since the signal number would be likely zero if there's no signal received.

#ifdef HAVE_LIBNCURSESW
# if MB_LEN_MAX >= 3 // Minimum required to support UTF-8 BMP subset
char CRT_degreeSign[MB_LEN_MAX * 2] = "\xc2\xb0";
Expand Down Expand Up @@ -957,8 +960,22 @@ int CRT_scrollWheelVAmount = 10;

ColorScheme CRT_colorScheme = COLORSCHEME_DEFAULT;

ATTR_NORETURN

static void CRT_handleSIGTERM(int sgn) {
CRT_terminateSignal = (sig_atomic_t)sgn;
CRT_terminateRequested = 1;
Comment on lines +965 to +966
Copy link
Member

@BenBE BenBE Dec 17, 2025

Choose a reason for hiding this comment

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

This is only thread/async-safe if you guarantee write order with a memory barrier.

Does not handle re-entrance …
}
Comment on lines 964 to +967
Copy link
Member

@BenBE BenBE Dec 17, 2025

Choose a reason for hiding this comment

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

This causes things like SIGSEGV to get ignored silently with code returning in an undefined state.

That these handlers were ATTR_NORETURN was intentional, as for many situations like SIGSEGV, SIGFPE, and some others you can't really guarantee the state of the program.

Furthermore moving the actual signal execution away from the signal handler causes the reported backtrace to become useless, as it now will report the (known) call path to the central user code handler …
Copy link
Author

@DeltachangeOG DeltachangeOG Dec 18, 2025

Choose a reason for hiding this comment

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

Understood. I’ll rework this to keep the existing handler-driven structure and instead remove/rewrite the non–async-signal-safe calls used by the handler(s), so termination and reporting still happen in the signal context. You can close this PR and I'll rework and resubmit.
Copy link
Author

@DeltachangeOG DeltachangeOG Dec 18, 2025

Choose a reason for hiding this comment

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

@BenBE Quick question for the rework: CRT_done() is curses-based (endwin(), refresh(), etc.), so it can’t be made async-signal-safe in a meaningful way. For SIGINT/SIGTERM/SIGQUIT, would you prefer (a) dropping CRT_done() from the handler and accepting that the terminal may be left in a bad state, or (b) keeping the current cleanup despite not being signal-safe? I’ll follow the project’s preferred behavior.
Copy link
Member

@BenBE BenBE Dec 18, 2025

Choose a reason for hiding this comment

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

While we can't fully avoid the signal nesting, and thus catching a signal inside our handler, we may be able to apply to mitigations:

  1. Mask signals where possible and handle reentrancy as early as possible, to avoid re-running the later handler part upon receiving secondary signals. I.e. disarm signal handling as much as possible and upon detecting a second entry bail out.

  2. Use semaphores to ensure once-only execution of the handler part of routine such that even if there was a nested call of the signal handler, we wouldn't reach any of the non-reentrant code paths. E.g. knowing CRT_done is not reentrant-safe, any exclusion logic would need to be placed before it, that any nested call cannout reach it again (and instead hard-exits).

So to answer your question: (c) leaving the console in a somewhat bad state is acceptable, although it should be avoided for common cases where reasonably possible.

static void CRT_tryTermExit(void) {

if (!CRT_terminateRequested) {
return;
}

int sgn = (int)CRT_terminateSignal;
signal(SIGINT, SIG_DFL);
signal(SIGTERM, SIG_DFL);
signal(SIGQUIT, SIG_DFL);
Comment on lines +976 to +978
Copy link
Member

@BenBE BenBE Dec 17, 2025

Choose a reason for hiding this comment

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

Those LOC are the only ones in this PR that got me at least partially convinced.
CRT_done();

if (!CRT_settings->changed)
Expand All @@ -970,12 +987,13 @@ static void CRT_handleSIGTERM(int sgn) {

char err_buf[512];
snprintf(err_buf, sizeof(err_buf),
"A signal %d (%s) was received, exiting without persisting settings to htoprc.\n",
sgn, signal_str);
"A signal %d (%s) was received, exiting without persisting settings to htoprc.\n",
sgn, signal_str);
Comment on lines -973 to +991
Copy link
Member

@BenBE BenBE Dec 17, 2025

Choose a reason for hiding this comment

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

Why change the indentation?
full_write_str(STDERR_FILENO, err_buf);
_exit(0);
}


Copy link
Member

@BenBE BenBE Dec 17, 2025

Choose a reason for hiding this comment

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

Unnecessary whitespace change
#ifndef NDEBUG

static int stderrRedirectNewFd = -1;
Expand Down Expand Up @@ -1311,9 +1329,11 @@ void CRT_fatalError(const char* note) {
}

int CRT_readKey(void) {

Copy link
Member

@BenBE BenBE Dec 17, 2025

Choose a reason for hiding this comment

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

Unnecessary whitespace change
nocbreak();
cbreak();
nodelay(stdscr, FALSE);
CRT_tryTermExit();
Copy link
Member

@BenBE BenBE Dec 17, 2025

Choose a reason for hiding this comment

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

See note above for why this breaks more than it repairs.
int ret = getch();
halfdelay(CRT_settings->delay);
return ret;
Expand Down