Skip to content

Commit 87e447d

Browse files
committed
Reduce chance of crashes in CTRL-C handler, rework handler example
Inside a signal handler, you cannot allocate memory because the signal handler, being implemented with a C [`signal`](https://en.cppreference.com/w/c/program/signal) call, can be called _during_ a memory allocation - when that happens, the CTRL-C handler causes a segfault and/or other inconsistent state. Similarly, the call can happen from a non-nim thread or inside a C library function call etc, most of which do not support reentrancy and therefore cannot be called _from_ a signal handler. The stack trace facility used in the default handler is unfortunately beyond fixing without more significant refactoring since it uses garbage-collected types in its API and implementation, but we can at least allocate the buffer outside of the signal handler itself - hopefully, this should reduce the frequency of crashes, if nothing else. It will still crash from time to time on Windows in particular, but since we're about to quit without cleanup, the loss of functionality is simited (ie the stack trace will not show and a crash dump will happen which the OS will notice). Finally, the example of a ctrl-c handler performs the same mistake of calling `echo` which is not well-defined - replace it with an example that is mostly correct (except maybe for the lack of `volatile` for the `stop` variable).
1 parent f7145dd commit 87e447d

File tree

2 files changed

+38
-11
lines changed

2 files changed

+38
-11
lines changed

lib/system.nim

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2176,22 +2176,37 @@ when not defined(js):
21762176
when declared(initGC): initGC()
21772177

21782178
when notJSnotNims:
2179-
proc setControlCHook*(hook: proc () {.noconv.})
2179+
proc setControlCHook*(hook: proc () {.noconv.}) {.raises: [], gcsafe.}
21802180
## Allows you to override the behaviour of your application when CTRL+C
21812181
## is pressed. Only one such hook is supported.
2182-
## Example:
21832182
##
2184-
## ```nim
2183+
## The handler runs inside a C signal handler and comes with similar
2184+
## limitations.
2185+
##
2186+
## Allocating memory and interacting with most system calls, including using
2187+
## `echo`, `string`, `seq`, raising or catching exceptions etc is undefined
2188+
## behavior and will likely lead to application crashes.
2189+
##
2190+
## The OS may call the ctrl-c handler from any thread, including threads
2191+
## that were not created by Nim, such as happens on Windows.
2192+
##
2193+
## ## Example:
2194+
##
2195+
## ```nim
2196+
## var stop: Atomic[bool]
21852197
## proc ctrlc() {.noconv.} =
2186-
## echo "Ctrl+C fired!"
2187-
## # do clean up stuff
2188-
## quit()
2198+
## # Using atomics types is safe!
2199+
## stop.store(true)
21892200
##
21902201
## setControlCHook(ctrlc)
2202+
##
2203+
## while not stop.load():
2204+
## echo "Still running.."
2205+
## sleep(1000)
21912206
## ```
21922207

21932208
when not defined(noSignalHandler) and not defined(useNimRtl):
2194-
proc unsetControlCHook*()
2209+
proc unsetControlCHook*() {.raises: [], gcsafe.}
21952210
## Reverts a call to setControlCHook.
21962211

21972212
when hostOS != "standalone":

lib/system/excpt.nim

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@ proc showErrorMessage(data: cstring, length: int) {.gcsafe, raises: [].} =
5858
writeToStdErr(data, length)
5959

6060
proc showErrorMessage2(data: string) {.inline.} =
61+
# TODO showErrorMessage will turn it back to a string when a hook is set (!)
6162
showErrorMessage(data.cstring, data.len)
6263

6364
proc chckIndx(i, a, b: int): int {.inline, compilerproc, benign.}
@@ -624,6 +625,18 @@ when not defined(noSignalHandler) and not defined(useNimRtl):
624625
type Sighandler = proc (a: cint) {.noconv, benign.}
625626
# xxx factor with ansi_c.CSighandlerT, posix.Sighandler
626627

628+
# TODO this allocation happens in the main thread while the CTRL-C handler
629+
# runs in any thread - since we're about to quit, hopefully nobody will
630+
# notice.
631+
# 32kb is hopefully enough to not cause further allocations, since
632+
# allocations are not well-defined in a signal handler.
633+
# Unfortunately, `showErrorMessage2` and `rawWriteStackTrace` may still
634+
# perform allocations on their own which will continue to cause crashes.
635+
# Fixing this requires changing the API which in turn requires a larger
636+
# refactor of the stack trace handling mechanism, including its support
637+
# for libbacktrace.
638+
var sigHandlerBuf = newStringOfCap(32*1024)
639+
627640
proc signalHandler(sign: cint) {.exportc: "signalHandler", noconv.} =
628641
template processSignal(s, action: untyped) {.dirty.} =
629642
if s == SIGINT: action("SIGINT: Interrupted by Ctrl-C.\n")
@@ -648,10 +661,9 @@ when not defined(noSignalHandler) and not defined(useNimRtl):
648661
logPendingOps()
649662
when hasSomeStackTrace:
650663
when not usesDestructors: GC_disable()
651-
var buf = newStringOfCap(2000)
652-
rawWriteStackTrace(buf)
653-
processSignal(sign, buf.add) # nice hu? currying a la Nim :-)
654-
showErrorMessage2(buf)
664+
rawWriteStackTrace(sigHandlerBuf)
665+
processSignal(sign, sigHandlerBuf.add) # nice hu? currying a la Nim :-)
666+
showErrorMessage2(sigHandlerBuf)
655667
when not usesDestructors: GC_enable()
656668
else:
657669
var msg: cstring

0 commit comments

Comments
 (0)