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

Add functions for writing zeroed bytes to &mut impl Zeroable and &mut [impl Zeroable] #193

Merged
merged 4 commits into from
Sep 5, 2023

Conversation

thomcc
Copy link
Contributor

@thomcc thomcc commented Jun 5, 2023

Two reasons this functionality is worth having:

  1. Some cases in FFI want a struct required input to be cleared with memset, because they expect to add fields to the end of a struct and use sizeof(TheStruct) to indicate which fields exist on the type.

    That is: a new field could be added to the end of a struct without changing the size if the bytes of that field were previously tail padding -- in this case if all bytes were not zeroed in this manner, the uninit bytes in the field may be interpreted as having meaning.

  2. *blah = T::zeroed() will end up having a few redundant copies of T on the stack in debug builds which could cause various problems.

I didn't add these as methods on Zeroable intentionally, but don't care much about it. If you'd prefer them there or something else I'm fine with it.

I'm pretty sure I didn't miss anything in terms of correctness here, although I had initially forgotten that Zeroable didn't imply Copy, so a bit of scrutiny wouldn't be out of place to ensure I'm not adding a problematic API.

@thomcc
Copy link
Contributor Author

thomcc commented Jun 5, 2023

I decided supporting non-copy types was actually useful because sometimes code doesn't bother with a #[derive(Copy, Clone)]. Also Cell and stuff.

But a lot of them won't need dropping (so this should still be fast) and it isn't really that hard to handle the ones that do.

@thomcc
Copy link
Contributor Author

thomcc commented Jun 5, 2023

Also while this does clobber the provenance of any pointer it overwrites with zero bytes, that is true for writing ptr::null() too (as null() has no provenance).

(This is probably not news tho, I had just not realized it. Presumably this is part of why it's fine for raw pointers to have a Zeroable impl)

src/lib.rs Outdated
Comment on lines 410 to 417
unsafe {
core::ptr::drop_in_place(target);
core::ptr::write_bytes(
target as *mut T as *mut u8,
0u8,
core::mem::size_of::<T>(),
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This will lead to a double-drop if drop_in_place panics. You should probably call write_byte in a drop guard instead.

Copy link
Owner

Choose a reason for hiding this comment

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

This is true. Alternately, do we want to require Copy after all?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any reason to limit to Copy types if there is a zero-cost and correct way to support all types

Copy link
Owner

Choose a reason for hiding this comment

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

Alright, just checking i hadn't forgotten anything i guess

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmm, lets just make these require Copy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, sure, we can do it in the drop guard.

src/lib.rs Outdated Show resolved Hide resolved
Co-authored-by: Alphyr <47725341+a1phyr@users.noreply.github.com>
@Lokathor Lokathor added the semver-minor semver minor change label Sep 5, 2023
@Lokathor Lokathor merged commit d790c04 into Lokathor:main Sep 5, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor semver minor change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants