From e06641dbe155aa829f9438c795791856ee067a63 Mon Sep 17 00:00:00 2001 From: Noah <33094578+coolreader18@users.noreply.github.com> Date: Sun, 21 Jun 2020 22:34:59 -0500 Subject: [PATCH] Fix all the weird Windows bugs --- Lib/subprocess.py | 27 ++++++++++++++++++++------- tests/snippets/stdlib_subprocess.py | 28 ++++++++++++++++++++-------- vm/src/stdlib/os.rs | 9 ++++----- vm/src/stdlib/posixsubprocess.rs | 4 +--- vm/src/stdlib/winapi.rs | 12 ++++++++++-- 5 files changed, 55 insertions(+), 25 deletions(-) diff --git a/Lib/subprocess.py b/Lib/subprocess.py index 5c2c2f050..5ce787d61 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -807,13 +807,14 @@ class Popen(object): # 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 @@ class Popen(object): 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 @@ class Popen(object): 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 @@ class Popen(object): 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, diff --git a/tests/snippets/stdlib_subprocess.py b/tests/snippets/stdlib_subprocess.py index bbfdd88ca..96ead7658 100644 --- a/tests/snippets/stdlib_subprocess.py +++ b/tests/snippets/stdlib_subprocess.py @@ -5,7 +5,20 @@ import signal 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.returncode is None assert p.poll() == 0 assert p.returncode == 0 -p = subprocess.Popen(["sleep", "2"]) +p = subprocess.Popen(sleep(2)) assert p.poll() is None @@ -25,14 +38,13 @@ p.wait() 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: @@ -40,7 +52,7 @@ if is_unix: else: assert p.returncode == 1 -p = subprocess.Popen(["sleep", "2"]) +p = subprocess.Popen(sleep(2)) p.kill() p.wait() if is_unix: @@ -48,10 +60,10 @@ if is_unix: 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) diff --git a/vm/src/stdlib/os.rs b/vm/src/stdlib/os.rs index 529e1784c..2a1af9a38 100644 --- a/vm/src/stdlib/os.rs +++ b/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 { diff --git a/vm/src/stdlib/posixsubprocess.rs b/vm/src/stdlib/posixsubprocess.rs index ecada7567..42cb887f2 100644 --- a/vm/src/stdlib/posixsubprocess.rs +++ b/vm/src/stdlib/posixsubprocess.rs @@ -23,9 +23,7 @@ struct CStrPathLike { } impl TryFromObject for CStrPathLike { fn try_from_object(vm: &VirtualMachine, obj: PyObjectRef) -> PyResult { - 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 }) diff --git a/vm/src/stdlib/winapi.rs b/vm/src/stdlib/winapi.rs index 12caf8c6c..11956445a 100644 --- a/vm/src/stdlib/winapi.rs +++ b/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::() as _; + si.StartupInfo.cb = std::mem::size_of::() as _; macro_rules! si_attr { ($attr:ident, $t:ty) => {{ @@ -309,7 +309,7 @@ fn getattributelist(obj: PyObjectRef, vm: &VirtualMachine) -> PyResult()) as _, null_mut(), null_mut(), ) @@ -340,6 +340,13 @@ fn _winapi_GetExitCodeProcess(h: usize, vm: &VirtualMachine) -> PyResult { 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),