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

Bad use of mem::uninitialized in constructor #12

Closed
pnkfelix opened this issue Mar 5, 2019 · 2 comments
Closed

Bad use of mem::uninitialized in constructor #12

pnkfelix opened this issue Mar 5, 2019 · 2 comments

Comments

@pnkfelix
Copy link

pnkfelix commented Mar 5, 2019

Spawned from rust-lang/rust#58684

The following test is likely to fail in the current code base, at least for many versions of rustc.

    #[test]
    fn test_option_encoding() {
        let tester: ArrayDeque<[Box<()>; 100], Wrapping> = ArrayDeque::new();
        assert!(Some(tester).is_some());
    }

The reason it will fail is because ArrayDeque::new() is using mem::uninitialized to create the array itself, which is discouraged because the resulting value will tend to yield undefined behavior unless the type is valid for all possible bit patterns. (And an easy example of a type that is not valid for all possible bit patterns is Box<T>, since it must be non-null.)

The test above is demonstrating how violating this rule can break invariants like Some(E).is_some() for any E.

  • One potential fix for this has been pointed out by oli: Use an untagged union instead of mem::uninitialized.
  • Another potential fix would be to use MaybeUninit instead of ManuallyDrop, as pointed out by nagisa.

Neither of these fixes is available in stable Rust today, though. (untagged unions only support Copy data if you're on stable.)

@pnkfelix
Copy link
Author

pnkfelix commented Mar 5, 2019

Since this library was inspired by bluss/arrayvec, see also bluss/arrayvec#105 and bluss/arrayvec#118, which discusses similar issues, and PRs bluss/arrayvec#114 and bluss/arrayvec#116, which demonstrate the changes that library has adopted in attempting to resolve those problems.

@andylokandy
Copy link
Owner

andylokandy commented Mar 6, 2019

Thanks for bringing this up! And arrayvec shows me a good trick to solve it.

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

No branches or pull requests

2 participants