fix a critical bug in windows.osproc leading to resource leaks and bl…

…ocking IO [backport] (#14296)

changelog.md

@@ -86,6 +86,9 @@
   an uninitialized `DateTime`, the exceptions are `==` and `$` (which returns `"Uninitialized DateTime"`). The proc `times.isInitialized`
   has been added which can be used to check if a `DateTime` has been initialized.
 
+- 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
 - In newruntime it is now allowed to assign discriminator field without restrictions as long as case object doesn't have custom destructor. Discriminator value doesn't have to be a constant either. If you have custom destructor for case object and you do want to freely assign discriminator fields, it is recommended to refactor object into 2 objects like this:
   ```nim

lib/pure/osproc.nim

@@ -441,7 +441,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 =

lib/windows/winlean.nim

@@ -31,6 +31,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

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)