From 4d3f6de41a4bb999c8941cab2962a2cb5fb91393 Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Wed, 11 Dec 2019 23:30:30 +0000 Subject: [PATCH] Fix vague startup crash messages with WinPTY Fixes #2344. --- CHANGELOG.md | 1 + winpty/src/windows.rs | 139 ++++++++++++++++++++++++------------------ 2 files changed, 80 insertions(+), 60 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ead9370..3bd9f84d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Width of Unicode 11/12 emojis - Minimize on windows causing layout issues - Performance bottleneck when clearing colored rows +- Vague startup crash messages on Windows with WinPTY backend ## 0.4.0 diff --git a/winpty/src/windows.rs b/winpty/src/windows.rs index 27780d4f..20cb3dc3 100644 --- a/winpty/src/windows.rs +++ b/winpty/src/windows.rs @@ -1,4 +1,3 @@ -use std::error::Error; use std::fmt::{self, Display, Formatter}; use std::os::windows::io::RawHandle; use std::path::PathBuf; @@ -9,8 +8,8 @@ use winpty_sys::*; use widestring::WideCString; -pub enum ErrorCodes { - Success, +#[derive(Copy, Clone, Debug)] +pub enum ErrorCode { OutOfMemory, SpawnCreateProcessFailed, LostConnection, @@ -19,6 +18,7 @@ pub enum ErrorCodes { AgentDied, AgentTimeout, AgentCreationFailed, + UnknownError(u32), } pub enum MouseMode { @@ -43,32 +43,43 @@ bitflags!( ); #[derive(Debug)] -pub struct Err { - code: u32, +pub struct Error { + code: ErrorCode, message: String, } // Check to see whether winpty gave us an error, and perform the necessary memory freeing -fn check_err(e: *mut winpty_error_t) -> Option { +fn check_err(e: *mut winpty_error_t) -> Result<(), Error> { unsafe { let code = winpty_error_code(e); let raw = winpty_error_msg(e); let message = String::from_utf16_lossy(std::slice::from_raw_parts(raw, wcslen(raw))); winpty_error_free(e); - match code { - 0 => None, - _ => Some(Err { code, message }), - } + + let code = match code { + WINPTY_ERROR_SUCCESS => return Ok(()), + WINPTY_ERROR_OUT_OF_MEMORY => ErrorCode::OutOfMemory, + WINPTY_ERROR_SPAWN_CREATE_PROCESS_FAILED => ErrorCode::SpawnCreateProcessFailed, + WINPTY_ERROR_LOST_CONNECTION => ErrorCode::LostConnection, + WINPTY_ERROR_AGENT_EXE_MISSING => ErrorCode::AgentExeMissing, + WINPTY_ERROR_UNSPECIFIED => ErrorCode::Unspecified, + WINPTY_ERROR_AGENT_DIED => ErrorCode::AgentDied, + WINPTY_ERROR_AGENT_TIMEOUT => ErrorCode::AgentTimeout, + WINPTY_ERROR_AGENT_CREATION_FAILED => ErrorCode::AgentCreationFailed, + code => ErrorCode::UnknownError(code), + }; + + Err(Error { code, message }) } } -impl Display for Err { +impl Display for Error { fn fmt(&self, f: &mut Formatter) -> Result<(), fmt::Error> { - write!(f, "Code: {}, Message: {}", self.code, self.message) + write!(f, "Code: {:?}, Message: {}", self.code, self.message) } } -impl Error for Err { +impl std::error::Error for Error { fn description(&self) -> &str { &self.message } @@ -79,10 +90,12 @@ impl Error for Err { pub struct Config(*mut winpty_config_t); impl Config { - pub fn new(flags: ConfigFlags) -> Result { + pub fn new(flags: ConfigFlags) -> Result { let mut err = null_mut() as *mut winpty_error_t; let config = unsafe { winpty_config_new(flags.bits(), &mut err) }; - check_err(err).map_or(Ok(Self(config)), Result::Err) + check_err(err)?; + + Ok(Self(config)) } /// Set the initial size of the console window @@ -95,12 +108,12 @@ impl Config { /// Set the mouse mode pub fn set_mouse_mode(&mut self, mode: &MouseMode) { let m = match mode { - MouseMode::None => 0, - MouseMode::Auto => 1, - MouseMode::Force => 2, + MouseMode::None => WINPTY_MOUSE_MODE_NONE, + MouseMode::Auto => WINPTY_MOUSE_MODE_AUTO, + MouseMode::Force => WINPTY_MOUSE_MODE_FORCE, }; unsafe { - winpty_config_set_mouse_mode(self.0, m); + winpty_config_set_mouse_mode(self.0, m as i32); } } @@ -127,14 +140,21 @@ impl Drop for Config { /// A struct representing the winpty agent process pub struct Winpty(*mut winpty_t); +pub struct ChildHandles { + pub process: HANDLE, + pub thread: HANDLE, +} + impl Winpty { /// Starts the agent. This process will connect to the agent /// over a control pipe, and the agent will open data pipes /// (e.g. CONIN and CONOUT). - pub fn open(cfg: &Config) -> Result { + pub fn open(cfg: &Config) -> Result { let mut err = null_mut() as *mut winpty_error_t; let winpty = unsafe { winpty_open(cfg.0, &mut err) }; - check_err(err).map_or(Ok(Self(winpty)), Result::Err) + check_err(err)?; + + Ok(Self(winpty)) } /// Returns the handle to the winpty agent process @@ -173,7 +193,7 @@ impl Winpty { /// Change the size of the Windows console window. /// /// cols & rows MUST be greater than 0 - pub fn set_size(&mut self, cols: u16, rows: u16) -> Result<(), Err> { + pub fn set_size(&mut self, cols: u16, rows: u16) -> Result<(), Error> { assert!(cols > 0 && rows > 0); let mut err = null_mut() as *mut winpty_error_t; @@ -181,14 +201,14 @@ impl Winpty { winpty_set_size(self.0, i32::from(cols), i32::from(rows), &mut err); } - check_err(err).map_or(Ok(()), Result::Err) + check_err(err) } /// Get the list of processes running in the winpty agent. Returns <= count processes /// /// `count` must be greater than 0. Larger values cause a larger allocation. // TODO: This should return Vec instead of Vec - pub fn console_process_list(&mut self, count: usize) -> Result, Err> { + pub fn console_process_list(&mut self, count: usize) -> Result, Error> { assert!(count > 0); let mut err = null_mut() as *mut winpty_error_t; @@ -204,7 +224,9 @@ impl Winpty { process_list.set_len(len); } - check_err(err).map_or(Ok(process_list), Result::Err) + check_err(err)?; + + Ok(process_list) } /// Spawns the new process. @@ -213,26 +235,29 @@ impl Winpty { /// before the output data pipe(s) is/are connected, then collected output is /// buffered until the pipes are connected, rather than being discarded. /// (https://blogs.msdn.microsoft.com/oldnewthing/20110107-00/?p=11803) - // TODO: Support getting the process and thread handle of the spawned process (Not the agent) - // TODO: Support returning the error from CreateProcess - pub fn spawn(&mut self, cfg: &SpawnConfig) -> Result<(), Err> { + pub fn spawn(&mut self, cfg: &SpawnConfig) -> Result { + let mut handles = + ChildHandles { process: std::ptr::null_mut(), thread: std::ptr::null_mut() }; + + let mut create_process_error: DWORD = 0; let mut err = null_mut() as *mut winpty_error_t; unsafe { - let ok = winpty_spawn( + winpty_spawn( self.0, cfg.0 as *const winpty_spawn_config_s, - null_mut(), // Process handle - null_mut(), // Thread handle - null_mut(), // Create process error + &mut handles.process as *mut _, + &mut handles.thread as *mut _, + &mut create_process_error as *mut _, &mut err, ); - if ok == 0 { - return Ok(()); - } } - check_err(err).map_or(Ok(()), Result::Err) + let mut result = check_err(err); + if let Err(Error { code: ErrorCode::SpawnCreateProcessFailed, message }) = &mut result { + *message = format!("{} (error code {})", message, create_process_error); + } + result.map(|_| handles) } } @@ -260,36 +285,30 @@ impl SpawnConfig { cmdline: Option<&str>, cwd: Option<&str>, end: Option<&str>, - ) -> Result { + ) -> Result { let mut err = null_mut() as *mut winpty_error_t; - let (appname, cmdline, cwd, end) = ( - appname.map_or(null(), |s| WideCString::from_str(s).unwrap().into_raw()), - cmdline.map_or(null(), |s| WideCString::from_str(s).unwrap().into_raw()), - cwd.map_or(null(), |s| WideCString::from_str(s).unwrap().into_raw()), - end.map_or(null(), |s| WideCString::from_str(s).unwrap().into_raw()), - ); + let to_wstring = |s| WideCString::from_str(s).unwrap(); + let appname = appname.map(to_wstring); + let cmdline = cmdline.map(to_wstring); + let cwd = cwd.map(to_wstring); + let end = end.map(to_wstring); + + let wstring_ptr = |opt: &Option| opt.as_ref().map_or(null(), |ws| ws.as_ptr()); let spawn_config = unsafe { - winpty_spawn_config_new(spawnflags.bits(), appname, cmdline, cwd, end, &mut err) + winpty_spawn_config_new( + spawnflags.bits(), + wstring_ptr(&appname), + wstring_ptr(&cmdline), + wstring_ptr(&cwd), + wstring_ptr(&end), + &mut err, + ) }; - // Required to free the strings - unsafe { - if !appname.is_null() { - WideCString::from_raw(appname as *mut u16); - } - if !cmdline.is_null() { - WideCString::from_raw(cmdline as *mut u16); - } - if !cwd.is_null() { - WideCString::from_raw(cwd as *mut u16); - } - if !end.is_null() { - WideCString::from_raw(end as *mut u16); - } - } + check_err(err)?; - check_err(err).map_or(Ok(Self(spawn_config)), Result::Err) + Ok(Self(spawn_config)) } }