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

Vec::drain is unsound with range_bounds_assert_len feature #81157

Closed
KamilaBorowska opened this issue Jan 18, 2021 · 2 comments · Fixed by #81154
Closed

Vec::drain is unsound with range_bounds_assert_len feature #81157

KamilaBorowska opened this issue Jan 18, 2021 · 2 comments · Fixed by #81154
Labels
A-collections Area: `std::collection` C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness requires-nightly This issue requires a nightly compiler in some way. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@KamilaBorowska
Copy link
Contributor

KamilaBorowska commented Jan 18, 2021

Manually implementing assert_len can cause unsoundness in Vec::drain. This only happens in nightly, as overriding assert_len is only possible in nightly releases.

The following program will segfault.

#![feature(range_bounds_assert_len)]

use std::cell::Cell;
use std::ops::{Bound, Range, RangeBounds};

struct EvilRange(Cell<bool>);

impl RangeBounds<usize> for EvilRange {
    fn start_bound(&self) -> Bound<&usize> {
        unimplemented!()
    }
    fn end_bound(&self) -> Bound<&usize> {
        unimplemented!()
    }
    fn assert_len(self, _len: usize) -> Range<usize> {
        0..42
    }
}

fn main() {
    vec![1, 2, 3].drain(EvilRange(Cell::new(false)));
}

Related issues: #76393, #81154.

@KamilaBorowska KamilaBorowska added the C-bug Category: This is a bug. label Jan 18, 2021
@jonas-schievink jonas-schievink added A-collections Area: `std::collection` I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jan 18, 2021
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jan 18, 2021
@lcnr lcnr added requires-nightly This issue requires a nightly compiler in some way. and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jan 18, 2021
@dylni
Copy link
Contributor

dylni commented Jan 18, 2021

Thanks for noticing this. I think the best fix is to define the method on Range instead. I'll update this PR with that change unless anyone has a better idea.

@RalfJung
Copy link
Member

Good catch!

JohnTitor added a commit to JohnTitor/rust that referenced this issue Feb 11, 2021
… r=KodrAus

Improve design of `assert_len`

It was discussed in the [tracking issue](rust-lang#76393 (comment)) that `assert_len`'s name and usage are confusing. This PR improves them based on a suggestion by `@scottmcm` in that issue.

I also improved the documentation to make it clearer when you might want to use this method.

Old example:

```rust
let range = range.assert_len(slice.len());
```

New example:

```rust
let range = range.ensure_subset_of(..slice.len());
```

Fixes rust-lang#81157
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Feb 23, 2021
… r=KodrAus

Improve design of `assert_len`

It was discussed in the [tracking issue](rust-lang#76393 (comment)) that `assert_len`'s name and usage are confusing. This PR improves them based on a suggestion by `@scottmcm` in that issue.

I also improved the documentation to make it clearer when you might want to use this method.

Old example:

```rust
let range = range.assert_len(slice.len());
```

New example:

```rust
let range = range.ensure_subset_of(..slice.len());
```

Fixes rust-lang#81157
@bors bors closed this as completed in 72e6d51 Feb 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-collections Area: `std::collection` C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness requires-nightly This issue requires a nightly compiler in some way. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants