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

Box<[T]>::into_raw is useless #36284

Closed
vinipsmaker opened this issue Sep 5, 2016 · 19 comments
Closed

Box<[T]>::into_raw is useless #36284

vinipsmaker opened this issue Sep 5, 2016 · 19 comments
Labels
A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. P-low Low priority T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@vinipsmaker
Copy link

Sorry about the horrible title, but I really don't know how to else "summarize" this problem.

I tried to get an answer for this several times on StackOverflow (after documentation proved useless) and now I'm coming here. Maybe it's just a matter of updating documentation to have guarantees of a behaviour that will continue to work on future versions of Rust (the main purpose of documentation in the first place) or maybe the error is me who failed to spot the useful part.

I need to expose a Rust library to C applications and I need to expose an array of size only know at runtime to a function which will own this data and later will call a Rust function to deallocate it. I have a Vec<u8> and I need to get this (*mut u8, usize) tuple out of it. Later I need to get receive this same tuple and deallocate its data.

One option is to use mem::forget + Vec::from_raw_parts, but this is far from ideal as shrink_to_fit isn't guaranteed to work and I'd also need to store capacity (the C library really shouldn't need to worry about that).

The perfect option would be to use Vec::into_boxed_slice. However, it crashed when I tried to use the pointer for the first element (slice::as_mut_ptr) to Box::from_raw, as it should. And I cannot pass the pointer return from Box::into_raw to the C function, as the memory layout is unspecified and I don't have any guarantee it'll be a pointer to the first element.

How do we fix this? Update documentation? Add some extra function to Box<[T]> like Box<[T]>::from_raw_parts(ptr: *mut T, len: usize)?

@nagisa
Copy link
Member

nagisa commented Sep 5, 2016

I have a Vec and I need to get this (*mut u8, usize) tuple out of it. Later I need to get receive this same tuple and deallocate its data.

vec.shrink_to_fit();
let ptr = vec.as_mut_ptr();
let len = vec.len();
::std::mem::forget(vec);
// <snip>
drop(Vec::from_raw_parts(ptr, len, len));

this is far from ideal as shrink_to_fit isn't guaranteed to work… The perfect option would be to use Vec::into_boxed_slice. However, it crashed when I tried to use the pointer for the first element (slice::as_mut_ptr) to Box::from_raw,

This is how you construct a fat slice box out of pointer and capacity:

let slice = slice::from_raw_parts_mut(ptr, capacity);
let boxed_slice: Box<[T]> = Box::from_raw(slice);

shrink_to_fit is exactly what Vec::into_boxed_slice does before making a boxed slice, so there’s really absolutely no difference between using that or Vec::into_boxed_slice.

And I cannot pass the pointer return from Box::into_raw to the C function, as the memory layout is unspecified and I don't have any guarantee it'll be a pointer to the first element.

You should be able to pass [T]::as_mut_ptr() to C code just fine.

@ollie27
Copy link
Member

ollie27 commented Sep 5, 2016

@nagisa shrink_to_fit doesn't guarantee that the length will equal the capacity so I think into_boxed_slice is the correct thing to use.

@nagisa
Copy link
Member

nagisa commented Sep 5, 2016

@ollie27 then we have a larger problem of invalid implementation of Vec::into_boxed_slice.

@ollie27
Copy link
Member

ollie27 commented Sep 5, 2016

See #29847.

@ustulation
Copy link

I think the problem is with documentation. shrink_to_fit from the documentation isn't guaranteed to make capacity == len. So if you loose information about capacity, from_raw_parts is an undefined behaviour (if you assume len == capacity and pass it as such). I assume the only way into_vec() on Boxed-slice will be well behaved is if it guarantees to make len == capacity but documentation says it's exactly as if we did shrink_to_fit. This seems contradictory.

@vinipsmaker
Copy link
Author

I think boxed slice is more appropriate here. We don't need to give more guarantees to Vec. We could just "fix" boxed slice.

@brson brson added A-docs I-needs-decision Issue: In need of a decision. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools labels Sep 7, 2016
vinipsmaker added a commit to vinipsmaker/safe_core that referenced this issue Sep 8, 2016
Previous implementation assumed `Vec::shrink_to_fit` would always make
Vec's length equal to its capacity. However, there was no such guarantee
and undefined behaviour could be triggered.

Currently Rust implementation assume this as well. A proper fix would
use boxed slices so we don't need to store capacity.

We may want to revisit this as the following Rust issue gets more
attention: rust-lang/rust#36284
vinipsmaker added a commit to vinipsmaker/safe_core that referenced this issue Sep 8, 2016
Previous implementation assumed `Vec::shrink_to_fit` would always make
Vec's length equal to its capacity. However, there was no such guarantee
and undefined behaviour could be triggered.

Currently Rust implementation assume this as well. A proper fix would
use boxed slices so we don't need to store capacity.

We may want to revisit this as the following Rust issue gets more
attention: rust-lang/rust#36284
@vinipsmaker
Copy link
Author

Hey guys, any update on this? Our FFI interfaces still feel weird taking a capacity member everywhere.

@ollie27
Copy link
Member

ollie27 commented Dec 7, 2016

I'm not sure what update you're waiting on. The documentation certainly needs to be improved but as far as I'm aware you can do the following now with no problems:

pub fn vec_to_ptr_len(v: Vec<u8>) -> (*mut u8, usize) {
    let mut b = v.into_boxed_slice();
    let ptr = b.as_mut_ptr();
    let len = b.len();
    std::mem::forget(b);
    (ptr, len)
}

pub unsafe fn ptr_len_to_vec(ptr: *mut u8, len: usize) -> Vec<u8> {
    Box::from_raw(std::slice::from_raw_parts_mut(ptr, len)).into_vec()
}

@steveklabnik steveklabnik removed the A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools label Mar 10, 2017
@steveklabnik steveklabnik added A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools and removed A-docs labels Mar 23, 2017
@steveklabnik
Copy link
Member

steveklabnik commented Apr 25, 2017

I would like to close out this ticket. Here's how I understand it: What's left is basically @ustulation's comment above, that is, shrink_to_fit says it does not guarantee capacity but into_boxed_slice seems like it must, yet says it's the same as shrink_to_fit. Is this accurate?

@rust-lang/libs, what do you want done here?

@bluss
Copy link
Member

bluss commented Apr 25, 2017

I think that there is lacking clarity and it needs one hard look at usable size again (if that is ok)? Rust could gain some memory utilization there, removing clown shoes, without breaking any current Vec API. It requires looking at jemalloc's size bins and working out the relevant details. The theory is that all capacities in the same bin are equivalent, and usable size gives you the largest one in the allocation's current bin.

With that in place, docs related to this can be clarified with confidence.

@alexcrichton
Copy link
Member

Taking advantage of extra space given to us by the allocator seems like it falls under #29931, in which case it sounds to me like @steveklabnik is right, we should just remove the clause from into_boxed_slice about it being equivalent to shrink_to_fit.

The fact that into_boxed_slice calls shrink_to_fit is just an implementation detail we need not worry about. If we change the implementation of Vec to take advantage of excess capacity then we'll fix that implementation as well.

@bluss
Copy link
Member

bluss commented May 1, 2017

That sounds good!

@Mark-Simulacrum Mark-Simulacrum added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jul 26, 2017
@steveklabnik steveklabnik added the P-low Low priority label Aug 30, 2017
@QuietMisdreavus
Copy link
Member

In case anyone is willing to tackle this, the docs team wants to take this approach:

...we should just remove the clause from into_boxed_slice about it being equivalent to shrink_to_fit.

@QuietMisdreavus QuietMisdreavus added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. and removed I-needs-decision Issue: In need of a decision. labels Sep 13, 2017
@seanprashad
Copy link
Contributor

Hi there! I'm a student at Seneca College whose enrolled in an Open Source course taught by my professor and Mozillian, David Humphrey. I would love to tackle this bug for my course if it's still available and unassigned.

@steveklabnik
Copy link
Member

That'd be great! Is @QuietMisdreavus 's post on what to do clear enough?

@seanprashad
Copy link
Contributor

seanprashad commented Sep 28, 2017

@steveklabnik I would love to say yes and get on my way but I'm still a little unclear. Would I be removing documentation for into_boxed_slice being equivalent to shrink_to_fit? Any and all clarification is welcomed!!

@steveklabnik
Copy link
Member

Would I be removing documentation for into_boxed_slice being equivalent to shrink_to_fit?

Yup! To elaborate a little further, that means modifying these lines:

rust/src/liballoc/vec.rs

Lines 510 to 516 in 94a82ad

/// Note that this will drop any excess capacity. Calling this and
/// converting back to a vector with [`into_vec`] is equivalent to calling
/// [`shrink_to_fit`].
///
/// [owned slice]: ../../std/boxed/struct.Box.html
/// [`into_vec`]: ../../std/primitive.slice.html#method.into_vec
/// [`shrink_to_fit`]: #method.shrink_to_fit

The sentence about excess capacity should stay, but the second one and the link should be removed.

@seanprashad
Copy link
Contributor

Thank you so much @steveklabnik! I will have to prepare my local environment on my laptop and get to work. Once I'm done and ready to submit a PR, I will let you know!

bors added a commit that referenced this issue Oct 2, 2017
Resolves #36284 - vec.rs documentation

Removed comments associated with `[into_vec]` being equivalent to `[shrink_to_fit]` as requested.
@bors bors closed this as completed in 1ebc7bb Oct 2, 2017
@vinipsmaker
Copy link
Author

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. P-low Low priority T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests