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

Implement split_at_spare_mut without Deref to a slice so that the spare slice is valid #92097

Merged
merged 2 commits into from
Jan 1, 2022

Conversation

saethlin
Copy link
Member

@saethlin saethlin commented Dec 19, 2021

I'm not sure I understand what's going on here correctly. And I'm pretty sure this safety comment needs to be changed. I'm just referring to the same thing that as_mut_ptr_range does. (Thanks @RalfJung for the guidance and clearing things up)

I tried to run https://github.com/rust-lang/miri-test-libstd on alloc with -Zmiri-track-raw-pointers, and got a failure on the test vec::test_extend_from_within.

I minimized the test failure into this program:

#![feature(vec_split_at_spare)]
fn main() {
    Vec::<i32>::with_capacity(1).split_at_spare_mut();
}

The problem is that the existing implementation is actually getting a pointer range where both pointers are derived from the initialized region of the Vec's allocation, but we need the second one to be valid for the region between len and capacity. (thanks Ralf for clearing this up)

@rust-highfive
Copy link
Collaborator

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 19, 2021
@saethlin saethlin changed the title Implement split_at_spare_mut without Deref to a slice Implement split_at_spare_mut without Deref to a slice so that the spare slice is valid Dec 19, 2021
@jyn514
Copy link
Member

jyn514 commented Dec 19, 2021

cc @RalfJung

The previous implementation used slice::as_mut_ptr_range to derive the
pointer for the spare capacity slice. This is invalid, because that
pointer is derived from the initialized region, so it does not have
provenance over the uninitialized region.
@RalfJung
Copy link
Member

LGTM, but I'll wait on libs team judgment on whether they want to land this (since it only is a fix wrt a proposed model, not wrt anything "official").

@nrc nrc added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Dec 20, 2021
@saethlin
Copy link
Member Author

r? @the8472

@rust-highfive rust-highfive assigned the8472 and unassigned m-ou-se Dec 31, 2021
Copy link
Member

@the8472 the8472 left a comment

Choose a reason for hiding this comment

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

Note that I'm libs-contributor, not libs-team. That said I'm fairly familiar with Vec so I'll go ahead anyway.

Looking at the history this was intentionally (but tangentially) changed in #82564 and reviewed by @RalfJung. Since that PR was about changing spare_capacity_mut undoing the split_at_spare_mut changes should be fine.

And since Vec uses raw pointers all over the place and it's consistent with changes previously made I don't see an issue with doing it here too.

library/alloc/src/vec/mod.rs Outdated Show resolved Hide resolved
@the8472
Copy link
Member

the8472 commented Dec 31, 2021

@bors r=the8472,ralfjung rollup=always

@bors
Copy link
Contributor

bors commented Dec 31, 2021

📌 Commit 777c853 has been approved by the8472

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 31, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 1, 2022
…askrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#88310 (Lock bootstrap (x.py) build directory)
 - rust-lang#92097 (Implement split_at_spare_mut without Deref to a slice so that the spare slice is valid)
 - rust-lang#92412 (Fix double space in pretty printed TryBlock)
 - rust-lang#92420 (Fix whitespace in pretty printed PatKind::Range)
 - rust-lang#92457 (Sync rustc_codegen_gcc)
 - rust-lang#92460 ([rustc_builtin_macros] add indices to format_foreign::printf::Substitution::Escape)
 - rust-lang#92469 (Make tidy check for magic numbers that spell things)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a6e4d68 into rust-lang:master Jan 1, 2022
@rustbot rustbot added this to the 1.59.0 milestone Jan 1, 2022
saethlin added a commit to saethlin/rayon that referenced this pull request Jan 3, 2022
The uninitialized region of a Vec cannot be accessed by slicing the Vec
first, see rust-lang/rust#92097

Similarly, it is never valid to merge adjacent slices, because any
pointer derived from a slice only has provenance over that slice, not
anything adjacent. So we pass raw pointers and a length around to avoid
narrowing provenance by converting to a reference.
saethlin added a commit to saethlin/rayon that referenced this pull request Jan 3, 2022
The uninitialized region of a Vec cannot be accessed by slicing the Vec
first, see rust-lang/rust#92097

Similarly, it is never valid to merge adjacent slices, because any
pointer derived from a slice only has provenance over that slice, not
anything adjacent. So we pass raw pointers and a length around to avoid
narrowing provenance by converting to a reference.
cuviper pushed a commit to saethlin/rayon that referenced this pull request Apr 1, 2022
The uninitialized region of a Vec cannot be accessed by slicing the Vec
first, see rust-lang/rust#92097

Similarly, it is never valid to merge adjacent slices, because any
pointer derived from a slice only has provenance over that slice, not
anything adjacent. So we pass raw pointers and a length around to avoid
narrowing provenance by converting to a reference.
@saethlin saethlin deleted the split-without-deref branch May 16, 2022 04:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants