-
Notifications
You must be signed in to change notification settings - Fork 884
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle PATHs with non-unicodes values on Windows #2649
Changes from 2 commits
7fac19d
f2a02c1
cdc3305
bdb97b9
0df3c7b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,39 +5,34 @@ 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<String> { | ||
pub fn get_path() -> Option<RegValue> { | ||
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) => Some(val), | ||
Err(ref e) if e.kind() == std::io::ErrorKind::NotFound => None, | ||
Err(e) => panic!( | ||
"Error getting PATH: {}\nBetter abort to avoid trahsing it.", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo 'trashing' |
||
e | ||
), | ||
} | ||
} | ||
|
||
#[cfg(not(unix))] | ||
fn restore_path(p: Option<String>) { | ||
fn restore_path(p: Option<RegValue>) { | ||
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"); | ||
} | ||
|
@@ -60,9 +55,9 @@ pub fn with_saved_path(f: &dyn Fn()) { | |
} | ||
|
||
#[cfg(unix)] | ||
pub fn get_path() -> Option<String> { | ||
pub fn get_path() -> Option<()> { | ||
None | ||
} | ||
|
||
#[cfg(unix)] | ||
fn restore_path(_: Option<String>) {} | ||
fn restore_path(_: Option<()>) {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -144,7 +144,7 @@ pub fn do_add_to_path() -> Result<()> { | |
_apply_new_path(new_path) | ||
} | ||
|
||
fn _apply_new_path(new_path: Option<String>) -> Result<()> { | ||
fn _apply_new_path(new_path: Option<Vec<u16>>) -> Result<()> { | ||
rbtcollins marked this conversation as resolved.
Show resolved
Hide resolved
|
||
use std::ptr; | ||
use winapi::shared::minwindef::*; | ||
use winapi::um::winuser::{ | ||
|
@@ -169,7 +169,7 @@ fn _apply_new_path(new_path: Option<String>) -> Result<()> { | |
.chain_err(|| ErrorKind::PermissionDenied)?; | ||
} else { | ||
let reg_value = RegValue { | ||
bytes: utils::string_to_winreg_bytes(&new_path), | ||
bytes: utils::string_to_winreg_bytes(new_path), | ||
vtype: RegType::REG_EXPAND_SZ, | ||
}; | ||
environment | ||
|
@@ -194,9 +194,9 @@ fn _apply_new_path(new_path: Option<String>) -> 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<Option<String>> { | ||
fn get_windows_path_var() -> Result<Option<Vec<u16>>> { | ||
rbtcollins marked this conversation as resolved.
Show resolved
Hide resolved
|
||
use std::io; | ||
use winreg::enums::{HKEY_CURRENT_USER, KEY_READ, KEY_WRITE}; | ||
use winreg::RegKey; | ||
|
@@ -212,63 +212,72 @@ fn get_windows_path_var() -> Result<Option<String>> { | |
if let Some(s) = utils::string_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<String> { | ||
fn _add_to_path(old_path: Vec<u16>, path_str: Vec<u16>) -> Option<Vec<u16>> { | ||
rbtcollins marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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<String> { | ||
let idx = old_path.find(&path_str)?; | ||
fn _remove_from_path(old_path: Vec<u16>, path_str: Vec<u16>) -> Option<Vec<u16>> { | ||
let idx = old_path | ||
.windows(path_str.len()) | ||
.position(|path| path == path_str)?; | ||
rbtcollins marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// 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: F) -> Result<Option<String>> | ||
fn _with_path_cargo_home_bin<F>(f: F) -> Result<Option<Vec<u16>>> | ||
where | ||
F: FnOnce(&str, String) -> Option<String>, | ||
F: FnOnce(Vec<u16>, Vec<u16>) -> Option<Vec<u16>>, | ||
{ | ||
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()?; | ||
rbtcollins marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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<()> { | ||
|
@@ -402,26 +411,33 @@ 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<u16> { | ||
OsString::from(str).encode_wide().collect() | ||
rbtcollins marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
#[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() { | ||
fn windows_doesnt_mess_with_a_non_string_path() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is still a non-unicode path, and we still want it not-messed with, so I don't think the title needs altering, does it? Perhaps you want to change it to "non_unicode_paths_are_handled_safely" or something |
||
// This writes an error, so we want a sink for it. | ||
let tp = Box::new(currentprocess::TestProcess { | ||
vars: [("HOME".to_string(), "/unused".to_string())] | ||
|
@@ -437,23 +453,19 @@ mod tests { | |
.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, | ||
bytes: vec![0x00, 0xD8, 0x01, 0x01, 0x00, 0x00], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please don't discard the explanatory comments about this byte sequence |
||
vtype: RegType::REG_BINARY, | ||
}; | ||
environment.set_raw_value("PATH", ®_value).unwrap(); | ||
// Ok(None) signals no change to the PATH setting layer | ||
fn panic(_: &str, _: String) -> Option<String> { | ||
panic!("called"); | ||
} | ||
assert_eq!(None, super::_with_path_cargo_home_bin(panic).unwrap()); | ||
assert_eq!( | ||
None, | ||
super::_with_path_cargo_home_bin(|_, _| panic!("called")).unwrap() | ||
); | ||
}) | ||
}); | ||
assert_eq!( | ||
r"warning: the registry key HKEY_CURRENT_USER\Environment\PATH does not contain valid Unicode. Not modifying the PATH variable | ||
r"warning: the registry key HKEY_CURRENT_USER\Environment\PATH is not a string. Not modifying the PATH variable | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand how we'll still see this error if your change to handle non-unicode values is complete. Perhaps you're adding a new corner case, or anticipating some other sort of error? If that non-unicode bytestring is able to trigger the error, then we don't actually handle non-unicode strings yet. |
||
", | ||
String::from_utf8(tp.get_stderr()).unwrap() | ||
); | ||
|
@@ -474,15 +486,15 @@ 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 | ||
.open_subkey_with_flags("Environment", KEY_READ | KEY_WRITE) | ||
.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!(utils::string_to_winreg_bytes(wide("foo")), &path.bytes[..]); | ||
}) | ||
}); | ||
} | ||
|
@@ -503,7 +515,7 @@ mod tests { | |
.set_raw_value( | ||
"PATH", | ||
&RegValue { | ||
bytes: utils::string_to_winreg_bytes("foo"), | ||
bytes: utils::string_to_winreg_bytes(wide("foo")), | ||
vtype: RegType::REG_EXPAND_SZ, | ||
}, | ||
) | ||
|
@@ -512,7 +524,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 { | ||
|
@@ -536,18 +548,18 @@ 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()); | ||
}) | ||
}); | ||
} | ||
|
||
#[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 +568,10 @@ mod tests { | |
#[test] | ||
fn windows_uninstall_removes_semicolon_from_path_suffix() { | ||
assert_eq!( | ||
"foo", | ||
wide("foo"), | ||
rbtcollins marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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() | ||
) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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<Url> { | |
} | ||
|
||
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), | ||
}) | ||
|
@@ -538,10 +539,8 @@ 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<u8> { | ||
use std::ffi::OsStr; | ||
use std::os::windows::ffi::OsStrExt; | ||
let v: Vec<u16> = OsStr::new(s).encode_wide().chain(Some(0)).collect(); | ||
pub fn string_to_winreg_bytes(mut v: Vec<u16>) -> Vec<u8> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. uhm, nope, lets not do this. I think we can just delete this helper if we switch to OsString. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so AFAICT this is only used from windows.rs now, lets just include it in that file and remove it from utils.rs - it will be cleaner and less of an attractive nuisance I think. |
||
v.push(0); | ||
unsafe { std::slice::from_raw_parts(v.as_ptr().cast::<u8>(), v.len() * 2).to_vec() } | ||
} | ||
|
||
|
@@ -550,23 +549,22 @@ pub fn string_to_winreg_bytes(s: &str) -> Vec<u8> { | |
// returns null. The winreg library itself does a lossy unicode | ||
// conversion. | ||
#[cfg(windows)] | ||
pub fn string_from_winreg_value(val: &winreg::RegValue) -> Option<String> { | ||
pub fn string_from_winreg_value(val: &winreg::RegValue) -> Option<Vec<u16>> { | ||
use std::slice; | ||
use winreg::enums::RegType; | ||
|
||
match val.vtype { | ||
RegType::REG_SZ | RegType::REG_EXPAND_SZ => { | ||
// Copied from winreg | ||
let words = unsafe { | ||
let mut words = unsafe { | ||
#[allow(clippy::cast_ptr_alignment)] | ||
slice::from_raw_parts(val.bytes.as_ptr().cast::<u16>(), val.bytes.len() / 2) | ||
.to_owned() | ||
}; | ||
String::from_utf16(words).ok().map(|mut s| { | ||
while s.ends_with('\u{0}') { | ||
s.pop(); | ||
} | ||
s | ||
}) | ||
while words.last() == Some(&0) { | ||
words.pop(); | ||
} | ||
Some(words) | ||
} | ||
_ => None, | ||
} | ||
|
@@ -741,7 +739,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<PathBuf> { | ||
use std::ffi::CStr; | ||
use std::ffi::{CStr, OsString}; | ||
use std::mem::MaybeUninit; | ||
use std::os::unix::ffi::OsStringExt; | ||
use std::ptr; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be a larger change, but how do you feel about changing the signature to Result<Option> rather than panicing ? We have other test stuff to unwind, and generally we've been heading towards Result...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where and how should we handle this result then ?
with_saved_path
or in each test ?