diff --git a/src/cli/self_update/test.rs b/src/cli/self_update/test.rs index 9c4e100556..29276eeedf 100644 --- a/src/cli/self_update/test.rs +++ b/src/cli/self_update/test.rs @@ -5,39 +5,31 @@ use std::sync::Mutex; use lazy_static::lazy_static; #[cfg(not(unix))] use winreg::{ - enums::{RegType, HKEY_CURRENT_USER, KEY_READ, KEY_WRITE}, + enums::{HKEY_CURRENT_USER, KEY_READ, KEY_WRITE}, RegKey, RegValue, }; #[cfg(not(unix))] -use crate::utils::utils; - -#[cfg(not(unix))] -pub fn get_path() -> Option { +pub fn get_path() -> std::io::Result> { let root = RegKey::predef(HKEY_CURRENT_USER); let environment = root .open_subkey_with_flags("Environment", KEY_READ | KEY_WRITE) .unwrap(); - // XXX: copied from the mock support crate, but I am suspicous of this - // code: This uses ok to allow signalling None for 'delete', but this - // can fail e.g. with !(winerror::ERROR_BAD_FILE_TYPE) or other - // failures; which will lead to attempting to delete the users path - // rather than aborting the test suite. - environment.get_value("PATH").ok() + match environment.get_raw_value("PATH") { + Ok(val) => Ok(Some(val)), + Err(ref e) if e.kind() == std::io::ErrorKind::NotFound => Ok(None), + Err(e) => Err(e), + } } #[cfg(not(unix))] -fn restore_path(p: Option) { +fn restore_path(p: Option) { let root = RegKey::predef(HKEY_CURRENT_USER); let environment = root .open_subkey_with_flags("Environment", KEY_READ | KEY_WRITE) .unwrap(); if let Some(p) = p.as_ref() { - let reg_value = RegValue { - bytes: utils::string_to_winreg_bytes(&p), - vtype: RegType::REG_EXPAND_SZ, - }; - environment.set_raw_value("PATH", ®_value).unwrap(); + environment.set_raw_value("PATH", &p).unwrap(); } else { let _ = environment.delete_value("PATH"); } @@ -53,16 +45,16 @@ pub fn with_saved_path(f: &dyn Fn()) { // On windows these tests mess with the user's PATH. Save // and restore them here to keep from trashing things. - let saved_path = get_path(); + let saved_path = get_path().expect("Error getting PATH: Better abort to avoid trashing it."); let _g = scopeguard::guard(saved_path, restore_path); f(); } #[cfg(unix)] -pub fn get_path() -> Option { - None +pub fn get_path() -> std::io::Result> { + Ok(None) } #[cfg(unix)] -fn restore_path(_: Option) {} +fn restore_path(_: Option<()>) {} diff --git a/src/cli/self_update/windows.rs b/src/cli/self_update/windows.rs index a434410b6d..400459da59 100644 --- a/src/cli/self_update/windows.rs +++ b/src/cli/self_update/windows.rs @@ -144,7 +144,7 @@ pub fn do_add_to_path() -> Result<()> { _apply_new_path(new_path) } -fn _apply_new_path(new_path: Option) -> Result<()> { +fn _apply_new_path(new_path: Option>) -> Result<()> { use std::ptr; use winapi::shared::minwindef::*; use winapi::um::winuser::{ @@ -169,7 +169,7 @@ fn _apply_new_path(new_path: Option) -> Result<()> { .chain_err(|| ErrorKind::PermissionDenied)?; } else { let reg_value = RegValue { - bytes: utils::string_to_winreg_bytes(&new_path), + bytes: to_winreg_bytes(new_path), vtype: RegType::REG_EXPAND_SZ, }; environment @@ -194,9 +194,9 @@ fn _apply_new_path(new_path: Option) -> Result<()> { } // Get the windows PATH variable out of the registry as a String. If -// this returns None then the PATH variable is not unicode and we +// this returns None then the PATH variable is not a string and we // should not mess with it. -fn get_windows_path_var() -> Result> { +fn get_windows_path_var() -> Result>> { use std::io; use winreg::enums::{HKEY_CURRENT_USER, KEY_READ, KEY_WRITE}; use winreg::RegKey; @@ -209,66 +209,75 @@ fn get_windows_path_var() -> Result> { let reg_value = environment.get_raw_value("PATH"); match reg_value { Ok(val) => { - if let Some(s) = utils::string_from_winreg_value(&val) { + if let Some(s) = from_winreg_value(&val) { Ok(Some(s)) } else { - warn!("the registry key HKEY_CURRENT_USER\\Environment\\PATH does not contain valid Unicode. \ - Not modifying the PATH variable"); + warn!( + "the registry key HKEY_CURRENT_USER\\Environment\\PATH is not a string. \ + Not modifying the PATH variable" + ); Ok(None) } } - Err(ref e) if e.kind() == io::ErrorKind::NotFound => Ok(Some(String::new())), + Err(ref e) if e.kind() == io::ErrorKind::NotFound => Ok(Some(Vec::new())), Err(e) => Err(e).chain_err(|| ErrorKind::WindowsUninstallMadness), } } // Returns None if the existing old_path does not need changing, otherwise // prepends the path_str to old_path, handling empty old_path appropriately. -fn _add_to_path(old_path: &str, path_str: String) -> Option { +fn _add_to_path(old_path: Vec, path_str: Vec) -> Option> { if old_path.is_empty() { Some(path_str) - } else if old_path.contains(&path_str) { + } else if old_path + .windows(path_str.len()) + .any(|path| path == path_str) + { None } else { let mut new_path = path_str; - new_path.push_str(";"); - new_path.push_str(&old_path); + new_path.push(b';' as u16); + new_path.extend_from_slice(&old_path); Some(new_path) } } // Returns None if the existing old_path does not need changing -fn _remove_from_path(old_path: &str, path_str: String) -> Option { - let idx = old_path.find(&path_str)?; +fn _remove_from_path(old_path: Vec, path_str: Vec) -> Option> { + let idx = old_path + .windows(path_str.len()) + .position(|path| path == path_str)?; // If there's a trailing semicolon (likely, since we probably added one // during install), include that in the substring to remove. We don't search // for that to find the string, because if its the last string in the path, // there may not be. let mut len = path_str.len(); - if old_path.as_bytes().get(idx + path_str.len()) == Some(&b';') { + if old_path.get(idx + path_str.len()) == Some(&(b';' as u16)) { len += 1; } - let mut new_path = old_path[..idx].to_string(); - new_path.push_str(&old_path[idx + len..]); + let mut new_path = old_path[..idx].to_owned(); + new_path.extend_from_slice(&old_path[idx + len..]); // Don't leave a trailing ; though, we don't want an empty string in the // path. - if new_path.ends_with(';') { + if new_path.last() == Some(&(b';' as u16)) { new_path.pop(); } Some(new_path) } -fn _with_path_cargo_home_bin(f: F) -> Result> +fn _with_path_cargo_home_bin(f: F) -> Result>> where - F: FnOnce(&str, String) -> Option, + F: FnOnce(Vec, Vec) -> Option>, { + use std::ffi::OsString; + use std::os::windows::ffi::OsStrExt; + let windows_path = get_windows_path_var()?; - let path_str = utils::cargo_home()? - .join("bin") - .to_string_lossy() - .into_owned(); - Ok(windows_path.and_then(|old_path| f(&old_path, path_str))) + let mut path_str = utils::cargo_home()?; + path_str.push("bin"); + Ok(windows_path + .and_then(|old_path| f(old_path, OsString::from(path_str).encode_wide().collect()))) } pub fn do_remove_from_path() -> Result<()> { @@ -276,6 +285,36 @@ pub fn do_remove_from_path() -> Result<()> { _apply_new_path(new_path) } +/// Convert a vector UCS-2 chars to a null-terminated UCS-2 string in bytes +pub fn to_winreg_bytes(mut v: Vec) -> Vec { + v.push(0); + unsafe { std::slice::from_raw_parts(v.as_ptr().cast::(), v.len() * 2).to_vec() } +} + +/// This is used to decode the value of HKCU\Environment\PATH. If that key is +/// not REG_SZ | REG_EXPAND_SZ then this returns None. The winreg library itself +/// does a lossy unicode conversion. +pub fn from_winreg_value(val: &winreg::RegValue) -> Option> { + use std::slice; + use winreg::enums::RegType; + + match val.vtype { + RegType::REG_SZ | RegType::REG_EXPAND_SZ => { + // Copied from winreg + let mut words = unsafe { + #[allow(clippy::cast_ptr_alignment)] + slice::from_raw_parts(val.bytes.as_ptr().cast::(), val.bytes.len() / 2) + .to_owned() + }; + while words.last() == Some(&0) { + words.pop(); + } + Some(words) + } + _ => None, + } +} + pub fn run_update(setup_path: &Path) -> Result { Command::new(setup_path) .arg("--self-replace") @@ -402,60 +441,47 @@ pub fn delete_rustup_and_cargo_home() -> Result<()> { #[cfg(test)] mod tests { + use std::ffi::OsString; + use std::os::windows::ffi::OsStrExt; + use winreg::enums::{RegType, HKEY_CURRENT_USER, KEY_READ, KEY_WRITE}; use winreg::{RegKey, RegValue}; use crate::currentprocess; use crate::test::with_saved_path; - use crate::utils::utils; + + fn wide(str: &str) -> Vec { + OsString::from(str).encode_wide().collect() + } #[test] fn windows_install_does_not_add_path_twice() { assert_eq!( None, super::_add_to_path( - r"c:\users\example\.cargo\bin;foo", - r"c:\users\example\.cargo\bin".into() + wide(r"c:\users\example\.cargo\bin;foo"), + wide(r"c:\users\example\.cargo\bin") ) ); } #[test] - fn windows_doesnt_mess_with_a_non_unicode_path() { - // This writes an error, so we want a sink for it. - let tp = Box::new(currentprocess::TestProcess { - vars: [("HOME".to_string(), "/unused".to_string())] - .iter() - .cloned() - .collect(), - ..Default::default() - }); - with_saved_path(&|| { - currentprocess::with(tp.clone(), || { - let root = RegKey::predef(HKEY_CURRENT_USER); - let environment = root - .open_subkey_with_flags("Environment", KEY_READ | KEY_WRITE) - .unwrap(); - let reg_value = RegValue { - bytes: vec![ - 0x00, 0xD8, // leading surrogate - 0x01, 0x01, // bogus trailing surrogate - 0x00, 0x00, - ], // null - vtype: RegType::REG_EXPAND_SZ, - }; - environment.set_raw_value("PATH", ®_value).unwrap(); - // Ok(None) signals no change to the PATH setting layer - fn panic(_: &str, _: String) -> Option { - panic!("called"); - } - assert_eq!(None, super::_with_path_cargo_home_bin(panic).unwrap()); - }) - }); + fn windows_handle_non_unicode_path() { + let initial_path = vec![ + 0xD800, // leading surrogate + 0x0101, // bogus trailing surrogate + 0x0000, // null + ]; + let cargo_home = wide(r"c:\users\example\.cargo\bin"); + let final_path = [&cargo_home, &[b';' as u16][..], &initial_path].join(&[][..]); + assert_eq!( - r"warning: the registry key HKEY_CURRENT_USER\Environment\PATH does not contain valid Unicode. Not modifying the PATH variable -", - String::from_utf8(tp.get_stderr()).unwrap() + &final_path, + &super::_add_to_path(initial_path.clone(), cargo_home.clone(),).unwrap() + ); + assert_eq!( + &initial_path, + &super::_remove_from_path(final_path, cargo_home,).unwrap() ); } @@ -474,7 +500,7 @@ mod tests { { // Can't compare the Results as Eq isn't derived; thanks error-chain. #![allow(clippy::unit_cmp)] - assert_eq!((), super::_apply_new_path(Some("foo".into())).unwrap()); + assert_eq!((), super::_apply_new_path(Some(wide("foo"))).unwrap()); } let root = RegKey::predef(HKEY_CURRENT_USER); let environment = root @@ -482,7 +508,7 @@ mod tests { .unwrap(); let path = environment.get_raw_value("PATH").unwrap(); assert_eq!(path.vtype, RegType::REG_EXPAND_SZ); - assert_eq!(utils::string_to_winreg_bytes("foo"), &path.bytes[..]); + assert_eq!(super::to_winreg_bytes(wide("foo")), &path.bytes[..]); }) }); } @@ -503,7 +529,7 @@ mod tests { .set_raw_value( "PATH", &RegValue { - bytes: utils::string_to_winreg_bytes("foo"), + bytes: super::to_winreg_bytes(wide("foo")), vtype: RegType::REG_EXPAND_SZ, }, ) @@ -512,7 +538,7 @@ mod tests { { // Can't compare the Results as Eq isn't derived; thanks error-chain. #![allow(clippy::unit_cmp)] - assert_eq!((), super::_apply_new_path(Some("".into())).unwrap()); + assert_eq!((), super::_apply_new_path(Some(Vec::new())).unwrap()); } let reg_value = environment.get_raw_value("PATH"); match reg_value { @@ -524,6 +550,41 @@ mod tests { }); } + #[test] + fn windows_doesnt_mess_with_a_non_string_path() { + // This writes an error, so we want a sink for it. + let tp = Box::new(currentprocess::TestProcess { + vars: [("HOME".to_string(), "/unused".to_string())] + .iter() + .cloned() + .collect(), + ..Default::default() + }); + with_saved_path(&|| { + currentprocess::with(tp.clone(), || { + let root = RegKey::predef(HKEY_CURRENT_USER); + let environment = root + .open_subkey_with_flags("Environment", KEY_READ | KEY_WRITE) + .unwrap(); + let reg_value = RegValue { + bytes: vec![0x12, 0x34], + vtype: RegType::REG_BINARY, + }; + environment.set_raw_value("PATH", ®_value).unwrap(); + // Ok(None) signals no change to the PATH setting layer + assert_eq!( + None, + super::_with_path_cargo_home_bin(|_, _| panic!("called")).unwrap() + ); + }) + }); + assert_eq!( + r"warning: the registry key HKEY_CURRENT_USER\Environment\PATH is not a string. Not modifying the PATH variable +", + String::from_utf8(tp.get_stderr()).unwrap() + ); + } + #[test] fn windows_treat_missing_path_as_empty() { // during install the PATH key may be missing; treat it as empty @@ -536,7 +597,7 @@ mod tests { .unwrap(); environment.delete_value("PATH").unwrap(); - assert_eq!(Some("".into()), super::get_windows_path_var().unwrap()); + assert_eq!(Some(Vec::new()), super::get_windows_path_var().unwrap()); }) }); } @@ -544,10 +605,10 @@ mod tests { #[test] fn windows_uninstall_removes_semicolon_from_path_prefix() { assert_eq!( - "foo", + wide("foo"), super::_remove_from_path( - r"c:\users\example\.cargo\bin;foo", - r"c:\users\example\.cargo\bin".into() + wide(r"c:\users\example\.cargo\bin;foo"), + wide(r"c:\users\example\.cargo\bin"), ) .unwrap() ) @@ -556,10 +617,10 @@ mod tests { #[test] fn windows_uninstall_removes_semicolon_from_path_suffix() { assert_eq!( - "foo", + wide("foo"), super::_remove_from_path( - r"foo;c:\users\example\.cargo\bin", - r"c:\users\example\.cargo\bin".into() + wide(r"foo;c:\users\example\.cargo\bin"), + wide(r"c:\users\example\.cargo\bin"), ) .unwrap() ) diff --git a/src/utils/utils.rs b/src/utils/utils.rs index 1385f297cf..2fc25901fc 100644 --- a/src/utils/utils.rs +++ b/src/utils/utils.rs @@ -1,6 +1,5 @@ use std::cmp::Ord; use std::env; -use std::ffi::OsString; use std::fs::{self, File}; use std::io::{self, BufReader, Write}; use std::path::{Path, PathBuf}; @@ -275,6 +274,8 @@ pub fn parse_url(url: &str) -> Result { } pub fn cmd_status(name: &'static str, cmd: &mut Command) -> Result<()> { + use std::ffi::OsString; + raw::cmd_status(cmd).chain_err(|| ErrorKind::RunningCommand { name: OsString::from(name), }) @@ -536,42 +537,6 @@ pub fn format_path_for_display(path: &str) -> String { } } -/// Encodes a utf-8 string as a null-terminated UCS-2 string in bytes -#[cfg(windows)] -pub fn string_to_winreg_bytes(s: &str) -> Vec { - use std::ffi::OsStr; - use std::os::windows::ffi::OsStrExt; - let v: Vec = OsStr::new(s).encode_wide().chain(Some(0)).collect(); - unsafe { std::slice::from_raw_parts(v.as_ptr().cast::(), v.len() * 2).to_vec() } -} - -// This is used to decode the value of HKCU\Environment\PATH. If that -// key is not unicode (or not REG_SZ | REG_EXPAND_SZ) then this -// returns null. The winreg library itself does a lossy unicode -// conversion. -#[cfg(windows)] -pub fn string_from_winreg_value(val: &winreg::RegValue) -> Option { - use std::slice; - use winreg::enums::RegType; - - match val.vtype { - RegType::REG_SZ | RegType::REG_EXPAND_SZ => { - // Copied from winreg - let words = unsafe { - #[allow(clippy::cast_ptr_alignment)] - slice::from_raw_parts(val.bytes.as_ptr().cast::(), val.bytes.len() / 2) - }; - String::from_utf16(words).ok().map(|mut s| { - while s.ends_with('\u{0}') { - s.pop(); - } - s - }) - } - _ => None, - } -} - pub fn toolchain_sort>(v: &mut Vec) { use semver::{Identifier, Version}; @@ -741,7 +706,7 @@ impl<'a> io::Read for FileReaderWithProgress<'a> { // search user database to get home dir of euid user #[cfg(unix)] pub fn home_dir_from_passwd() -> Option { - use std::ffi::CStr; + use std::ffi::{CStr, OsString}; use std::mem::MaybeUninit; use std::os::unix::ffi::OsStringExt; use std::ptr; diff --git a/tests/cli-paths.rs b/tests/cli-paths.rs index e45681e0ae..686d4c1a76 100644 --- a/tests/cli-paths.rs +++ b/tests/cli-paths.rs @@ -309,21 +309,69 @@ mod windows { use crate::mock::clitools::{self, expect_ok, Scenario}; #[test] - #[cfg(windows)] /// Smoke test for end-to-end code connectivity of the installer path mgmt on windows. fn install_uninstall_affect_path() { clitools::setup(Scenario::Empty, &|config| { with_saved_path(&|| { - let path = config.cargodir.join("bin").to_string_lossy().to_string(); + let path = format!("{:?}", config.cargodir.join("bin").to_string_lossy()); expect_ok(config, &INIT_NONE); assert!( - get_path().unwrap().contains(&path), - format!("`{}` not in `{}`", get_path().unwrap(), &path) + get_path() + .unwrap() + .unwrap() + .to_string() + .contains(path.trim_matches('"')), + format!("`{}` not in `{}`", path, get_path().unwrap().unwrap()) ); expect_ok(config, &["rustup", "self", "uninstall", "-y"]); - assert!(!get_path().unwrap().contains(&path)); + assert!(!get_path().unwrap().unwrap().to_string().contains(&path)); + }) + }); + } + + #[test] + /// Smoke test for end-to-end code connectivity of the installer path mgmt on windows. + fn install_uninstall_affect_path_with_non_unicode() { + use std::ffi::OsString; + use std::os::windows::ffi::OsStrExt; + use winreg::enums::{RegType, HKEY_CURRENT_USER, KEY_READ, KEY_WRITE}; + use winreg::{RegKey, RegValue}; + + clitools::setup(Scenario::Empty, &|config| { + with_saved_path(&|| { + // Set up a non unicode PATH + let reg_value = RegValue { + bytes: vec![ + 0x00, 0xD8, // leading surrogate + 0x01, 0x01, // bogus trailing surrogate + 0x00, 0x00, // null + ], + vtype: RegType::REG_EXPAND_SZ, + }; + RegKey::predef(HKEY_CURRENT_USER) + .open_subkey_with_flags("Environment", KEY_READ | KEY_WRITE) + .unwrap() + .set_raw_value("PATH", ®_value) + .unwrap(); + + // compute expected path after installation + let expected = RegValue { + bytes: OsString::from(config.cargodir.join("bin")) + .encode_wide() + .flat_map(|v| vec![v as u8, (v >> 8) as u8]) + .chain(vec![b';', 0]) + .chain(reg_value.bytes.iter().copied()) + .collect(), + vtype: RegType::REG_EXPAND_SZ, + }; + + expect_ok(config, &INIT_NONE); + assert_eq!(get_path().unwrap().unwrap(), expected); + + expect_ok(config, &["rustup", "self", "uninstall", "-y"]); + assert_eq!(get_path().unwrap().unwrap(), reg_value); }) }); }