Skip to content

Add unsafe test to check that dropped values are zeroized #1180

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dvdplm
Copy link

@dvdplm dvdplm commented May 5, 2025

Test that zeroization actually occurs when a boxed value goes out of scope.

Uses unsafe code, likely relies on UB and adds a custom allocator, hence the draft: is something like this acceptable?

c.f. iqlusioninc/crates#1310

bytes
);
}
// Check that the memory is cleared after the scope ends
Copy link
Member

Choose a reason for hiding this comment

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

You can use drop_in_place to invoke the drop handler (see other tests in the same module)

Copy link
Author

Choose a reason for hiding this comment

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

I saw those tests but felt that, given how magic drop_in_place is, readers would likely be mroe familiar with a plain old scope ending. Do you disagree?

@dvdplm
Copy link
Author

dvdplm commented May 7, 2025

I guess the original question still stands: is something like this acceptable? I'm on the fence myself, hence the draft.

@newpavlov
Copy link
Member

Personally, while I am fine with having such test, I don't see much worth in it. I will leave the final decision to @tarcieri.

As a small suggestion, it may be worth to slightly modify how the test is implemented. Instead of reading leaked memory in the test itself, it may be better to allocate a known size (e.g. 1024 bytes) using SecretBox and then in dealloc check that deallocated memory with such size should be filled with zeros.

@dvdplm dvdplm marked this pull request as ready for review May 16, 2025 14:20
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.

3 participants