CRT: make termination signal handler async signal safe #1839

DeltachangeOG · 2025-12-17T04:19:26Z

Move SIGINT/SIGTERM/SIGQUIT handling out of the signal handler to make it
async-signal-safe.

The signal handler now only records the signal via volatile sig_atomic_t flags.
New function for termination logic (CRT_done, warning formatting/output, and _exit(0)) built, and is
performed at a safe point before blocking on getch().

Behavior preserved:

unchanged settings exit silently
changed settings emit a warning and exit without persisting htoprc

Built and tested with gcc -fanalyzer; no analyzer unsafe-call warnings remain.

Fixes #1563

Explorer09 · 2025-12-17T07:00:20Z

CRT.c

+ if (!CRT_terminateRequested) {
+ return;
+}
+ CRT_terminateRequested = 0;


What if a signal happens between this line and the if (!CRT_terminateRequested) conditional?

Since the !CRT_terminateRequested returns, the code loop underneath won't execute leaving the flag set for the next pull of the TryExit. Preserving signal state. The flag reset was just to ensure that flag set == needs to be handled, flag reset == is handled. I am happy to remove it for clarity as the function currently always _exit(0), meaning flag cleanup is just defensive coding for other use cases.

The test and set is not atomic when you wrote that way, so it can cause a termination request to miss.

I think what you need to do is reset signals' handling at this point so that if a signal happens during printing of an signal error message (e.g. an snprintf operation below can throw a signal), exit abnormally instead of finish printing the message.

Restored default handling for SIGINT/SIGTERM/SIGQUIT after entering the termination path, ensuring a second signal causes immediate termination.

@DeltachangeOG
The signals I'm concerning are SIGSEGV, SIGILL, SIGFPE and such, not signals triggered by users.

It's fine to ignore or leave them unhandled for user-triggered signals at this point. But signals that indicate an internal program error can cause the handler to go to an infinite loop or a stack overflow. This is the worst and you don't want that to happen.

Understood — this change only affects user-triggered termination signals (SIGINT/SIGTERM/SIGQUIT).

Fault signals such as SIGSEGV/SIGILL/SIGFPE continue to use the existing CRT_handleSIGSEGV path installed via sigaction with SA_RESETHAND, so they won’t re-enter or loop during termination handling. Those code paths are unchanged.

Fault signals such as SIGSEGV/SIGILL/SIGFPE continue to use the existing CRT_handleSIGSEGV path installed via sigaction with SA_RESETHAND, so they won’t re-enter or loop during termination handling. Those code paths are unchanged.

Um. I don't think the SA_RESETHAND flag would do what you want.

According to the man pages, SA_RESETHAND resets the handler of that one signal that causes the call of the signal handler before the entry of the signal handler. It doesn't solve a situation like, (1) a SIGFPE is raised (for some kind of arithmetic error), (2) the CRT_handleSIGTERM set the global flag so that CRT_readKey will stop waiting for user input, and then (3) during the printing of the SIGFPE error, a SIGSEGV is raised.

The SIGSEGV in this instance is effectively ignored, but the program is unsafe to continue. What I am expecting is an immediate program termination in this situation.

Explorer09 · 2025-12-17T07:02:06Z

CRT.c

 CRT_done();

- if (!CRT_settings->changed)
+ if (!CRT_settings->changed) {


Unnecessary style change.

fixed, waiting for feedback on the flag cleanup above and will commit fixes

Explorer09 · 2025-12-17T11:27:24Z

CRT.c

 static const Settings* CRT_settings;

+static volatile sig_atomic_t CRT_terminateRequested = 0;
+static volatile sig_atomic_t CRT_terminateSignal = 0;


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.

BenBE

Let's leave the style guide issues in this PR aside for a moment, as there's some technical problem introduced by this change. The existing handler is intentionally written to terminate hard, as certain signals are thrown in situations where not handling them causes UB when returning to their call site.

Thus a proper fix to the original issue must keep the overall code structure in place and refactor all the used functions to avoid non-reentrant function calls. With the current implementation this is lready violated by the signal handler itself, as you could loose signals if there's e.g. a segmentation fault (improperly handled, cf. above) followed by some interrupt signl like SIGINT or SIGTERM

Also, by placing the actual signal handling away from the signal handler you loose the proper stack context, which is required by the existing error reporting mechanism. With the PR as-is the reported stack-trace becomes essentially useless for diagnosing the issue that cause a segmentation fault or other program crashes.

BenBE · 2025-12-17T19:57:23Z

CRT.c

 static void CRT_handleSIGTERM(int sgn) {
+ CRT_terminateSignal = (sig_atomic_t)sgn;
+ CRT_terminateRequested = 1;
+}


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 …

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.

@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.

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:

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.

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.

BenBE · 2025-12-17T19:58:21Z

CRT.c

-  "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);


Why change the indentation?

BenBE · 2025-12-17T19:58:45Z

CRT.c

 _exit(0);
 }

+


Unnecessary whitespace change

BenBE · 2025-12-17T19:58:51Z

CRT.c

 }

 int CRT_readKey(void) {
+


Unnecessary whitespace change

BenBE · 2025-12-17T19:59:15Z

CRT.c

 nocbreak();
 cbreak();
 nodelay(stdscr, FALSE);
+ CRT_tryTermExit();


See note above for why this breaks more than it repairs.

BenBE · 2025-12-17T20:01:21Z

CRT.c

+ CRT_terminateSignal = (sig_atomic_t)sgn;
+ CRT_terminateRequested = 1;


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

Does not handle re-entrance …

BenBE · 2025-12-17T20:02:34Z

CRT.c

+ signal(SIGINT, SIG_DFL);
+ signal(SIGTERM, SIG_DFL);
+ signal(SIGQUIT, SIG_DFL);


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

CRT: make termination signal handler async signal safe

c4861c9

Explorer09 reviewed Dec 17, 2025

View reviewed changes

Reset signal handling after initial signal handled

b7b16b2

Explorer09 reviewed Dec 17, 2025

View reviewed changes

BenBE mentioned this pull request Dec 17, 2025

CWE-479 in CRT.c #1563

Open

BenBE added enhancement Extension or improvement to existing feature code quality ♻️ Code quality enhancement labels Dec 17, 2025

BenBE requested changes Dec 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

CRT: make termination signal handler async signal safe #1839

CRT: make termination signal handler async signal safe #1839

DeltachangeOG commented Dec 17, 2025 •

edited

Loading

Explorer09 Dec 17, 2025

DeltachangeOG Dec 17, 2025 •

edited

Loading

Explorer09 Dec 17, 2025 •

edited

Loading

DeltachangeOG Dec 17, 2025

Explorer09 Dec 17, 2025 •

edited

Loading

DeltachangeOG Dec 17, 2025

Explorer09 Dec 17, 2025

Explorer09 Dec 17, 2025

DeltachangeOG Dec 17, 2025

Explorer09 Dec 17, 2025

BenBE left a comment

BenBE Dec 17, 2025

DeltachangeOG Dec 18, 2025

DeltachangeOG Dec 18, 2025

BenBE Dec 18, 2025

BenBE Dec 17, 2025

BenBE Dec 17, 2025

BenBE Dec 17, 2025

BenBE Dec 17, 2025

BenBE Dec 17, 2025

BenBE Dec 17, 2025

Labels

3 participants

		CRT_terminateSignal = (sig_atomic_t)sgn;
		CRT_terminateRequested = 1;

Uh oh!

CRT: make termination signal handler async signal safe #1839

Are you sure you want to change the base?

CRT: make termination signal handler async signal safe #1839

Conversation

DeltachangeOG commented Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

DeltachangeOG Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Explorer09 Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Explorer09 Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenBE left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Labels

3 participants

DeltachangeOG commented Dec 17, 2025 •

edited

Loading

DeltachangeOG Dec 17, 2025 •

edited

Loading

Explorer09 Dec 17, 2025 •

edited

Loading

Explorer09 Dec 17, 2025 •

edited

Loading