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

New lint: mem::replace(mem::uninitialized) is dangerous #4485

Closed
Shnatsel opened this issue Sep 1, 2019 · 3 comments · Fixed by #4511
Closed

New lint: mem::replace(mem::uninitialized) is dangerous #4485

Shnatsel opened this issue Sep 1, 2019 · 3 comments · Fixed by #4511
Labels
A-lint Area: New lints L-correctness Lint: Belongs in the correctness lint group

Comments

@Shnatsel
Copy link
Member

Shnatsel commented Sep 1, 2019

When working with a &mut T sometimes you need to turn it into T. This can be achieved using mem::replace():

fn myfunc (v: &mut Vec<i32>) {
    let taken_v = std::mem::replace(&mut v, mem::uninitialized());
    let new_v = do_stuff(taken_v);
    let uninit = std::mem::replace(&mut v, new_v);
    std::mem::forget(uninit);
}

People generally assume that this is safe because the uninitialized memory is never observed. However, this code will drop uninitialized memory if do_stuff() panics, because mem::forget will never be reached. This is undefined behavior and potentially a security vulnerability. Example of real-world code with this issue: sile/libflate#35

This can be fixed by using the take_mut crate as described here. AFAIK there is currently no stdlib solution.

Using mem::zeroed() instead of mem::uninitialized() is also not valid if the target type is not valid for zeroed bit pattern or in case the type is generic.

cc @RalfJung

@flip1995 flip1995 added L-correctness Lint: Belongs in the correctness lint group A-lint Area: New lints labels Sep 2, 2019
@llogiq
Copy link
Contributor

llogiq commented Sep 5, 2019

mem::uninitializedis going to be deprecated, so will we need that lint then, too?

@Shnatsel
Copy link
Member Author

Shnatsel commented Sep 5, 2019

There is no migration path from this to a safe usage of MaybeUninit. The only thing compiler warnings can do is prod people to convert this to MaybeUninit followed by .assume_init() immediately, which is just as unsafe as the current pattern. So pointing out the unsoundness and suggesting the proper solution is still valuable.

Also, the case with mem::zeroed() and a generic parameter still applies even without use of uninitialized memory.

@RalfJung
Copy link
Member

AFAIK there is currently no stdlib solution.

True, but it has been proposed to basically add take_mut: rust-lang/rfcs#1736.
It was rejected because it has to abort-on-panic and "having a function in core::mem abort the process seems too surprising to stabilize".

flip1995 added a commit to flip1995/rust-clippy that referenced this issue Sep 19, 2019
…lip1995

New lint: mem_replace_with_uninit

changelog: add `mem_replace_uninit` lint

This fixes rust-lang#4485
bors added a commit that referenced this issue Sep 19, 2019
New lint: mem_replace_with_uninit

changelog: add `mem_replace_uninit` lint

This fixes #4485
flip1995 added a commit to flip1995/rust-clippy that referenced this issue Sep 19, 2019
…lip1995

New lint: mem_replace_with_uninit

changelog: add `mem_replace_uninit` lint

This fixes rust-lang#4485
bors added a commit that referenced this issue Sep 19, 2019
New lint: mem_replace_with_uninit

changelog: add `mem_replace_uninit` lint

This fixes #4485
@bors bors closed this as completed in ca9e4ad Sep 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints L-correctness Lint: Belongs in the correctness lint group
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants