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

Avoid use of mem::uninitialized #1

Closed
bjorn3 opened this issue Jun 25, 2019 · 7 comments
Closed

Avoid use of mem::uninitialized #1

bjorn3 opened this issue Jun 25, 2019 · 7 comments
Assignees
Labels
bug Something isn't working

Comments

@bjorn3
Copy link

bjorn3 commented Jun 25, 2019

https://github.com/maplant/aljabar/blob/4c60bb838f2480b4a454703e7f1f278a1cc748c3/src/lib.rs#L224-L225

You should use MaybeUninit instead. Calling mem::uninitialized is undefined behavior for uninhabited types like ! of enum Void {}. It is also undefined behavior for bool, as the uninitialized content may have a different value than 0 or 1, which are the only valid values for a bool. Also mem::uninitialized is deprecated in favor of MaybeUninit see docs.

@maplant
Copy link
Owner

maplant commented Jun 25, 2019

I tried using MaybeUninit initially; you can see me commenting out the feature attribute after giving up.
Unfortunately it does not help in this use case. Items still need to be moved out of the arrays individually. MaybeUninit needs to implement a from_iter and drain method before it can be used here easily.

@maplant
Copy link
Owner

maplant commented Jun 26, 2019

Actually, I think I can make it work. But counter intuitively it's going to require more unsafe than otherwise.

@maplant
Copy link
Owner

maplant commented Jun 27, 2019

Hmmm... another roadblock run into, it's impossible to do the following:

let mut rhs: [MaybeUninit<B>; N] = unsafe { mem::transmute::<[B; N], [MaybeUninit<B>; N]>(x) };

@maplant maplant added blocked bug Something isn't working labels Jun 27, 2019
@maplant maplant self-assigned this Jun 27, 2019
@maplant
Copy link
Owner

maplant commented Jun 27, 2019

This issue is blocked on rust-lang/rust#61956

@jswrenn
Copy link

jswrenn commented Jun 27, 2019

I poked at this last night, and I think pointer arithmetic provides a sound work-around:

#[repr(transparent)]
pub struct Vector<T, const N: usize>([T; N]);

impl<T, const N: usize> Zero for Vector<T, {N}>
where
    T: Zero,
{
    fn zero() -> Self {
        let mut origin = mem::MaybeUninit::<Vector<T, {N}>>::uninit();
        let start: *mut T = unsafe { mem::transmute(&mut origin) };

        for i in 0..N {
            unsafe { start.offset(i as isize).write(<T as Zero>::zero()) }
        }

        unsafe { origin.assume_init() }
    }

    /* ... */
}

My understanding is: although it is unsound to create an uninitialized value, it is nonetheless sound to create raw pointers to an uninitialized value (so long as we never dereference the pointer).

I'm not an expert at this, though. Does this approach seem reasonable?

@maplant
Copy link
Owner

maplant commented Jun 27, 2019

@jswrenn Indeed, this approach is safe. Mutable pointers don't drop what they point to unless you call drop_in_place. Thanks for the approach! I'll work on adding this.

I think I would prefer something closer to:

fn zero() -> Self {
    let mut origin = mem::MaybeUninit::<Vector<T, {N}>>::uninit();
    let mut start: *mut T = unsafe { mem::transmute(&mut origin) };

    for _ in 0..N {
        start = unsafe {
                start.write(<T as Zero>::zero());
                start.offset(1)
        };
    }
    unsafe { origin.assume_init() }
}

I'm surprised by the line:

let start: *mut T = unsafe { mem::transmute(&mut origin) };

Didn't realize that was even possible.

@maplant maplant removed the blocked label Jun 27, 2019
maplant added a commit that referenced this issue Jun 28, 2019
Closes issue #1
Thanks to @jswrenn for the suggestion!
@maplant
Copy link
Owner

maplant commented Jun 28, 2019

This issue has been fixed! Aljabar is now "safe"

@maplant maplant closed this as completed Jun 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants