Handle PATHs with non-unicodes values on Windows by kellda · Pull Request #2649 · rust-lang/rustup

kellda · 2021-02-05T13:52:15Z

Fixes #265

kinnison · 2021-02-07T09:33:31Z

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.

rbtcollins

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.

rbtcollins · 2021-02-07T16:22:26Z

src/cli/self_update/test.rs

+        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.",


typo 'trashing'

rbtcollins · 2021-02-07T16:24:12Z

src/cli/self_update/test.rs

-
-#[cfg(not(unix))]
-pub fn get_path() -> Option<String> {
+pub fn get_path() -> Option<RegValue> {


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...

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

rbtcollins · 2021-02-07T16:43:53Z

src/cli/self_update/windows.rs

-                        0x00, 0x00,
-                    ], // null
-                    vtype: RegType::REG_EXPAND_SZ,
+                    bytes: vec![0x00, 0xD8, 0x01, 0x01, 0x00, 0x00],


please don't discard the explanatory comments about this byte sequence

rbtcollins · 2021-02-07T16:44:39Z

src/cli/self_update/windows.rs


    #[test]
-    fn windows_doesnt_mess_with_a_non_unicode_path() {
+    fn windows_doesnt_mess_with_a_non_string_path() {


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

rbtcollins · 2021-02-07T16:54:34Z

src/cli/self_update/windows.rs

        });
        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


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/cli/self_update/windows.rs

rbtcollins · 2021-02-07T16:57:07Z

src/utils/utils.rs

-    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> {


uhm, nope, lets not do this. I think we can just delete this helper if we switch to OsString.

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.

kellda · 2021-02-07T17:28:28Z

@kinnison @rbtcollins thanks, I'll take time tomorrow to make the reqpired changes.

About Vec<u16> and OsString: I understand OsString would be cleaner, and I initialy tried to use it, but it has currently really few methods available (i.e. just push and clear really modify it). So most of the time using OsString would mean encode_wide, edit, from_wide, so I don't really see the point here.

src/cli/self_update/windows.rs

kellda · 2021-02-08T10:00:37Z

Done:

Fixed the typo
Enhanced tests for non Unicode strings
There is still a test to check we don't mess with non string values (e.g. binary, dword, qword)

Questions:

Should I return a result from get_path and where to handle it?
~~Do you still think I should encode the path in a OsString to pass it around ?~~
Is there anything else I missed ?

rbtcollins · 2021-02-08T15:23:59Z

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?

kellda · 2021-02-08T15:33:46Z

I had thought add_to_path could avoid it

I also through so, but when I tried to see what it would look like to use OsString I realized it checks whether cargo_home is already in the path and that can't be done with OsString.

I did not find an open issue in the rust repo, but I think this is a known limitation.

rbtcollins · 2021-02-08T15:37:36Z

I think its worth opening one; this is a decent example of it being desired but impossible.

kellda · 2021-02-08T15:46:24Z

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 ?

rbtcollins · 2021-02-09T08:52:43Z

src/cli/self_update/windows.rs

    }

+    #[test]
+    fn windows_doesnt_mess_with_a_non_string_path() {


much better - thank you!

rbtcollins · 2021-02-09T08:55:36Z

tests/cli-paths.rs

+
+    #[test]
+    /// Smoke test for end-to-end code connectivity of the installer path mgmt on windows.
+    fn install_uninstall_affect_path_with_non_unicode() {


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 🤷

rbtcollins

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.

kellda · 2021-02-10T09:21:09Z

Done !

rbtcollins · 2021-02-10T09:32:27Z

src/cli/self_update/windows.rs

    _apply_new_path(new_path)
 }

+/// Encodes a utf-8 string as a null-terminated UCS-2 string in bytes


I think this comment is wrong though yes? the input isn't a utf8 string ;)

Hope it is good now, not really sure of how to describe the operation

rbtcollins · 2021-02-10T10:31:47Z

Thank you very much for adding this improvement!

Handle PATHs with non-unicodes values on Windows

7fac19d

kellda force-pushed the non-unicode-path branch from 22e0a9c to 216ecbf Compare February 5, 2021 16:01

Fix tests

f2a02c1

kellda force-pushed the non-unicode-path branch from 216ecbf to f2a02c1 Compare February 5, 2021 16:03

rbtcollins suggested changes Feb 7, 2021

View reviewed changes

kellda commented Feb 8, 2021

View reviewed changes

src/cli/self_update/windows.rs Show resolved Hide resolved

Better tests for non unicode strings

cdc3305

Return a Result from get_path instead of panicking

bdb97b9

kellda force-pushed the non-unicode-path branch from a0f495b to bdb97b9 Compare February 9, 2021 06:46

kellda requested a review from rbtcollins February 9, 2021 07:15

rbtcollins reviewed Feb 9, 2021

View reviewed changes

rbtcollins suggested changes Feb 9, 2021

View reviewed changes

rbtcollins reviewed Feb 10, 2021

View reviewed changes

Move functions about the windows registry to the windows module

0df3c7b

kellda force-pushed the non-unicode-path branch from 5cdc32c to 0df3c7b Compare February 10, 2021 09:49

rbtcollins merged commit 3ac5076 into rust-lang:master Feb 10, 2021

kellda deleted the non-unicode-path branch February 10, 2021 10:42

Conversation

kellda commented Feb 5, 2021

Uh oh!

kinnison commented Feb 7, 2021

Uh oh!

rbtcollins left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kellda commented Feb 7, 2021

Uh oh!

Uh oh!

kellda commented Feb 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rbtcollins commented Feb 8, 2021

Uh oh!

kellda commented Feb 8, 2021

Uh oh!

rbtcollins commented Feb 8, 2021

Uh oh!

kellda commented Feb 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbtcollins left a comment

Choose a reason for hiding this comment

Uh oh!

kellda commented Feb 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbtcollins commented Feb 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kellda commented Feb 8, 2021 •

edited

Loading