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

Undefined Behavior in Vec::insert if passed an index outside of its capacity #122760

Closed
jwong101 opened this issue Mar 20, 2024 · 0 comments · Fixed by #122761
Closed

Undefined Behavior in Vec::insert if passed an index outside of its capacity #122760

jwong101 opened this issue Mar 20, 2024 · 0 comments · Fixed by #122761
Labels
C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@jwong101
Copy link
Contributor

I tried this code:

vec![].insert(usize::MAX, usize::MAX);

I expected to see this happen:
The program should panic, but not have any undefined behavior.

Instead, this happened:
Miri reports that the program triggers undefined behavior.

MIRI Backtrace

   Compiling playground v0.0.1 (/playground)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 1.51s
     Running `/playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo-miri runner target/miri/x86_64-unknown-linux-gnu/debug/playground`
error: Undefined Behavior: out-of-bounds pointer arithmetic: alloc1582 has size 32, so pointer to 8 bytes starting at offset -8 is out-of-bounds
    --> /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/vec/mod.rs:1554:25
     |
1554 |                 let p = self.as_mut_ptr().add(index);
     |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ out-of-bounds pointer arithmetic: alloc1582 has size 32, so pointer to 8 bytes starting at offset -8 is out-of-bounds
     |
     = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
     = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
help: alloc1582 was allocated here:
    --> src/main.rs:2:5
     |
2    |     vec![].insert(usize::MAX, usize::MAX);
     |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     = note: BACKTRACE (of the first span):
     = note: inside `std::vec::Vec::<usize>::insert` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/vec/mod.rs:1554:25: 1554:53
note: inside `main`
    --> src/main.rs:2:5
     |
2    |     vec![].insert(usize::MAX, usize::MAX);
     |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to 1 previous error

Meta

It seems like the issue is here:

let p = self.as_mut_ptr().add(index);
if index < len {
// 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);
}

The length check needs to happen before the computation of ptr::add, since it's undefined behavior if the new pointer is out of bounds of the allocation or overflows the address space.

rustc --version --verbose:

rustc 1.79.0-nightly (a7e4de13c 2024-03-19)
@jwong101 jwong101 added the C-bug Category: This is a bug. label Mar 20, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Mar 20, 2024
@jwong101 jwong101 changed the title Undefined Behavior in Vec::insert if passed an index outside of it's capacity Undefined Behavior in Vec::insert if passed an index outside of its capacity Mar 20, 2024
@Nilstrieb Nilstrieb added I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Mar 20, 2024
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Mar 20, 2024
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Mar 20, 2024
…ng-usize-max, r=Nilstrieb

Add `usize::MAX` arg tests for Vec

Tests to prevent recurrence of the UB from the rust-lang#122760 issue.

I skipped the `with_capacity`, `drain`, `reserve`, etc. APIs because they actually had a good assortment of tests earlier in the same file.

r? Nilstrieb
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Mar 20, 2024
…jubilee,Nilstrieb

fix OOB pointer formed in Vec::index

Move the length check to before using `index` with `ptr::add` to prevent an out of bounds pointer from being formed.

Fixes rust-lang#122760
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Mar 20, 2024
…ng-usize-max, r=Nilstrieb

Add `usize::MAX` arg tests for Vec

Tests to prevent recurrence of the UB from the rust-lang#122760 issue.

I skipped the `with_capacity`, `drain`, `reserve`, etc. APIs because they actually had a good assortment of tests earlier in the same file.

r? Nilstrieb
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Mar 20, 2024
…ng-usize-max, r=Nilstrieb

Add `usize::MAX` arg tests for Vec

Tests to prevent recurrence of the UB from the rust-lang#122760 issue.

I skipped the `with_capacity`, `drain`, `reserve`, etc. APIs because they actually had a good assortment of tests earlier in the same file.

r? Nilstrieb
@apiraino apiraino removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Mar 20, 2024
bors added a commit to rust-lang-ci/rust that referenced this issue Mar 20, 2024
…bilee,Nilstrieb

fix OOB pointer formed in Vec::index

Move the length check to before using `index` with `ptr::add` to prevent an out of bounds pointer from being formed.

Fixes rust-lang#122760
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 20, 2024
…ng-usize-max, r=Nilstrieb

Add `usize::MAX` arg tests for Vec

Tests to prevent recurrence of the UB from the rust-lang#122760 issue.

I skipped the `with_capacity`, `drain`, `reserve`, etc. APIs because they actually had a good assortment of tests earlier in the same file.

r? Nilstrieb
@bors bors closed this as completed in 37718f9 Mar 20, 2024
jhpratt added a commit to jhpratt/rust that referenced this issue Mar 21, 2024
…ng-usize-max, r=Nilstrieb

Add `usize::MAX` arg tests for Vec

Tests to prevent recurrence of the UB from the rust-lang#122760 issue.

I skipped the `with_capacity`, `drain`, `reserve`, etc. APIs because they actually had a good assortment of tests earlier in the same file.

r? Nilstrieb
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Mar 21, 2024
Rollup merge of rust-lang#122765 - workingjubilee:test-for-vec-handling-usize-max, r=Nilstrieb

Add `usize::MAX` arg tests for Vec

Tests to prevent recurrence of the UB from the rust-lang#122760 issue.

I skipped the `with_capacity`, `drain`, `reserve`, etc. APIs because they actually had a good assortment of tests earlier in the same file.

r? Nilstrieb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants