Fix all the weird Windows bugs

Author: coolreader18

Lib/subprocess.py

@@ -807,13 +807,14 @@ def __init__(self, args, bufsize=-1, executable=None,
         # quickly terminating child could make our fds unwrappable
         # (see #8458).
 
-        if _mswindows:
-            if p2cwrite != -1:
-                p2cwrite = msvcrt.open_osfhandle(p2cwrite.Detach(), 0)
-            if c2pread != -1:
-                c2pread = msvcrt.open_osfhandle(c2pread.Detach(), 0)
-            if errread != -1:
-                errread = msvcrt.open_osfhandle(errread.Detach(), 0)
+        # XXX RustPython TODO: have fds for fs functions be actual CRT fds on windows, not handles
+        # if _mswindows:
+        #    if p2cwrite != -1:
+        #        p2cwrite = msvcrt.open_osfhandle(p2cwrite.Detach(), 0)
+        #    if c2pread != -1:
+        #        c2pread = msvcrt.open_osfhandle(c2pread.Detach(), 0)
+        #    if errread != -1:
+        #        errread = msvcrt.open_osfhandle(errread.Detach(), 0)
 
         self.text_mode = encoding or errors or text or universal_newlines
 
@@ -1154,7 +1155,11 @@ def _get_handles(self, stdin, stdout, stderr):
             else:
                 # Assuming file-like object
                 p2cread = msvcrt.get_osfhandle(stdin.fileno())
+            # XXX RUSTPYTHON TODO: figure out why closing these old, non-inheritable
+            # pipe handles is necessary for us, but not CPython
+            old = p2cread
             p2cread = self._make_inheritable(p2cread)
+            if stdin == PIPE: _winapi.CloseHandle(old)
 
             if stdout is None:
                 c2pwrite = _winapi.GetStdHandle(_winapi.STD_OUTPUT_HANDLE)
@@ -1172,7 +1177,11 @@ def _get_handles(self, stdin, stdout, stderr):
             else:
                 # Assuming file-like object
                 c2pwrite = msvcrt.get_osfhandle(stdout.fileno())
+            # XXX RUSTPYTHON TODO: figure out why closing these old, non-inheritable
+            # pipe handles is necessary for us, but not CPython
+            old = c2pwrite
             c2pwrite = self._make_inheritable(c2pwrite)
+            if stdout == PIPE: _winapi.CloseHandle(old)
 
             if stderr is None:
                 errwrite = _winapi.GetStdHandle(_winapi.STD_ERROR_HANDLE)
@@ -1192,7 +1201,11 @@ def _get_handles(self, stdin, stdout, stderr):
             else:
                 # Assuming file-like object
                 errwrite = msvcrt.get_osfhandle(stderr.fileno())
+            # XXX RUSTPYTHON TODO: figure out why closing these old, non-inheritable
+            # pipe handles is necessary for us, but not CPython
+            old = errwrite
             errwrite = self._make_inheritable(errwrite)
+            if stderr == PIPE: _winapi.CloseHandle(old)
 
             return (p2cread, p2cwrite,
                     c2pread, c2pwrite,

tests/snippets/stdlib_subprocess.py

@@ -5,7 +5,20 @@
 
 from testutils import assert_raises
 
-p = subprocess.Popen(["echo", "test"])
+is_unix = not sys.platform.startswith("win")
+if is_unix:
+    def echo(text):
+        return ["echo", text]
+    def sleep(secs):
+        return ["sleep", str(secs)]
+else:
+    def echo(text):
+        return ["cmd", "/C", f"echo {text}"]
+    def sleep(secs):
+        # TODO: make work in a non-unixy environment (something with timeout.exe?)
+        return ["sleep", str(secs)]
+
+p = subprocess.Popen(echo("test"))
 
 time.sleep(0.1)
 
@@ -14,7 +27,7 @@
 assert p.poll() == 0
 assert p.returncode == 0
 
-p = subprocess.Popen(["sleep", "2"])
+p = subprocess.Popen(sleep(2))
 
 assert p.poll() is None
 
@@ -25,33 +38,32 @@
 
 assert p.returncode == 0
 
-p = subprocess.Popen(["echo", "test"], stdout=subprocess.PIPE)
+p = subprocess.Popen(echo("test"), stdout=subprocess.PIPE)
 p.wait()
 
-is_unix = "win" not in sys.platform or "darwin" in sys.platform
 
 assert p.stdout.read().strip() == b"test"
 
-p = subprocess.Popen(["sleep", "2"])
+p = subprocess.Popen(sleep(2))
 p.terminate()
 p.wait()
 if is_unix:
 	assert p.returncode == -signal.SIGTERM
 else:
 	assert p.returncode == 1
 
-p = subprocess.Popen(["sleep", "2"])
+p = subprocess.Popen(sleep(2))
 p.kill()
 p.wait()
 if is_unix:
 	assert p.returncode == -signal.SIGKILL
 else:
 	assert p.returncode == 1
 
-p = subprocess.Popen(["echo", "test"], stdout=subprocess.PIPE)
+p = subprocess.Popen(echo("test"), stdout=subprocess.PIPE)
 (stdout, stderr) = p.communicate()
 assert stdout.strip() == b"test"
 
-p = subprocess.Popen(["sleep", "5"], stdout=subprocess.PIPE)
+p = subprocess.Popen(sleep(5), stdout=subprocess.PIPE)
 with assert_raises(subprocess.TimeoutExpired):
 	p.communicate(timeout=1)

vm/src/stdlib/os.rs

@@ -28,7 +28,7 @@ use crate::exceptions::PyBaseExceptionRef;
 use crate::function::{IntoPyNativeFunc, OptionalArg, PyFuncArgs};
 use crate::obj::objbyteinner::PyBytesLike;
 use crate::obj::objbytes::{PyBytes, PyBytesRef};
-use crate::obj::objdict::{PyDictRef, PyMapping};
+use crate::obj::objdict::PyDictRef;
 use crate::obj::objint::PyIntRef;
 use crate::obj::objiter;
 use crate::obj::objset::PySet;
@@ -43,6 +43,8 @@ use crate::vm::VirtualMachine;
 
 // just to avoid unused import warnings
 #[cfg(unix)]
+use crate::obj::objdict::PyMapping;
+#[cfg(unix)]
 use crate::pyobject::PyIterable;
 #[cfg(unix)]
 use std::convert::TryFrom;
@@ -124,7 +126,7 @@ fn output_by_mode(val: String, mode: OutputMode, vm: &VirtualMachine) -> PyObjec
 }
 
 pub struct PyPathLike {
-    path: ffi::OsString,
+    pub path: ffi::OsString,
     mode: OutputMode,
 }
 
@@ -140,9 +142,6 @@ impl PyPathLike {
         use std::os::windows::ffi::OsStrExt;
         self.path.encode_wide().chain(std::iter::once(0)).collect()
     }
-    pub fn into_os_string(self) -> ffi::OsString {
-        self.path
-    }
 }
 
 impl TryFromObject for PyPathLike {

vm/src/stdlib/posixsubprocess.rs

@@ -23,9 +23,7 @@ struct CStrPathLike {
 }
 impl TryFromObject for CStrPathLike {
     fn try_from_object(vm: &VirtualMachine, obj: PyObjectRef) -> PyResult<Self> {
-        let s = PyPathLike::try_from_object(vm, obj)?
-            .into_os_string()
-            .into_vec();
+        let s = PyPathLike::try_from_object(vm, obj)?.path.into_vec();
         let s = CString::new(s)
             .map_err(|_| vm.new_value_error("embedded null character".to_owned()))?;
         Ok(CStrPathLike { s })

vm/src/stdlib/winapi.rs

@@ -132,7 +132,7 @@ fn _winapi_CreateProcess(
 ) -> PyResult<(usize, usize, u32, u32)> {
     use winbase::STARTUPINFOEXW;
     let mut si: STARTUPINFOEXW = unsafe { std::mem::zeroed() };
-    si.StartupInfo.cb = 84; // std::mem::size_of::<STARTUPINFOEXW>() as _;
+    si.StartupInfo.cb = std::mem::size_of::<STARTUPINFOEXW>() as _;
 
     macro_rules! si_attr {
         ($attr:ident, $t:ty) => {{
@@ -309,7 +309,7 @@ fn getattributelist(obj: PyObjectRef, vm: &VirtualMachine) -> PyResult<Option<At
                         0,
                         (2 & 0xffff) | 0x20000, // PROC_THREAD_ATTRIBUTE_HANDLE_LIST
                         handlelist.as_mut_ptr() as _,
-                        handlelist.len() as _,
+                        (handlelist.len() * std::mem::size_of::<HANDLE>()) as _,
                         null_mut(),
                         null_mut(),
                     )
@@ -340,6 +340,13 @@ fn _winapi_GetExitCodeProcess(h: usize, vm: &VirtualMachine) -> PyResult<u32> {
     Ok(ec)
 }
 
+fn _winapi_TerminateProcess(h: usize, exit_code: u32, vm: &VirtualMachine) -> PyResult<()> {
+    cvt(vm, unsafe {
+        processthreadsapi::TerminateProcess(h as _, exit_code)
+    })
+    .map(drop)
+}
+
 pub fn make_module(vm: &VirtualMachine) -> PyObjectRef {
     let ctx = &vm.ctx;
     py_module!(vm, "_winapi", {
@@ -351,6 +358,7 @@ pub fn make_module(vm: &VirtualMachine) -> PyObjectRef {
         "CreateProcess" => named_function!(ctx, _winapi, CreateProcess),
         "WaitForSingleObject" => named_function!(ctx, _winapi, WaitForSingleObject),
         "GetExitCodeProcess" => named_function!(ctx, _winapi, GetExitCodeProcess),
+        "TerminateProcess" => named_function!(ctx, _winapi, TerminateProcess),
 
         "WAIT_OBJECT_0" => ctx.new_int(winbase::WAIT_OBJECT_0),
         "WAIT_ABANDONED" => ctx.new_int(winbase::WAIT_ABANDONED),