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

Be more defensive about how windows RegistryKeys are handled #739

Merged
merged 2 commits into from Oct 31, 2022

Conversation

thomcc
Copy link
Member

@thomcc thomcc commented Oct 29, 2022

Few things along a general theme of skepticism in this registry code.

  • Fixes an issue where HKEY_LOCAL_MACHINE is not sign-extended into 64 bits on 64 bit targets.

    From discussion with @ChrisDenton it's unclear why this works, perhaps internal details, or odd handle behavior with KEY_WOW64_32KEY. Dunno, easy fix (hopefully).

  • Replaces Repr::Const(...) with Repr::LocalMachine.

    According to https://learn.microsoft.com/en-us/windows/win32/api/winreg/nf-winreg-regqueryvalueexw, using this with some HKEY types (HKEY_PERFORMANCE_DATA and presumably its subkeys) requires using the API in a totally different way, and knowing exactly which type you're dealing with. If you try to use the API as you normally, it seems like you'd end up with UB.

    Since we don't care at all about HKEY_PERFORMANCE_DATA, this just makes it clearer that we should never have other Repr::Consts than HKEY_LOCAL_MACHINE.

  • Behaves a lot more defensively in RegistryKey::query_str. Specifically, it adds some asserts that the values we get are as we'd expect, and defensively zeroes the memory we allocate for the wide string.

    This is being a bit paranoid, but most of these weird behaviors seem like they could happen if we happened to have a HKEY_PERFORMANCE_DATA-derived key, and there's no easy way for us to check for that.

    These checks try to minimize how disastrous it would be if we did end up with one (panic rather than either heap buffer overflow or returning a pile of uninit memory to the caller).

  • Fixes a small oversight in a case where we'd panic the value we read happens to be the empty string. Presumably this couldn't actually happen, or we'd have gotten a bug report by now.

  • Adds some comments explaining this stuff, to make it more clear which things are done or not done intentionally.

r? @ChrisDenton

@rustbot
Copy link

rustbot commented Oct 29, 2022

Error: This repository is not enabled to use triagebot.
Add a triagebot.toml in the root of the default branch to enable it.

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

Copy link
Contributor

@ChrisDenton ChrisDenton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is maybe a bit paranoid in places but this code isn't performance sensitive code so extra checks won't hurt. I only have one small nit but nothing I feel strongly about.

src/registry.rs Outdated Show resolved Hide resolved
Co-authored-by: Chris Denton <ChrisDenton@users.noreply.github.com>
@thomcc
Copy link
Member Author

thomcc commented Oct 30, 2022

Yep makes sense to me.

@ChrisDenton ChrisDenton merged commit f914e8a into rust-lang:main Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants