Skip to content
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

Should windows_registry::Key::(get|set)_string use OsStr instead of str? #3119

Closed
rami3l opened this issue Jun 21, 2024 · 5 comments · Fixed by #3133
Closed

Should windows_registry::Key::(get|set)_string use OsStr instead of str? #3119

rami3l opened this issue Jun 21, 2024 · 5 comments · Fixed by #3133
Labels
enhancement New feature or request

Comments

@rami3l
Copy link

rami3l commented Jun 21, 2024

Hi, and thanks for making this project!

Background

Coming from rust-lang/rustup#3896, we (the Rustup team) are currently evaluating the possibility of replacing winreg with windows-registry in Rustup. This is mainly because we use the registry APIs to inspect and change the PATH variable, which is a necessary step in Rustup's installation process.

However, the following function signatures look a bit concerning:

pub fn get_string<T: AsRef<str>>(&self, name: T) -> Result<String> {

pub fn set_string<T: AsRef<str>>(&self, name: T, value: T) -> Result<()> {

... which is because being valid UTF8 is a core invariant of the str type. OTOH, it seems to me that there's no guarantee that we should always get/set valid UTF16LE from/to the registry, and we'd like to make sure that Rustup is still working correctly even if there are non-Unicode bytes in the registry value in question (which could be added by a third-party app?).

For example, we now have the following smoke test that inserts into the registry a sequence of u16 that would be considered malformed UTF16LE, and it's expected to work:

                // 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", &reg_value)
                    .unwrap();

https://github.com/rust-lang/rustup/blob/4c3ff9ce638e8402fbc2e8755d9e8a2cd148f6be/tests/suite/cli_paths.rs#L427-L440

Suggestion

For the exact reason explained above, I believe OsStr/OsString is a better alternative to str/String in APIs like this.

Quoting https://doc.rust-lang.org/std/ffi/struct.OsString.html:

The need for this type arises from the fact that:

  • On Windows, strings are often arbitrary sequences of non-zero 16-bit values, interpreted as UTF-16 when it is valid to do so.

Once that change is made, we would be able to use windows::ffi::OsStringExt::from_wide to construct an OsString from any &[u16].

I'm still not very familiar with Windows development myself, so please feel free to correct me if I'm wrong about anything.
Many thanks in advance!

@rami3l rami3l added the bug Something isn't working label Jun 21, 2024
@kennykerr
Copy link
Collaborator

How about this: #3120

It allows you to gracefully handle registry values even when their byte values are not necessarily valid or corresponding to their purported type. You can thus query a "string" value as bytes and interpret it however you wish.

Let me know what you think.

@kennykerr kennykerr added enhancement New feature or request and removed bug Something isn't working labels Jun 21, 2024
@rami3l
Copy link
Author

rami3l commented Jun 21, 2024

How about this: #3120

It allows you to gracefully handle registry values even when their byte values are not necessarily valid or corresponding to their purported type. You can thus query a "string" value as bytes and interpret it however you wish.

Let me know what you think.

@kennykerr Thanks for the quick response! After a quick look it seems that this only solves half the problem though: since we're modifying the PATH, we have to find a way to put the value back after appending to it...

I still personally prefer the OsStr approach, as it's quite clear that:

  • It's intended to be used as a string-like value;
  • When we get such a value, converting it to String might very well fail due to encoding problems. That error is better handled on our side rather than by the library.

@kennykerr
Copy link
Collaborator

How about if we added get_os_string and set_os_string? That way folks that need no_std support or just prefer str can still use windows-registry and folks that prefer/need OsStr can opt for that as well.

@rami3l
Copy link
Author

rami3l commented Jun 21, 2024

How about if we added get_os_string and set_os_string? That way folks that need no_std support or just prefer str can still use windows-registry and folks that prefer/need OsStr can opt for that as well.

@kennykerr Sounds good!

@kennykerr
Copy link
Collaborator

#3121 adds OsString support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants