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

Replace some mem::zeroed uses with idiomatic/safer approaches #6

Merged
merged 1 commit into from
Nov 4, 2023

Conversation

dwattttt
Copy link
Contributor

@dwattttt dwattttt commented Oct 3, 2023

You mentioned learning Rust and making a pull requests to offer advice, so here's something: std::mem::zeroed can cause unexpected failures where the value it creates should not exist, e.g. a reference cannot ever be null, just creating one triggers UB; if a field in a struct is a reference, it is instant UB to create the struct via zeroed (see a good discussion at rust-lang/rust#52898)

Two approaches to avoid needing to use zeroed are in this PR: where a variable is being initialised to zero because it's just going to be replaced (i.e. it's an out param of an FFI call), it's preferred to use MaybeUninit to represent this. This semantically changes the operations being performed from:

  1. unsafely create a variable via zeroed, which may not uphold any invariants the type is meant to possess
  2. perform a call that initialises the variable
  3. the variable is now safe to use

into:

  1. create a variable via MaybeUninit. This is safe; the unsafe-ty now occurs when interacting with the variable (since it's not yet initialised)
  2. perform a call that initialises the variable. This is where unsafe is required. If initialisation were happening in pure Rust, it would need to happen via pointer rather than reference.
  3. declare that the variable is now safe to use, by calling assume_init. This also requires unsafe, as it's a declaration that the variable now has a valid representation for its type.

You can see this in the PR; it would not be safe to use the pi variable prior to checking that the function call to initialise it succeeded, and so it's only after checking ret != 0 that we call assume_init and can safely interact with the variable.

The other zeroed use I replaced was to derive Default rather than implement it directly. Deriving Default instructs the compiler to defer to a Default implementation for every field of the struct. For example, if GUID had an invalid representation (type bits set to an incorrect version?), using zeroed would break this assumption, whereas using Default defers to GUIDs implementation.

Rather than deriving Default you could instead call GUID::default within your implementation of Default for your type, if you're trying to avoid deriving traits. However, in case you weren't aware of derive, it instructs the compiler to provide default (hah) implementations of traits where possible. Default, Copy, Clone, PartialEq, Eq, are all examples of common traits that can be derived rather than written by hand.

Love the intersection of low level Windows & Rust, and windbg has been an almost daily tool for many years now.

@TimMisiak
Copy link
Owner

Thanks so much for your PR! Your explanation makes a lot of sense.

@TimMisiak TimMisiak merged commit 075cdd0 into TimMisiak:main Nov 4, 2023
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.

2 participants