Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

panic safety fix: guard against double drop #10

Merged
merged 1 commit into from
Jan 11, 2021
Merged

panic safety fix: guard against double drop #10

merged 1 commit into from
Jan 11, 2021

Conversation

JOE1994
Copy link
Contributor

@JOE1994 JOE1994 commented Jan 11, 2021

Hello 🦀 ,
we (Rust group @sslab-gatech) found a memory-safety/soundness issue in this crate while scanning Rust code on crates.io for potential vulnerabilities. This PR contains a fix for the issue.

Issue Description

glsl-layout/src/array.rs

Lines 177 to 199 in 119f5ec

fn map_array(mut values: [T; $size], mut f: F) -> Self {
use std::{
mem::forget,
ptr::{read, write},
};
unsafe {
// All elements of `result` is written.
// Each element of `values` read once and then forgotten.
// Hence safe in case `f` never panics.
// TODO: Make it panic-safe.
let mut result: ::std::mem::MaybeUninit<[U; $size]> =
::std::mem::MaybeUninit::zeroed();
for i in 0..$size {
write(
result.as_mut_ptr().cast::<U>().add(i),
f(read(&mut values[i])),
);
}
forget(values);
result.assume_init()
}
}

As the comments in the code indicates, the code above needs a patch for panic safety.
If a panic happens within f,
the item copied by ptr::read() can be dropped twice since the ownership of the item is duplicated.

Suggested Fix

I used ManuallyDrop<_> to contain values, in order to guard against double drop.

Thank you for reviewing this PR 👍


This change is Reviewable

@JOE1994
Copy link
Contributor Author

JOE1994 commented Jan 11, 2021

p.s. the same issue exists for the code that matches the latest crates.io release (v0.3.2)

@zakarumych
Copy link
Collaborator

Thanks. This looks exactly how it should be done.
It is possible to add a guard to drop values not yet read. But leaks are safe so it's not mandatory.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants