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

RFC for Vec::append_from_within() #2714

Open
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@Shnatsel
Copy link

commented Jun 21, 2019

Rendered

This addition is quite trivial. However, in the pre-RFC I've learned that there are many approaches to solving this, so I'm opening an RFC to provide a clear rationale and get some input on the final design.

Prototype implementation in playground

Shnatsel added some commits Jun 21, 2019

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

This feels very ad-hoc. I wonder if exposing some intermediate building block would help? Something like:

impl<T> Vec<T> {
    /// Returns the result of `self.as_mut_slice()` together with the rest of the capacity.
    ///
    /// The two slices have length `self.len()` and `self.capacity() - self.len()` respectively.
    pub fn with_uninit_capacity(&mut self) -> (&mut [T], &mut [MaybeUninit<T>]) {…}
}

impl<T> MaybeUninit<T> {
    /// Panic if the two slices have different lengths.
    pub fn copy_from_slice(this: &mut [Self], other: &[T]) where T: Copy {…}
    pub fn clone_from_slice(this: &mut [Self], other: &[T]) where T: Clone {…}
}

Then, append_from_within could be implemented outside of the standard library with the only unsafe code being a call to Vec::set_len.

@Shnatsel

This comment has been minimized.

Copy link
Author

commented Jun 21, 2019

I do not believe the MaybeUninit solution is optimal. For one, MaybeUninit::copy_from_slice actually guarantees that the buffer is initialized after its completion, but this is not reflected in the type system. Which is why this still requires an unsafe set_len() operation on the vector. Also, performance characteristics of this solution when used in a loop are exactly the same as for append_from_within. Finally, two similar but entirely safe solutions are already discussed in detail in "Alternatives" section.

Note that the desired end result in all three motivating examples is actually repeat_tail(&mut self, elements: usize, times: usize), while append_from_within() intended as a minimum viable safe building block for implementing the former.

While a hypothetical more general solutions could serve more use cases (see the pre-RFC for a motivating example from Claxon for a more general abstraction), it would take a long time to design and implement, and it would not entirely supersede append_from_within(). This function is more discoverable and easier to apply than a generic abstraction by virtue of being much simpler and also by being the counterpart of an existing method on slice. And in this case having a low cognitive load is important: as illustrated by the motivating examples, people are repeatedly tempted to hand-roll this and keep getting it wrong.

@Ixrec

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2019

Could we add a self-contained description of why this method is so difficult to implement correctly? Two of the three example links didn't seem to ever describe the bug, and the third explained only at the end of a very long post about a bunch of other security stuff that the broken code forgot to check if an argument was 0 (which imo is a perfectly good answer, if that is correct, it just needs to be easier to find).

@Shnatsel

This comment has been minimized.

Copy link
Author

commented Jun 22, 2019

The code in rust stdlib was not checking for overflow in capacity calculation. Here's the fix.

DEFLATE decoders forgot to check that one of the arguments is not 0. Here is a simplified version of the code they used that illustrates the problem:

/// Repeats `repeating_fragment_len` bytes at the end of the buffer
/// until an additional `num_bytes_to_fill` in the buffer is filled
pub fn decode_rle_vulnerable(buffer: &mut Vec<u8>, repeating_fragment_len: usize, num_bytes_to_fill: usize) {
    buffer.reserve(num_bytes_to_fill); // allocate required memory immediately, it's faster this way
    unsafe { // set length of the buffer up front so we can set elements in a slice instead of pushing, which is slow
        buffer.set_len(buffer.len() + num_bytes_to_fill);
    }
    for i in (buffer.len() - num_bytes_to_fill)..buffer.len() {
        self.buffer[i] = self.buffer[i - repeating_fragment_len];
    }
}

If you pass repeating_fragment_len set to 0 it will expose contents of uninitialized memory in the output. Both inflate and libflate crates have this bug even though they were implemented independently.

@Ixrec

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2019

Thanks, that makes perfect sense now.

@burdges

This comment has been minimized.

Copy link

commented Jun 22, 2019

At present, you'll need separate append_copies_from_within and append_clones_from_within, because the copy version require only Vec::set_len, while the Clone versions requires first Vec::as_mut_ptr and then later Vec::set_len.

In the simple versions, If Vec<(F,Arc<T>)> panics mid-way, due to F::Clone panicing, then the Arc<T> gets left with an incorrect ref count, which sounds okay but annoying. If otoh Mutex<Vec<F>> panics then PoisonError permits access while the Vec<F> contains uninitialized data. You could do only one Vec::set_len at the end in the Clone version, but doing them after each Clone works too.

If you wait for specialization, then presumably append_from_within can be specialized for Copy types, so I'd kinda suggest waiting for specialization.

Oops! Just noticed you want this restricted to Copy types throughout.

@Shnatsel

This comment has been minimized.

Copy link
Author

commented Jun 22, 2019

I was using slice::copy_within() as reference, which is restricted to Copy types and does not come with an equivalent function for Clone types.

I am open to adding append_clones_from_within() if there are use cases for it.

@scottmcm

This comment has been minimized.

Copy link
Member

commented Jun 23, 2019

On copy-vs-clone, I think Copy is fine for now. It could be expanded compatibly to Clone in the future if desired, especially once we eventually get specialization and could thus promise that it's just a memcpy when the input is Copy.

I was using slice::copy_within() as reference

I like this plan -- when I saw it get stabilized in 1.37 I also thought of this scenario, as it implicitly set a precedent for API design for things that treat part of themselves as input.

(Now that it's simplified so much and not blazing its own trail -- something like .as_fixed_capacity() was a much bigger addition -- I'd personally consider it small enough to just be a PR instead of an RFC, though of course I'm not on libs so my opinion isn't all that important here.)

Shnatsel added some commits Jun 23, 2019

@Shnatsel Shnatsel referenced this pull request Jun 23, 2019

Open

Canvas unsafe code in the wild #146

0 of 9 tasks complete
@Shnatsel

This comment has been minimized.

Copy link
Author

commented Jun 29, 2019

@WanzenBug and me have ported libflate to a 100% safe RLE implementation using this abstraction. That let us remove an unsound bespoke unsafe block.

It also improved end-to-end performance by 5% 10% over the unsafe, unsound implementation, and by much more over the best safe implementation possible without append_from_within().

Add one-sentence description of the function
to guide-level explanation, as requested in a comment
@XAMPPRocky

This comment has been minimized.

Copy link

commented Jul 11, 2019

From reading the RFC I found that Vec::repeat_part was a more clear description and API for the desired operation than append_from_within despite being more generic. That coupled with the potential to allow for eliding the checks makes it more compelling to me than append_from_within.

bikeshed I think switching the position of the parameters of repeat_part would better match the name. e.g. repeat_part(&mut self, times: usize, src: Range) as you first specify how may times you're going to repeat and then what part you want to repeat.

@comex

This comment has been minimized.

Copy link

commented Jul 11, 2019

I don't think the name repeat_part is clear at all; it sounds like it might return a repeated value without modifying the Vec, or perhaps replace the part with repeated copies of itself (as opposed to appending those copies at the end).

@XAMPPRocky

This comment has been minimized.

Copy link

commented Jul 12, 2019

So I went through Vec's current methods to see if there was existing terminology that would make more sense, and I think the extend terminology might suit this better. E.g. append_from_within -> extend_from_within, repeat_part -> extend_from_part.

@Shnatsel Shnatsel referenced this pull request Jul 21, 2019

Open

Audit libflate #1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.