Skip to content

Commit 4796607

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 c6352ce commit 4796607

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
@@ -2192,22 +2192,37 @@ when not defined(js):
21922192
when declared(initGC): initGC()
21932193

21942194
when notJSnotNims:
2195-
proc setControlCHook*(hook: proc () {.noconv.})
2195+
proc setControlCHook*(hook: proc () {.noconv.}) {.raises: [], gcsafe.}
21962196
## Allows you to override the behaviour of your application when CTRL+C
21972197
## is pressed. Only one such hook is supported.
2198-
## Example:
21992198
##
2200-
## ```nim
2199+
## The handler runs inside a C signal handler and comes with similar
2200+
## limitations.
2201+
##
2202+
## Allocating memory and interacting with most system calls, including using
2203+
## `echo`, `string`, `seq`, raising or catching exceptions etc is undefined
2204+
## behavior and will likely lead to application crashes.
2205+
##
2206+
## The OS may call the ctrl-c handler from any thread, including threads
2207+
## that were not created by Nim, such as happens on Windows.
2208+
##
2209+
## ## Example:
2210+
##
2211+
## ```nim
2212+
## var stop: Atomic[bool]
22012213
## proc ctrlc() {.noconv.} =
2202-
## echo "Ctrl+C fired!"
2203-
## # do clean up stuff
2204-
## quit()
2214+
## # Using atomics types is safe!
2215+
## stop.store(true)
22052216
##
22062217
## setControlCHook(ctrlc)
2218+
##
2219+
## while not stop.load():
2220+
## echo "Still running.."
2221+
## sleep(1000)
22072222
## ```
22082223

22092224
when not defined(noSignalHandler) and not defined(useNimRtl):
2210-
proc unsetControlCHook*()
2225+
proc unsetControlCHook*() {.raises: [], gcsafe.}
22112226
## Reverts a call to setControlCHook.
22122227

22132228
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.}
@@ -619,6 +620,18 @@ when not defined(noSignalHandler) and not defined(useNimRtl):
619620
type Sighandler = proc (a: cint) {.noconv, benign.}
620621
# xxx factor with ansi_c.CSighandlerT, posix.Sighandler
621622

623+
# TODO this allocation happens in the main thread while the CTRL-C handler
624+
# runs in any thread - since we're about to quit, hopefully nobody will
625+
# notice.
626+
# 32kb is hopefully enough to not cause further allocations, since
627+
# allocations are not well-defined in a signal handler.
628+
# Unfortunately, `showErrorMessage2` and `rawWriteStackTrace` may still
629+
# perform allocations on their own which will continue to cause crashes.
630+
# Fixing this requires changing the API which in turn requires a larger
631+
# refactor of the stack trace handling mechanism, including its support
632+
# for libbacktrace.
633+
var sigHandlerBuf = newStringOfCap(32*1024)
634+
622635
proc signalHandler(sign: cint) {.exportc: "signalHandler", noconv.} =
623636
template processSignal(s, action: untyped) {.dirty.} =
624637
if s == SIGINT: action("SIGINT: Interrupted by Ctrl-C.\n")
@@ -643,10 +656,9 @@ when not defined(noSignalHandler) and not defined(useNimRtl):
643656
logPendingOps()
644657
when hasSomeStackTrace:
645658
when not usesDestructors: GC_disable()
646-
var buf = newStringOfCap(2000)
647-
rawWriteStackTrace(buf)
648-
processSignal(sign, buf.add) # nice hu? currying a la Nim :-)
649-
showErrorMessage2(buf)
659+
rawWriteStackTrace(sigHandlerBuf)
660+
processSignal(sign, sigHandlerBuf.add) # nice hu? currying a la Nim :-)
661+
showErrorMessage2(sigHandlerBuf)
650662
when not usesDestructors: GC_enable()
651663
else:
652664
var msg: cstring

0 commit comments

Comments
 (0)