Fix osproc so that it doesn't close pipe/process/thread handles twice (#16385) [backport]

* Add error check to closeHandle and fix closing handle twice in osproc

* Fix compile error on Linux
This commit is contained in:
Tomohiro 2020-12-18 18:17:19 +09:00 • committed by GitHub
commit dcdbae798c
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23

View file

@ -414,6 +414,8 @@ proc execProcesses*(cmds: openArray[string],
raiseOSError(err) raiseOSError(err)
if rexit >= 0: if rexit >= 0:
when defined(windows):
let processHandle = q[rexit].fProcessHandle
result = max(result, abs(q[rexit].peekExitCode())) result = max(result, abs(q[rexit].peekExitCode()))
if afterRunEvent != nil: afterRunEvent(idxs[rexit], q[rexit]) if afterRunEvent != nil: afterRunEvent(idxs[rexit], q[rexit])
close(q[rexit]) close(q[rexit])
@ -428,7 +430,7 @@ proc execProcesses*(cmds: openArray[string],
else: else:
when defined(windows): when defined(windows):
for k in 0..wcount - 1: for k in 0..wcount - 1:
if w[k] == q[rexit].fProcessHandle: if w[k] == processHandle:
w[k] = w[wcount - 1] w[k] = w[wcount - 1]
w[wcount - 1] = 0 w[wcount - 1] = 0
dec(wcount) dec(wcount)
@ -525,10 +527,17 @@ when defined(Windows) and not defined(useNimRtl):
handle: Handle handle: Handle
atTheEnd: bool atTheEnd: bool
proc closeHandleCheck(handle: Handle) {.inline.} =
if handle.closeHandle() == 0:
raiseOSError(osLastError())
proc fileClose[T: Handle | FileHandle](h: var T) {.inline.} =
if h > 4:
closeHandleCheck(h)
h = INVALID_HANDLE_VALUE.T
proc hsClose(s: Stream) = proc hsClose(s: Stream) =
# xxx here + elsewhere: check instead of discard; ignoring errors leads to FileHandleStream(s).handle.fileClose()
# hard to track bugs
discard FileHandleStream(s).handle.closeHandle
proc hsAtEnd(s: Stream): bool = return FileHandleStream(s).atTheEnd proc hsAtEnd(s: Stream): bool = return FileHandleStream(s).atTheEnd
@ -633,8 +642,8 @@ when defined(Windows) and not defined(useNimRtl):
stdin = myDup(pipeIn, 0) stdin = myDup(pipeIn, 0)
stdout = myDup(pipeOut, 0) stdout = myDup(pipeOut, 0)
discard closeHandle(pipeIn) closeHandleCheck(pipeIn)
discard closeHandle(pipeOut) closeHandleCheck(pipeOut)
stderr = stdout stderr = stdout
proc createPipeHandles(rdHandle, wrHandle: var Handle) = proc createPipeHandles(rdHandle, wrHandle: var Handle) =
@ -645,9 +654,6 @@ when defined(Windows) and not defined(useNimRtl):
if createPipe(rdHandle, wrHandle, sa, 0) == 0'i32: if createPipe(rdHandle, wrHandle, sa, 0) == 0'i32:
raiseOSError(osLastError()) raiseOSError(osLastError())
proc fileClose(h: Handle) {.inline.} =
if h > 4: discard closeHandle(h)
proc startProcess(command: string, workingDir: string = "", proc startProcess(command: string, workingDir: string = "",
args: openArray[string] = [], env: StringTableRef = nil, args: openArray[string] = [], env: StringTableRef = nil,
options: set[ProcessOption] = {poStdErrToStdOut}): options: set[ProcessOption] = {poStdErrToStdOut}):
@ -742,13 +748,31 @@ when defined(Windows) and not defined(useNimRtl):
result.id = procInfo.dwProcessId result.id = procInfo.dwProcessId
result.exitFlag = false result.exitFlag = false
proc closeThreadAndProcessHandle(p: Process) =
if p.fThreadHandle != 0:
closeHandleCheck(p.fThreadHandle)
p.fThreadHandle = 0
if p.fProcessHandle != 0:
closeHandleCheck(p.fProcessHandle)
p.fProcessHandle = 0
proc close(p: Process) = proc close(p: Process) =
if poParentStreams notin p.options: if poParentStreams notin p.options:
discard closeHandle(p.inHandle) if p.inStream == nil:
discard closeHandle(p.outHandle) p.inHandle.fileClose()
discard closeHandle(p.errHandle) else:
discard closeHandle(p.fThreadHandle) # p.inHandle can be already closed via inputStream.
discard closeHandle(p.fProcessHandle) p.inStream.close
# You may NOT close outputStream and errorStream.
assert p.outStream == nil or FileHandleStream(p.outStream).handle != INVALID_HANDLE_VALUE
assert p.errStream == nil or FileHandleStream(p.errStream).handle != INVALID_HANDLE_VALUE
if p.outHandle != p.errHandle:
p.errHandle.fileClose()
p.outHandle.fileClose()
p.closeThreadAndProcessHandle()
proc suspend(p: Process) = proc suspend(p: Process) =
discard suspendThread(p.fThreadHandle) discard suspendThread(p.fThreadHandle)
@ -782,8 +806,7 @@ when defined(Windows) and not defined(useNimRtl):
if status != STILL_ACTIVE: if status != STILL_ACTIVE:
p.exitFlag = true p.exitFlag = true
p.exitStatus = status p.exitStatus = status
discard closeHandle(p.fThreadHandle) p.closeThreadAndProcessHandle()
discard closeHandle(p.fProcessHandle)
result = status result = status
else: else:
result = -1 result = -1
@ -799,8 +822,7 @@ when defined(Windows) and not defined(useNimRtl):
discard getExitCodeProcess(p.fProcessHandle, status) discard getExitCodeProcess(p.fProcessHandle, status)
p.exitFlag = true p.exitFlag = true
p.exitStatus = status p.exitStatus = status
discard closeHandle(p.fThreadHandle) p.closeThreadAndProcessHandle()
discard closeHandle(p.fProcessHandle)
result = status result = status
proc inputStream(p: Process): Stream = proc inputStream(p: Process): Stream =