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

Fixed unsoundness of using &mut MaybeUninit<T> as the out type for … #1

Merged
merged 5 commits into from
Feb 29, 2020

Conversation

danielhenrymantilla
Copy link
Owner

@danielhenrymantilla danielhenrymantilla commented Jan 30, 2020

This (WIP) PR objective is to fix the issues spotted by @Kixunil (thanks for telling me!):

  • Minor: using transmute on fat references makes incorrect assumptions about their layout, so that code could break in a future version of Rust.

    • Fixed by using slice::from_raw_parts{,_mut} instead.
  • MAJOR: The API used &mut MaybeUninit<_> as its &out reference type, assuming that it was sound to transmute between &mut T and &mut MaybeUninit<T>, which it is not (see Document that casting &mut T to &mut MaybeUninit<T> is not safe rust-lang/rust#66699)

    • Fixed by using a custom abstraction over &out references that does not let writing MaybeUninit::uninit() into the pointee.
  • Added some API features, such as being able to forge a &out T from &mut ManuallyDrop<T> (to offer &out refs even when T : !Copy and could thus have drop glue (although leaking is safe, I take an opinionated stance here that so doing ought to be visible and thus verbose)

@HeroicKatora do you wanna review this?

TODO:

  • Document new methods

  • Add extra helpers to Out references

@danielhenrymantilla danielhenrymantilla added the wip Work in progress label Jan 30, 2020
src/out_references.rs Outdated Show resolved Hide resolved
src/extension_traits/vec.rs Outdated Show resolved Hide resolved
src/extension_traits/vec.rs Outdated Show resolved Hide resolved
src/out_references.rs Outdated Show resolved Hide resolved
src/out_references.rs Outdated Show resolved Hide resolved
src/out_references.rs Outdated Show resolved Hide resolved
src/out_references.rs Outdated Show resolved Hide resolved
src/read/impls.rs Outdated Show resolved Hide resolved
src/read/impls.rs Outdated Show resolved Hide resolved
src/read/mod.rs Outdated Show resolved Hide resolved
It features:

  - enhancing `Vec::reserve_uninit` to a more general API for accessing with the backing buffer / capacity of a Vec;

  - Rename `idx` to `.get_out`

  - (Ab)use `.copy_from_slice()` in read implementations to use less unsafe

TODO:

  - The iterator API,

  - Documentation

Co-Authored-By: Andreas Molzer <[email protected]>
Copy link
Owner Author

@danielhenrymantilla danielhenrymantilla left a comment

Choose a reason for hiding this comment

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

Some TODOs

src/extension_traits/vec.rs Outdated Show resolved Hide resolved
src/extension_traits/vec.rs Outdated Show resolved Hide resolved
src/extension_traits/vec.rs Outdated Show resolved Hide resolved
src/extension_traits/vec.rs Show resolved Hide resolved
Copy link
Owner Author

@danielhenrymantilla danielhenrymantilla left a comment

Choose a reason for hiding this comment

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

Typos

README.md Outdated Show resolved Hide resolved
src/extension_traits/vec.rs Outdated Show resolved Hide resolved
Not only does this lead to replacing `OutSlice<T>` with the more readable
`Out<[T]>`, it also results in other parts of the API being greatly simplified
(mainly the `AsOut` trait).
Copy link
Contributor

@HeroicKatora HeroicKatora left a comment

Choose a reason for hiding this comment

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

Review, round two. The interface with Out works great! The core of it came together very well. There are a few nits, documentation and naming, but no direct unsoundess that I could find.

src/extension_traits/vec.rs Outdated Show resolved Hide resolved
src/extension_traits/vec.rs Show resolved Hide resolved
src/out_ref.rs Show resolved Hide resolved
src/out_ref.rs Show resolved Hide resolved
src/out_ref.rs Outdated Show resolved Hide resolved
src/out_ref.rs Show resolved Hide resolved
src/out_ref.rs Show resolved Hide resolved
src/out_ref.rs Show resolved Hide resolved
src/extension_traits/vec.rs Outdated Show resolved Hide resolved
src/extension_traits/vec.rs Outdated Show resolved Hide resolved
@danielhenrymantilla danielhenrymantilla added enhancement New feature or request and removed wip Work in progress labels Feb 29, 2020
@danielhenrymantilla danielhenrymantilla merged commit f884ca3 into master Feb 29, 2020
@danielhenrymantilla danielhenrymantilla deleted the introduce_out_refs branch March 1, 2020 00:45
@danielhenrymantilla danielhenrymantilla restored the introduce_out_refs branch September 29, 2020 21:01
@danielhenrymantilla danielhenrymantilla deleted the introduce_out_refs branch September 30, 2020 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants