From 070bbe51fb2023e79b36a2acf1d63ed7486a3a96 Mon Sep 17 00:00:00 2001 From: Timothee Cour Date: Sat, 17 Apr 2021 01:21:45 -0700 Subject: [PATCH] fix #17749 SIGPIPE --- changelog.md | 20 ++++++++++++-------- lib/system/excpt.nim | 15 ++++++++++++--- tests/stdlib/tosproc.nim | 17 +++++++++++++++-- tests/system/tsigexitcode.nim | 11 +++++++---- 4 files changed, 46 insertions(+), 17 deletions(-) diff --git a/changelog.md b/changelog.md index cfdcd3c6297da..6e5b3434d2bab 100644 --- a/changelog.md +++ b/changelog.md @@ -37,6 +37,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` @@ -217,14 +229,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. diff --git a/lib/system/excpt.nim b/lib/system/excpt.nim index 06fa4509775ce..0ad96b8ba6dde 100644 --- a/lib/system/excpt.nim +++ b/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: "".}: 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,16 @@ 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)) + c_signal(SIGINT, cast[Sighandler](hook)) when not defined(noSignalHandler) and not defined(useNimRtl): proc unsetControlCHook() = diff --git a/tests/stdlib/tosproc.nim b/tests/stdlib/tosproc.nim index 96cff1468164f..80f806a914192 100644 --- a/tests/stdlib/tosproc.nim +++ b/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,16 @@ 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 + doAssertRaises(IOError): + 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? diff --git a/tests/system/tsigexitcode.nim b/tests/system/tsigexitcode.nim index 6922cb8ebd1e3..249256b40431a 100644 --- a/tests/system/tsigexitcode.nim +++ b/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()