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.

Sign up for GitHub

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

Adding a zeroize function for flat types #1045

Merged
merged 13 commits into from
Jan 31, 2024
Merged

Adding a zeroize function for flat types #1045

Changes from 9 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
7b62b6c
added zeroize_flat_type()
nstilt1 Jan 21, 2024
d4999a5
restricted the method for Sized types and ammended the safety note
nstilt1 Jan 22, 2024
b42151d
fmt again :(
nstilt1 Jan 22, 2024
4801c84
revised zeroize_flat_type()
nstilt1 Jan 22, 2024
aace15e
fmt :|
nstilt1 Jan 22, 2024
8ab3c8b
removed an invalid safety comment
nstilt1 Jan 22, 2024
9961905
Revert "removed an invalid safety comment"; was confused for a moment
nstilt1 Jan 22, 2024
4524200
revised docs
nstilt1 Jan 23, 2024
2548228
removed colon from safety section :/
nstilt1 Jan 23, 2024
9fdd1cd
revised safety docs and added examples
nstilt1 Jan 24, 2024
dc91cc2
add no_run for the bad example
nstilt1 Jan 24, 2024
92d89e3
revised docs some more
nstilt1 Jan 24, 2024
2ff71ab
typo
nstilt1 Jan 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions zeroize/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -795,6 +795,25 @@ unsafe fn volatile_set<T: Copy + Sized>(dst: *mut T, src: T, count: usize) {
}
}

/// Zeroizes a flat type/struct. Only zeroizes the values that it owns, and it does not work on
/// dynamically sized values or trait objects. Do not use this function on a type that already
/// implements `ZeroizeOnDrop`.
///
/// # Safety
/// - The type must not contain references to outside data or dynamically sized data
/// - This function can invalidate the type if it is used after this function is called on it. It is
/// advisable to call this method in `impl Drop`.
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be good to add something here about requiring that the bit pattern of all zeros must be valid for the type

Copy link
Member

@newpavlov newpavlov Jan 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I do not think this requirement is needed for this function. We just need to mention that the function may leave the memory in a state invalid for type T.

In other words, I think (but not 100% sure) that the following code is safe:

#[repr(transparent)]
struct Foo(NonZeroU32);

impl Drop for Foo {
    fn drop(&mut self) {
        unsafe { zeroize_flat_type(self); }
    }
}

Copy link
Member

Choose a reason for hiding this comment

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

I'm not really clear on if that's allowed or not: rust-lang/unsafe-code-guidelines#394

Copy link
Member

Choose a reason for hiding this comment

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

Notably it seems like it would be UB with drop_in_place or ManuallyDrop::drop which both permit access to the value after running the Drop impl

Copy link
Member

Choose a reason for hiding this comment

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

Both drop_in_place and ManuallyDrop::drop are unsafe, so it should not be a problem. Per ManuallyDrop::drop docs:

However, this “zombie” value should not be exposed to safe code, and this function should not be called more than once. To use a value after it’s been dropped, or drop a value multiple times, can cause Undefined Behavior (depending on what drop does).

For now, we probably should write about validity (with NonZeroU32 as a counter-example), but mention that zeroized state is allowed to break safety invariants of the type.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, that sounds good

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. Feel free to make any changes

#[inline(always)]
pub unsafe fn zeroize_flat_type<T: Sized>(data: *mut T) {
let size = mem::size_of::<T>();
// Safety:
//
// This is safe because `mem::size_of<T>()` returns the exact size of the object in memory, and
// `data_ptr` points directly to the first byte of the data.
volatile_set(data as *mut u8, 0, size);
atomic_fence()
}

/// Internal module used as support for `AssertZeroizeOnDrop`.
#[doc(hidden)]
pub mod __internal {
Expand Down