-
Notifications
You must be signed in to change notification settings - Fork 877
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
Conversation
22e0a9c
to
216ecbf
Compare
216ecbf
to
f2a02c1
Compare
Fundamentally I think this looks okay, but I am very much not experienced in Windows development, particularly around this. Could, we, from a cleanliness perspective, use OsString in the APIs rather than Vec or would that be even more complex? @rbtcollins Could you please spare some time to review this, as I feel quite lost when it comes to faffing with Windows PATHs etc. |
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.
Thank you very much for closing this tech debt of ours. I think we have more than one copy of that PATH set-restore code that will need updating to be fully safe, but doing the core like you're doing is a very good start and sufficient to be merged once we work through my feedback here.
src/cli/self_update/test.rs
Outdated
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 comment
The reason will be displayed to describe this comment to others. Learn more.
typo 'trashing'
src/cli/self_update/test.rs
Outdated
|
||
#[cfg(not(unix))] | ||
pub fn get_path() -> Option<String> { | ||
pub fn get_path() -> Option<RegValue> { |
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 ?
- In
with_saved_path
or in each test ? - By ignoring the test or by panicking ?
src/cli/self_update/windows.rs
Outdated
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 comment
The reason will be displayed to describe this comment to others. Learn more.
please don't discard the explanatory comments about this byte sequence
src/cli/self_update/windows.rs
Outdated
) | ||
); | ||
} | ||
|
||
#[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 comment
The 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
src/cli/self_update/windows.rs
Outdated
}) | ||
}); | ||
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 comment
The 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.
If it isn't able to trigger the error, but something else is generating the error, the test isn't clear.
src/utils/utils.rs
Outdated
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 comment
The 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 comment
The 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.
@kinnison @rbtcollins thanks, I'll take time tomorrow to make the reqpired changes. About |
Done:
Questions:
|
Vec is fine everywhere then. I had thought add_to_path could avoid it, but if it can't there really is a clear win with Vec... Is there a ticket on OsString's limits? |
I also through so, but when I tried to see what it would look like to use I did not find an open issue in the rust repo, but I think this is a known limitation. |
I think its worth opening one; this is a decent example of it being desired but impossible. |
I just found that these issues are in the rfcs repo, not the rust one (so there is rust-lang/rfcs#900). Btw can you re-review my PR ? |
a0f495b
to
bdb97b9
Compare
@@ -524,6 521,41 @@ mod tests { | |||
}); | |||
} | |||
|
|||
#[test] | |||
fn windows_doesnt_mess_with_a_non_string_path() { |
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.
much better - thank you!
|
||
#[test] | ||
/// Smoke test for end-to-end code connectivity of the installer path mgmt on windows. | ||
fn install_uninstall_affect_path_with_non_unicode() { |
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.
thank you. I would have actually been ok with no functional test given the excellent local coverage we have now for this part of the system, but 🤷
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.
One last change: move the utils function you change into windows.rs (and probably change the name to something that reflects what it is actually doing for your new code) and we're good.
Done ! |
src/cli/self_update/windows.rs
Outdated
@@ -285,6 285,36 @@ pub fn do_remove_from_path() -> Result<()> { | |||
_apply_new_path(new_path) | |||
} | |||
|
|||
/// Encodes a utf-8 string as a null-terminated UCS-2 string in bytes |
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.
I think this comment is wrong though yes? the input isn't a utf8 string ;)
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.
Hope it is good now, not really sure of how to describe the operation
5cdc32c
to
0df3c7b
Compare
Thank you very much for adding this improvement! |
Fixes #265