From 931a70b123743b78d45c44d62300eb1694686a82 Mon Sep 17 00:00:00 2001
From: Timothee Cour <timothee.cour2@gmail.com>
Date: Mon, 11 May 2020 02:14:21 -0700
Subject: [PATCH] fix a critical bug in windows.osproc leading to resource
 leaks and blocking IO [backport] (#14296)

(cherry picked from commit d11cb9d49596957e9fa097110cf19e9caf085592)
---
 changelog.md             |  3 +++
 lib/pure/osproc.nim      |  6 +++++-
 lib/windows/winlean.nim  |  2 ++
 tests/stdlib/tosproc.nim | 26 ++++++++++++++++++++++++++
 4 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/changelog.md b/changelog.md
index 5e8ad40c37..1de7d3afe9 100644
--- a/changelog.md
+++ b/changelog.md
@@ -29,6 +29,9 @@
 
 
 
+- Fix a bug where calling `close` on io streams in osproc.startProcess was a noop and led to
+  hangs if a process had both reads from stdin and writes (eg to stdout).
+
 ## Language changes
 
 
diff --git a/lib/pure/osproc.nim b/lib/pure/osproc.nim
index 4608ffb516..82ed5f4343 100644
--- a/lib/pure/osproc.nim
+++ b/lib/pure/osproc.nim
@@ -440,7 +440,11 @@ when defined(Windows) and not defined(useNimRtl):
       handle: Handle
       atTheEnd: bool
 
-  proc hsClose(s: Stream) = discard # nothing to do here
+  proc hsClose(s: Stream) =
+    # xxx here + elsewhere: check instead of discard; ignoring errors leads to
+    # hard to track bugs
+    discard FileHandleStream(s).handle.closeHandle
+
   proc hsAtEnd(s: Stream): bool = return FileHandleStream(s).atTheEnd
 
   proc hsReadData(s: Stream, buffer: pointer, bufLen: int): int =
diff --git a/lib/windows/winlean.nim b/lib/windows/winlean.nim
index 1eba3ac36c..e6fd4b0afa 100644
--- a/lib/windows/winlean.nim
+++ b/lib/windows/winlean.nim
@@ -33,6 +33,8 @@ type
   ULONG* = int32
   PULONG* = ptr int
   WINBOOL* = int32
+    ## `WINBOOL` uses opposite convention as posix, !=0 meaning success.
+    # xxx this should be distinct int32, distinct would make code less error prone
   DWORD* = int32
   PDWORD* = ptr DWORD
   LPINT* = ptr int32
diff --git a/tests/stdlib/tosproc.nim b/tests/stdlib/tosproc.nim
index 1859877e74..ec6e8421f6 100644
--- a/tests/stdlib/tosproc.nim
+++ b/tests/stdlib/tosproc.nim
@@ -93,3 +93,29 @@ else:
       removeFile(exePath)
     except OSError:
       discard
+
+  import std/streams
+  block: # test for startProcess (more tests needed)
+    # bugfix: windows stdin.close was a noop and led to blocking reads
+    proc startProcessTest(command: string, options: set[ProcessOption] = {
+                    poStdErrToStdOut, poUsePath}, input = ""): tuple[
+                    output: TaintedString,
+                    exitCode: int] {.tags:
+                    [ExecIOEffect, ReadIOEffect, RootEffect], gcsafe.} =
+      var p = startProcess(command, options = options + {poEvalCommand})
+      var outp = outputStream(p)
+      if input.len > 0: inputStream(p).write(input)
+      close inputStream(p)
+      result = (TaintedString"", -1)
+      var line = newStringOfCap(120).TaintedString
+      while true:
+        if outp.readLine(line):
+          result[0].string.add(line.string)
+          result[0].string.add("\n")
+        else:
+          result[1] = peekExitCode(p)
+          if result[1] != -1: break
+      close(p)
+
+    var result = startProcessTest("nim r --hints:off -", options = {}, input = "echo 3*4")
+    doAssert result == ("12\n", 0)