fix #17749 ignore SIGPIPE signals, fix nim CI #17748 (#17752)

* fix #17749 SIGPIPE

* fix for windows

changelog.md

@@ -39,6 +39,18 @@
 - In `std/os`, `getHomeDir`, `expandTilde`, `getTempDir`, `getConfigDir` now do not include trailing `DirSep`,
   unless `-d:nimLegacyHomeDir` is specified (for a transition period).
 
+- On POSIX systems, the default signal handlers used for Nim programs (it's
+  used for printing the stacktrace on fatal signals) will now re-raise the
+  signal for the OS default handlers to handle.
+
+  This lets the OS perform its default actions, which might include core
+  dumping (on select signals) and notifying the parent process about the cause
+  of termination.
+
+- On POSIX systems, we now ignore `SIGPIPE` signals, use `-d:nimLegacySigpipeHandler`
+  for previous behavior.
+
+
 ## Standard library additions and changes
 - Added support for parenthesized expressions in `strformat`
 
@@ -219,14 +231,6 @@
   `ValueError` when the real command line is not available. `parseopt` was
   previously excluded from `prelude` for JS, as it could not be imported.
 
-- On POSIX systems, the default signal handlers used for Nim programs (it's
-  used for printing the stacktrace on fatal signals) will now re-raise the
-  signal for the OS default handlers to handle.
-
-  This lets the OS perform its default actions, which might include core
-  dumping (on select signals) and notifying the parent process about the cause
-  of termination.
-
 - Added `system.prepareStrMutation` for better support of low
   level `moveMem`, `copyMem` operations for Orc's copy-on-write string
   implementation.

koch.nim

@@ -542,6 +542,10 @@ proc runCI(cmd: string) =
   # boot without -d:nimHasLibFFI to make sure this still works
   kochExecFold("Boot in release mode", "boot -d:release -d:nimStrictMode")
 
+  when false: # debugging: when you need to run only 1 test in CI, use something like this:
+    execFold("debugging test", "nim r tests/stdlib/tosproc.nim")
+    doAssert false, "debugging only"
+
   ## build nimble early on to enable remainder to depend on it if needed
   kochExecFold("Build Nimble", "nimble")
 

lib/system/excpt.nim

@@ -602,6 +602,9 @@ when defined(cpp) and appType != "lib" and not gotoBasedExceptions and
     quit 1
 
 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.} =
     template processSignal(s, action: untyped) {.dirty.} =
       if s == SIGINT: action("SIGINT: Interrupted by Ctrl-C.\n")
@@ -652,7 +655,11 @@ when not defined(noSignalHandler) and not defined(useNimRtl):
     else:
       quit(1)
 
+  var SIG_IGN {.importc: "SIG_IGN", header: "<signal.h>".}: Sighandler
+
   proc registerSignalHandler() =
+    # xxx `signal` is deprecated and has many caveats, we should use `sigaction` instead, e.g.
+    # https://stackoverflow.com/questions/231912/what-is-the-difference-between-sigaction-and-signal
     c_signal(SIGINT, signalHandler)
     c_signal(SIGSEGV, signalHandler)
     c_signal(SIGABRT, signalHandler)
@@ -661,14 +668,17 @@ when not defined(noSignalHandler) and not defined(useNimRtl):
     when declared(SIGBUS):
       c_signal(SIGBUS, signalHandler)
     when declared(SIGPIPE):
-      c_signal(SIGPIPE, signalHandler)
+      when defined(nimLegacySigpipeHandler):
+        c_signal(SIGPIPE, signalHandler)
+      else:
+        c_signal(SIGPIPE, SIG_IGN)
 
   registerSignalHandler() # call it in initialization section
 
 proc setControlCHook(hook: proc () {.noconv.}) =
   # ugly cast, but should work on all architectures:
-  type SignalHandler = proc (sign: cint) {.noconv, benign.}
-  c_signal(SIGINT, cast[SignalHandler](hook))
+  when declared(Sighandler):
+    c_signal(SIGINT, cast[Sighandler](hook))
 
 when not defined(noSignalHandler) and not defined(useNimRtl):
   proc unsetControlCHook() =

tests/stdlib/tosproc.nim

@@ -206,8 +206,8 @@ else: # main driver
       var line = newStringOfCap(120)
       while true:
         if outp.readLine(line):
-          result[0].string.add(line.string)
-          result[0].string.add("\n")
+          result[0].add(line)
+          result[0].add("\n")
         else:
           result[1] = peekExitCode(p)
           if result[1] != -1: break
@@ -279,3 +279,22 @@ else: # main driver
     when defined(posix):
       doAssert execCmdEx("echo $FO", env = newStringTable({"FO": "B"})) == ("B\n", 0)
       doAssert execCmdEx("echo $PWD", workingDir = "/") == ("/\n", 0)
+
+  block: # bug #17749
+    let output = compileNimProg("-d:case_testfile4", "D20210417T011153")
+    var p = startProcess(output, dir)
+    let inp = p.inputStream
+    var count = 0
+    when defined(windows):
+      # xxx we should make osproc.hsWriteData raise IOError on windows, consistent
+      # with posix; we could also (in addition) make IOError a subclass of OSError.
+      type SIGPIPEError = OSError
+    else:
+      type SIGPIPEError = IOError
+    doAssertRaises(SIGPIPEError):
+      for i in 0..<100000:
+        count.inc
+        inp.writeLine "ok" # was giving SIGPIPE and crashing
+    doAssert count >= 100
+    doAssert waitForExit(p) == QuitFailure
+    close(p) # xxx isn't that missing in other places?

tests/system/tsigexitcode.nim

@@ -11,10 +11,13 @@ proc main() =
     discard posix.raise(signal)
   else:
     # synchronize this list with lib/system/except.nim:registerSignalHandler()
-    let fatalSigs = [SIGINT, SIGSEGV, SIGABRT, SIGFPE, SIGILL, SIGBUS,
-                     SIGPIPE]
-    for s in fatalSigs:
+    let sigs = [SIGINT, SIGSEGV, SIGABRT, SIGFPE, SIGILL, SIGBUS, SIGPIPE]
+    for s in sigs:
       let (_, exitCode) = execCmdEx(quoteShellCommand [getAppFilename(), $s])
-      doAssert exitCode == 128 + s, "mismatched exit code for signal " & $s
+      if s == SIGPIPE:
+        # SIGPIPE should be ignored
+        doAssert exitCode == 0, $(exitCode, s)
+      else:
+        doAssert exitCode == 128+s, $(exitCode, s)
 
 main()