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

eh

skip signal handler in alloc-counting tests

more alloc stats

raises?

fun with raises

more tests

reenable on windows

we're about to terminate anyway

Author: arnetheduck

lib/pure/cgi.nim

@@ -289,11 +289,14 @@ Content-Type: text/html
 proc writeErrorMessage*(data: string) =
   ## Tries to reset browser state and writes `data` to stdout in
   ## <plaintext> tag.
-  resetForStacktrace()
-  # We use <plaintext> here, instead of escaping, so stacktrace can
-  # be understood by human looking at source.
-  stdout.write("<plaintext>\n")
-  stdout.write(data)
+  try:
+    resetForStacktrace()
+    # We use <plaintext> here, instead of escaping, so stacktrace can
+    # be understood by human looking at source.
+    stdout.write("<plaintext>\n")
+    stdout.write(data)
+  except IOError as exc:
+    discard # Too bad..
 
 proc setStackTraceStdout*() =
   ## Makes Nim output stacktraces to stdout, instead of server log.

lib/system.nim

@@ -2201,22 +2201,37 @@ when not defined(js):
     when declared(initGC): initGC()
 
 when notJSnotNims:
-  proc setControlCHook*(hook: proc () {.noconv.})
+  proc setControlCHook*(hook: proc () {.noconv.}) {.raises: [], gcsafe.}
     ## Allows you to override the behaviour of your application when CTRL+C
     ## is pressed. Only one such hook is supported.
-    ## Example:
     ##
-    ##   ```nim
+    ## The handler runs inside a C signal handler and comes with similar
+    ## limitations.
+    ##
+    ## Allocating memory and interacting with most system calls, including using
+    ## `echo`, `string`, `seq`, raising or catching exceptions etc is undefined
+    ## behavior and will likely lead to application crashes.
+    ##
+    ## The OS may call the ctrl-c handler from any thread, including threads
+    ## that were not created by Nim, such as happens on Windows.
+    ##
+    ## ## Example:
+    ##
+    ## ```nim
+    ##   var stop: Atomic[bool]
     ##   proc ctrlc() {.noconv.} =
-    ##     echo "Ctrl+C fired!"
-    ##     # do clean up stuff
-    ##     quit()
+    ##     # Using atomics types is safe!
+    ##     stop.store(true)
     ##
     ##   setControlCHook(ctrlc)
+    ##
+    ##   while not stop.load():
+    ##     echo "Still running.."
+    ##     sleep(1000)
     ##   ```
 
   when not defined(noSignalHandler) and not defined(useNimRtl):
-    proc unsetControlCHook*()
+    proc unsetControlCHook*() {.raises: [], gcsafe.}
       ## Reverts a call to setControlCHook.
 
   when hostOS != "standalone":

lib/system/ansi_c.nim

@@ -11,7 +11,7 @@
 # and definitions of Ansi C types in Nim syntax
 # All symbols are prefixed with 'c_' to avoid ambiguities
 
-{.push hints:off, stack_trace: off, profiler: off.}
+{.push hints:off, stack_trace: off, profiler: off, raises: [].}
 
 proc c_memchr*(s: pointer, c: cint, n: csize_t): pointer {.
   importc: "memchr", header: "<string.h>".}

lib/system/excpt.nim

@@ -17,7 +17,7 @@ const noStacktraceAvailable = "No stack traceback available\n"
 
 var
   errorMessageWriter*: (proc(msg: string) {.tags: [WriteIOEffect], benign,
-                                            nimcall.})
+                                            nimcall, raises: [].})
     ## Function that will be called
     ## instead of `stdmsg.write` when printing stacktrace.
     ## Unstable API.
@@ -58,6 +58,7 @@ proc showErrorMessage(data: cstring, length: int) {.gcsafe, raises: [].} =
       writeToStdErr(data, length)
 
 proc showErrorMessage2(data: string) {.inline.} =
+  # TODO showErrorMessage will turn it back to a string when a hook is set (!)
   showErrorMessage(data.cstring, data.len)
 
 proc chckIndx(i, a, b: int): int {.inline, compilerproc, benign.}
@@ -619,7 +620,19 @@ when not defined(noSignalHandler) and not defined(useNimRtl):
   type Sighandler = proc (a: cint) {.noconv, benign.}
     # xxx factor with ansi_c.CSighandlerT, posix.Sighandler
 
-  proc signalHandler(sign: cint) {.exportc: "signalHandler", noconv.} =
+  # TODO this allocation happens in the main thread while the CTRL-C handler
+  #      runs in any thread - since we're about to quit, hopefully nobody will
+  #      notice.
+  #      32kb is hopefully enough to not cause further allocations, since
+  #      allocations are not well-defined in a signal handler.
+  #      Unfortunately, `showErrorMessage2` and `rawWriteStackTrace` may still
+  #      perform allocations on their own which will continue to cause crashes.
+  #      Fixing this requires changing the API which in turn requires a larger
+  #      refactor of the stack trace handling mechanism, including its support
+  #      for libbacktrace.
+  var sigHandlerBuf = newStringOfCap(32*1024)
+
+  proc signalHandler(sign: cint) {.exportc: "signalHandler", noconv, raises: [].} =
     template processSignal(s, action: untyped) {.dirty.} =
       if s == SIGINT: action("SIGINT: Interrupted by Ctrl-C.\n")
       elif s == SIGSEGV:
@@ -641,26 +654,35 @@ when not defined(noSignalHandler) and not defined(useNimRtl):
     # print stack trace and quit
     when defined(memtracker):
       logPendingOps()
+    # On windows, it is common that the signal handler is called from a non-Nim
+    # thread and any allocation will (likely) cause a crash.
+    # On other platforms, if memory needs to be allocated and the signal happens
+    # during memory allocation, we'll alos (likely) see a crash and corrupt the
+    # memory allocator - less frequently than on windows but still.
+    # However, since we're about to go down anyway, YOLO.
+
     when hasSomeStackTrace:
       when not usesDestructors: GC_disable()
-      var buf = newStringOfCap(2000)
-      rawWriteStackTrace(buf)
-      processSignal(sign, buf.add) # nice hu? currying a la Nim :-)
-      showErrorMessage2(buf)
+      rawWriteStackTrace(sigHandlerBuf)
+      processSignal(sign, sigHandlerBuf.add) # nice hu? currying a la Nim :-)
+      showErrorMessage2(sigHandlerBuf)
       when not usesDestructors: GC_enable()
     else:
       var msg: cstring
       template asgn(y) =
         msg = y
       processSignal(sign, asgn)
-      # xxx use string for msg instead of cstring, and here use showErrorMessage2(msg)
-      # unless there's a good reason to use cstring in signal handler to avoid
-      # using gc?
+      # showErrorMessage may allocate, which may cause a crash, and calls C
+      # library functions which is undefined behavior, ie it may also crash.
+      # Nevertheless, we sometimes manage to emit the message regardless which
+      # pragmatically makes this attempt "useful enough".
+      # See also https://en.cppreference.com/w/c/program/signal
       showErrorMessage(msg, msg.len)
 
     when defined(posix):
       # reset the signal handler to OS default
-      c_signal(sign, SIG_DFL)
+      {.cast(raises: []).}: # Work around -d:laxEffects bugs
+        discard c_signal(sign, SIG_DFL)
 
       # re-raise the signal, which will arrive once this handler exit.
       # this lets the OS perform actions like core dumping and will
@@ -697,4 +719,5 @@ proc setControlCHook(hook: proc () {.noconv.}) =
 when not defined(noSignalHandler) and not defined(useNimRtl):
   proc unsetControlCHook() =
     # proc to unset a hook set by setControlCHook
-    c_signal(SIGINT, signalHandler)
+    {.gcsafe.}: # Work around -d:laxEffects bugs
+      discard c_signal(SIGINT, signalHandler)

lib/system/memtracker.nim

@@ -35,7 +35,7 @@ type
     count*: int
     disabled: bool
     data*: array[400, LogEntry]
-  TrackLogger* = proc (log: TrackLog) {.nimcall, tags: [], gcsafe.}
+  TrackLogger* = proc (log: TrackLog) {.nimcall, tags: [], gcsafe, raises: [].}
 
 var
   gLog*: TrackLog

tests/arc/tasyncleak.nim

@@ -1,6 +1,6 @@
 discard """
   outputsub: "(allocCount: 4011, deallocCount: 4009)"
-  cmd: "nim c --gc:orc -d:nimAllocStats $file"
+  cmd: "nim c --gc:orc -d:nimAllocStats -d:noSignalHandler $file"
 """
 
 import asyncdispatch

tests/arc/tasyncleak2.nim

@@ -1,6 +1,6 @@
 discard """
   output: "success"
-  cmd: "nim c --gc:orc $file"
+  cmd: "nim c --gc:orc -d:noSignalHandler $file"
 """
 
 # issue #15076

tests/arc/tclosureiter.nim

@@ -1,5 +1,5 @@
 discard """
-  cmd: '''nim c -d:nimAllocStats --gc:arc $file'''
+  cmd: '''nim c -d:nimAllocStats -d:noSignalHandler --gc:arc $file'''
   output: '''(allocCount: 102, deallocCount: 102)'''
 """
 

tests/arc/tdeepcopy.nim

@@ -1,5 +1,5 @@
 discard """
-  cmd: "nim c --gc:arc --deepcopy:on $file"
+  cmd: "nim c --gc:arc -d:noSignalHandler --deepcopy:on $file"
   output: '''13 abc
 13 abc
 13 abc

tests/arc/tdestroy_in_loopcond.nim

@@ -1,6 +1,6 @@
 discard """
   output: '''400 true'''
-  cmd: "nim c --gc:orc $file"
+  cmd: "nim c --gc:orc -d:noSignalHandler $file"
 """
 
 type HeapQueue*[T] = object