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.

Sign up for GitHub

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

Jump to bottom

AioCb::from_boxed_slice violates mutability guarantees #788

Closed
asomers opened this issue Nov 4, 2017 · 6 comments
Closed

AioCb::from_boxed_slice violates mutability guarantees #788

asomers opened this issue Nov 4, 2017 · 6 comments
Assignees
@asomers
Labels
A-bug
Milestone
0.10.0

Comments

@asomers
Copy link
Member

asomers commented Nov 4, 2017

An Rc<Box<[u8]>> does not have interior mutability. It's supposed to be impossible to update its contents. But AioCb::from_boxed_slice current allows you to. It does it by casting a *const c_void to a *mut c_void. Technically, that's a safe operation. The only unsafe part is when you dereference the pointer. But we have to do that in an unsafe block anyway, so the compiler never alerted us to the mutability problem.

Fixing this issue is trivial. The problem is that nix's consumers may be relying on this behavior.
@asomers asomers self-assigned this Nov 4, 2017
@asomers asomers added this to the 0.10.0 milestone Nov 5, 2017
@asomers asomers mentioned this issue Nov 5, 2017
Closed
@Susurrus
Copy link
Contributor

@asomers Any progress with this? Been over a month and if this is trivial, I know you wanted the 0.10 release was supposed to happen quickly. It's only blocking on this.

@asomers
Copy link
Member Author

asomers commented Dec 13, 2017

The problem is a good solution isn't trivial. It's a question of what we want a good AioCb interface to be. Adding a new type of Keeper that safely allows mutable access is easy, but would pull in several dependent crates. I'm hesitant to do that. But if we don't do that, then we may as well remove the Rc<Box<[u8]>> type from Keeper, because it isn't very useful if it's immutable. That would force many Nix consumers to use an unsafe method to create an AioCb with a mutable buffer. So be it. But if that's going to be the primary API, then why bother keeping the Keeper structure around at all? The PhantomData type isn't very useful in many real applications. Some, but not many.

I've been grappling with this question, but there's no perfect answer. Should we just remove Keeper altogether, require unsafe methods for creating AioCbs, then create a new crate that safely wraps them for both mutable and immutable buffers?

@Susurrus
Copy link
Contributor

Why is there a problem with pulling in new dependencies? If that allows us to offer a nice and safe API, what are the technical reasons not to do so?

@asomers
Copy link
Member Author

asomers commented Dec 13, 2017

Bloat. Bloat is the only reason. BTW, the three required crates would be bytes, iovec, and byteorder.

@Susurrus
Copy link
Contributor

Bloat isn't a technical reason tho. What defines bloat and why is it bad? It's hard to make a technical decision when some of the information you're judging to make that decision isn't technical. So until I hear technical reasons, I'm inclined to say that's the best solution as I see no technical downsides to its implementation.

asomers added a commit to asomers/nix that referenced this issue Dec 21, 2017
@asomers
Disallow aio_read on an AioCb created with from_boxed_slice
3d3eff8 
Fixes nix-rust#788
asomers added a commit to asomers/nix that referenced this issue Jan 8, 2018
@asomers
Disallow aio_read on an AioCb created with from_boxed_slice
9147430 
Fixes nix-rust#788
asomers added a commit to asomers/nix that referenced this issue Jan 14, 2018
@asomers
Disallow aio_read on an AioCb created with from_boxed_slice
accaf58 
Fixes nix-rust#788
asomers added a commit to asomers/nix that referenced this issue Jan 16, 2018
@asomers
aio: use Bytes instead of Rc<[u8]>
631e3f3 
It's not actually safe to read into an `Rc<[u8]>`.  It only worked
because of a coincidental `unsafe` block.  Replace that type with
`BytesMut` from the bytes crate.  For consistency's sake, use `Bytes`
for writing too, and completely remove methods relating to `Rc<[u8]>`.
Note that the `AioCb` will actually own the `BytesMut` object.  The
caller must call `into_buffer` to get it back once the I/O is complete.

Fixes nix-rust#788
@Susurrus Susurrus added the A-bug label Jan 20, 2018
@Susurrus
Copy link
Contributor

Fixed in #820.

It looks like GitHub doesn't auto-close issues when referenced from commits, only PR bodies. Lame.

asomers added a commit to asomers/mio-aio that referenced this issue Feb 11, 2018
@asomers
use bytes::Bytes instead of Rc<Box<[u8]>>
364474b 
Turns out it's not safe to use aio_read with an Rc<Box<[u8]>>.
Instead, use Bytes and BytesMut from the bytes crate.

Add LioCb::{emplace_bytes,emplace_bytes_mut}

Fallout from nix-rust/nix#788
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@asomers asomers

Labels
A-bug
Projects
None yet
Milestone
0.10.0
Development

No branches or pull requests
2 participants
@asomers @Susurrus