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

fix OOB pointer formed in Vec::index #122761

Merged
merged 1 commit into from
Mar 20, 2024
+3 −4
Merged

fix OOB pointer formed in Vec::index #122761

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions library/alloc/src/vec/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1541,6 +1541,9 @@ impl<T, A: Allocator> Vec<T, A> {
}

let len = self.len();
if index > len {
Copy link
Contributor

@nnethercote nnethercote Mar 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

#98755 explains the reasons for the original change. This PR will increase the number of tests in the common index < len case from one to two, and reinstate the zero-length copy in the index == len case.

If the add is the problem, it could be moved so it's only computed after the index < len and index == len tests. Something like this:

        // Space for the new element.
        if len == self.buf.capacity() {
            self.reserve(1);
        }

        unsafe {
            // Do not call `add` until the index is checked, to avoid UB. Also,
            // minimize the number of conditional tests, and avoid a
            // zero-length copy in the `index == len` case. (All this requires
            // the duplicate computation of `p`.)
            if index < len {
                // Shift everything over to make space. (Duplicating the
                // `index`th element into two consecutive places.) Then
                // overwrite the first copy of the `index`th element.
                let p = self.as_mut_ptr().add(index);
                ptr::copy(p, p.add(1), len - index);
                ptr::write(p, element);
            } else if index == len {
                // No elements need shifting. Overwrite the first copy of the
                // `index`th element.
                let p = self.as_mut_ptr().add(index);
                ptr::write(p, element);
            } else {
                assert_failed(index, len);
            }
            self.set_len(len + 1);                                                
        }

Copy link
Contributor

Choose a reason for hiding this comment

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

That would also avoid the unnecessary extra block :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR will increase the number of tests in the common index < len case from one to two, and reinstate the zero-length copy in the index == len case.

I believe I kept the original change where the ptr::copy is only called when index < len. I thought that was the main reason for the perf speedup from the original PR, however, I can see the extra check in the common path having an impact on perf.

Should I make a new commit to fold the index > len check back to the original comparison now or wait for a perf run?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, we might get a small speedup in the sense that we no longer potentially resize the Vec if the index is OOB, though obviously it would be silly to try to optimize that at the expense of the non-panicking in-bounds case.

assert_failed(index, len);
}

// space for the new element
if len == self.buf.capacity() {
Expand All @@ -1556,10 +1559,6 @@ impl<T, A: Allocator> Vec<T, A> {
// Shift everything over to make space. (Duplicating the
// `index`th element into two consecutive places.)
ptr::copy(p, p.add(1), len - index);
} else if index == len {
// No elements need shifting.
} else {
assert_failed(index, len);
}
// Write it in, overwriting the first copy of the `index`th
// element.
Expand Down
Loading