From 9af85fb69f26dcae5fba7881a597d78f6bc7ffc8 Mon Sep 17 00:00:00 2001 From: Timothee Cour Date: Sun, 13 Jan 2019 00:00:39 -0800 Subject: [PATCH] fixes #10273 execShellCmd now returns nonzero when child killed with signal + other fixes (#10274) * s/exitStatus(...)/exitStatusLikeShell(...)/ * fix #10273 execShellCmd now returns nonzero when child exits with signal * test case for #10249 and explanation for the bug * fix test failure * add tests/nim.cfg --- .gitignore | 2 +- lib/pure/os.nim | 21 +++-- lib/pure/osproc.nim | 21 ++--- testament/lib/stdtest/specialpaths.nim | 31 +++++++ tests/nim.cfg | 1 + tests/stdlib/t10231.nim | 2 + tests/stdlib/tos.nim | 2 + tests/stdlib/tosproc.nim | 109 +++++++++++++++++++++---- 8 files changed, 148 insertions(+), 41 deletions(-) create mode 100644 testament/lib/stdtest/specialpaths.nim create mode 100644 tests/nim.cfg diff --git a/.gitignore b/.gitignore index 132cd7f60c..2bf0bf30c3 100644 --- a/.gitignore +++ b/.gitignore @@ -32,7 +32,7 @@ doc/*.html doc/*.pdf doc/*.idx /web/upload -build/* +/build/* bin/* # iOS specific wildcards. diff --git a/lib/pure/os.nim b/lib/pure/os.nim index 3a959d4e2c..32fa2885ea 100644 --- a/lib/pure/os.nim +++ b/lib/pure/os.nim @@ -1287,6 +1287,17 @@ proc moveFile*(source, dest: string) {.rtl, extern: "nos$1", discard tryRemoveFile(dest) raise +proc exitStatusLikeShell*(status: cint): cint = + ## converts exit code from `c_system` into a shell exit code + when defined(posix) and not weirdTarget: + if WIFSIGNALED(status): + # like the shell! + 128 + WTERMSIG(status) + else: + WEXITSTATUS(status) + else: + status + proc execShellCmd*(command: string): int {.rtl, extern: "nos$1", tags: [ExecIOEffect], noNimScript.} = ## Executes a `shell command`:idx:. @@ -1295,14 +1306,8 @@ proc execShellCmd*(command: string): int {.rtl, extern: "nos$1", ## line arguments given to program. The proc returns the error code ## of the shell when it has finished. The proc does not return until ## the process has finished. To execute a program without having a - ## shell involved, use the `execProcess` proc of the `osproc` - ## module. - when defined(macosx): - result = c_system(command) shr 8 - elif defined(posix): - result = WEXITSTATUS(c_system(command)) - else: - result = c_system(command) + ## shell involved, use `osproc.execProcess`. + result = exitStatusLikeShell(c_system(command)) # Templates for filtering directories and files when defined(windows) and not weirdTarget: diff --git a/lib/pure/osproc.nim b/lib/pure/osproc.nim index 72581f47cc..dfe75d9982 100644 --- a/lib/pure/osproc.nim +++ b/lib/pure/osproc.nim @@ -724,13 +724,6 @@ elif not defined(useNimRtl): proc isExitStatus(status: cint): bool = WIFEXITED(status) or WIFSIGNALED(status) - proc exitStatus(status: cint): cint = - if WIFSIGNALED(status): - # like the shell! - 128 + WTERMSIG(status) - else: - WEXITSTATUS(status) - proc envToCStringArray(t: StringTableRef): cstringArray = result = cast[cstringArray](alloc0((t.len + 1) * sizeof(cstring))) var i = 0 @@ -1054,7 +1047,7 @@ elif not defined(useNimRtl): proc waitForExit(p: Process, timeout: int = -1): int = if p.exitFlag: - return exitStatus(p.exitStatus) + return exitStatusLikeShell(p.exitStatus) if timeout == -1: var status: cint = 1 @@ -1109,7 +1102,7 @@ elif not defined(useNimRtl): finally: discard posix.close(kqFD) - result = exitStatus(p.exitStatus) + result = exitStatusLikeShell(p.exitStatus) else: import times @@ -1142,7 +1135,7 @@ elif not defined(useNimRtl): s.tv_nsec = b.tv_nsec if p.exitFlag: - return exitStatus(p.exitStatus) + return exitStatusLikeShell(p.exitStatus) if timeout == -1: var status: cint = 1 @@ -1220,20 +1213,20 @@ elif not defined(useNimRtl): if sigprocmask(SIG_UNBLOCK, nmask, omask) == -1: raiseOSError(osLastError()) - result = exitStatus(p.exitStatus) + result = exitStatusLikeShell(p.exitStatus) proc peekExitCode(p: Process): int = var status = cint(0) result = -1 if p.exitFlag: - return exitStatus(p.exitStatus) + return exitStatusLikeShell(p.exitStatus) var ret = waitpid(p.id, status, WNOHANG) if ret > 0: if isExitStatus(status): p.exitFlag = true p.exitStatus = status - result = exitStatus(status) + result = exitStatusLikeShell(status) proc createStream(stream: var Stream, handle: var FileHandle, fileMode: FileMode) = @@ -1265,7 +1258,7 @@ elif not defined(useNimRtl): proc execCmd(command: string): int = when defined(linux): let tmp = csystem(command) - result = if tmp == -1: tmp else: exitStatus(tmp) + result = if tmp == -1: tmp else: exitStatusLikeShell(tmp) else: result = csystem(command) diff --git a/testament/lib/stdtest/specialpaths.nim b/testament/lib/stdtest/specialpaths.nim new file mode 100644 index 0000000000..3c8b2338cd --- /dev/null +++ b/testament/lib/stdtest/specialpaths.nim @@ -0,0 +1,31 @@ +#[ +todo: move findNimStdLibCompileTime, findNimStdLib here +]# + +import os + +# Note: all the const paths defined here are known at compile time and valid +# so long Nim repo isn't relocated after compilation. +# This means the binaries they produce will embed hardcoded paths, which +# isn't appropriate for some applications that need to be relocatable. + +const sourcePath = currentSourcePath() + # robust way to derive other paths here + # We don't depend on PATH so this is robust to having multiple nim + # binaries + +const nimRootDir* = sourcePath.parentDir.parentDir.parentDir.parentDir + ## root of Nim repo + +const stdlibDir* = nimRootDir / "lib" + # todo: make nimeval.findNimStdLibCompileTime use this + +const systemPath* = stdlibDir / "system.nim" + +const buildDir* = nimRootDir / "build" + ## refs #10268: all testament generated files should go here to avoid + ## polluting .gitignore + +static: + # sanity check + doAssert fileExists(systemPath) diff --git a/tests/nim.cfg b/tests/nim.cfg new file mode 100644 index 0000000000..577baaacda --- /dev/null +++ b/tests/nim.cfg @@ -0,0 +1 @@ +--path:"../testament/lib" # so we can `import stdtest/foo` in this dir diff --git a/tests/stdlib/t10231.nim b/tests/stdlib/t10231.nim index 5d1101aa42..2bb64b475c 100644 --- a/tests/stdlib/t10231.nim +++ b/tests/stdlib/t10231.nim @@ -6,6 +6,8 @@ discard """ import os +# consider moving this inside tosproc (taking care that it's for cpp mode) + if paramCount() == 0: # main process doAssert execShellCmd(getAppFilename().quoteShell & " test") == 1 diff --git a/tests/stdlib/tos.nim b/tests/stdlib/tos.nim index a7cf5d5b6f..e4e14d5a14 100644 --- a/tests/stdlib/tos.nim +++ b/tests/stdlib/tos.nim @@ -263,3 +263,5 @@ block splitFile: doAssert splitFile("abc/.") == ("abc", ".", "") doAssert splitFile("..") == ("", "..", "") doAssert splitFile("a/..") == ("a", "..", "") + +# execShellCmd is tested in tosproc diff --git a/tests/stdlib/tosproc.nim b/tests/stdlib/tosproc.nim index 9d57d45742..7a7c1836f9 100644 --- a/tests/stdlib/tosproc.nim +++ b/tests/stdlib/tosproc.nim @@ -1,23 +1,96 @@ -discard """ - output: "" -""" # test the osproc module -import os, osproc +import stdtest/specialpaths +import "../.." / compiler/unittest_light -block execProcessTest: - let dir = parentDir(currentSourcePath()) - let (outp, err) = execCmdEx("nim c " & quoteShell(dir / "osproctest.nim")) - doAssert err == 0 - let exePath = dir / addFileExt("osproctest", ExeExt) - let outStr1 = execProcess(exePath, workingDir=dir, args=["foo", "b A r"], options={}) - doAssert outStr1 == dir & "\nfoo\nb A r\n" +when defined(case_testfile): # compiled test file for child process + from posix import exitnow + proc c_exit2(code: c_int): void {.importc: "_exit", header: "".} + import os + var a = 0 + proc fun(b = 0) = + a.inc + if a mod 10000000 == 0: # prevents optimizing it away + echo a + fun(b+1) - const testDir = "t e st" - createDir(testDir) - doAssert dirExists(testDir) - let outStr2 = execProcess(exePath, workingDir=testDir, args=["x yz"], options={}) - doAssert outStr2 == absolutePath(testDir) & "\nx yz\n" + proc main() = + let args = commandLineParams() + echo (msg: "child binary", pid: getCurrentProcessId()) + let arg = args[0] + echo (arg: arg) + case arg + of "exit_0": + if true: quit(0) + of "exitnow_139": + if true: exitnow(139) + of "c_exit2_139": + if true: c_exit2(139) + of "quit_139": + # `exitStatusLikeShell` doesn't distinguish between a process that + # exit(139) and a process that gets killed with `SIGSEGV` because + # 139 = 11 + 128 = SIGSEGV + 128. + # However, as #10249 shows, this leads to bad debugging experience + # when a child process dies with SIGSEGV, leaving no trace of why it + # failed. The shell (and lldb debugger) solves that by inserting a + # helpful msg: `segmentation fault` when it detects a signal killed + # the child. + # todo: expose an API that will show more diagnostic, returing + # (exitCode, signal) instead of just `shellExitCode`. + if true: quit(139) + of "exit_recursion": # stack overflow by infinite recursion + fun() + echo a + of "exit_array": # bad array access + echo args[1] + main() - removeDir(testDir) - removeFile(exePath) +else: + + import os, osproc, strutils, posix + + block execShellCmdTest: + ## first, compile child program + const nim = getCurrentCompilerExe() + const sourcePath = currentSourcePath() + let output = buildDir / "D20190111T024543".addFileExt(ExeExt) + let cmd = "$# c -o:$# -d:release -d:case_testfile $#" % [nim, output, + sourcePath] + # we're testing `execShellCmd` so don't rely on it to compile test file + # note: this should be exported in posix.nim + proc c_system(cmd: cstring): cint {.importc: "system", + header: "".} + assertEquals c_system(cmd), 0 + + ## use it + template runTest(arg: string, expected: int) = + echo (arg2: arg, expected2: expected) + assertEquals execShellCmd(output & " " & arg), expected + + runTest("exit_0", 0) + runTest("exitnow_139", 139) + runTest("c_exit2_139", 139) + runTest("quit_139", 139) + runTest("exit_array", 1) + when defined(posix): # on windows, -1073741571 + runTest("exit_recursion", SIGSEGV.int + 128) # bug #10273: was returning 0 + assertEquals exitStatusLikeShell(SIGSEGV), SIGSEGV + 128.cint + + block execProcessTest: + let dir = parentDir(currentSourcePath()) + let (outp, err) = execCmdEx("nim c " & quoteShell(dir / "osproctest.nim")) + doAssert err == 0 + let exePath = dir / addFileExt("osproctest", ExeExt) + let outStr1 = execProcess(exePath, workingDir = dir, args = ["foo", + "b A r"], options = {}) + doAssert outStr1 == dir & "\nfoo\nb A r\n" + + const testDir = "t e st" + createDir(testDir) + doAssert dirExists(testDir) + let outStr2 = execProcess(exePath, workingDir = testDir, args = ["x yz"], + options = {}) + doAssert outStr2 == absolutePath(testDir) & "\nx yz\n" + + removeDir(testDir) + removeFile(exePath)