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

Jump to bottom

Fix UB in ThinVec::flat_map_in_place #136579

Merged
merged 1 commit into from
Feb 28, 2025
Merged

Fix UB in ThinVec::flat_map_in_place #136579

merged 1 commit into from
Feb 28, 2025

Conversation

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented Feb 5, 2025

thin_vec.as_ptr() goes through the Deref impl of ThinVec, which will not allow access to any memory as we did call set_len(0) first.

Found in the process of investigating #135870.

@rustbot
Copy link
Collaborator

rustbot commented Feb 5, 2025

r? @petrochenkov

rustbot has assigned @petrochenkov.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 5, 2025
@lqd
Copy link
Member

lqd commented Feb 5, 2025

(I myself was trying to see if we had applied the same strategies in rustc_data_structures as #135103 :)

@bjorn3 bjorn3 mentioned this pull request Feb 5, 2025
Merged
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Feb 6, 2025
@workingjubilee
Rollup merge of rust-lang#136580 - bjorn3:miri_fixes, r=lqd
38efb64 
Couple of changes to run rustc in miri

This is not the full set of patches required to run rustc in miri, but it is the fast majority of the changes to rustc itself. There is also a change to the jobserver crate necessary and on arm64 a change to the memchr crate. Running rustc in miri has already found some UB: rust-lang#136579

cc rust-lang#135870 (comment)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 6, 2025
@matthiaskrgr
Rollup merge of rust-lang#136580 - bjorn3:miri_fixes, r=lqd
53923c7 
Couple of changes to run rustc in miri

This is not the full set of patches required to run rustc in miri, but it is the fast majority of the changes to rustc itself. There is also a change to the jobserver crate necessary and on arm64 a change to the memchr crate. Running rustc in miri has already found some UB: rust-lang#136579

cc rust-lang#135870 (comment)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 6, 2025
@matthiaskrgr
Rollup merge of rust-lang#136580 - bjorn3:miri_fixes, r=lqd
3eb1ef8 
Couple of changes to run rustc in miri

This is not the full set of patches required to run rustc in miri, but it is the fast majority of the changes to rustc itself. There is also a change to the jobserver crate necessary and on arm64 a change to the memchr crate. Running rustc in miri has already found some UB: rust-lang#136579

cc rust-lang#135870 (comment)
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 7, 2025
@rust-timer
Unrolled build for rust-lang#136580
3f447fb 
Rollup merge of rust-lang#136580 - bjorn3:miri_fixes, r=lqd

Couple of changes to run rustc in miri

This is not the full set of patches required to run rustc in miri, but it is the fast majority of the changes to rustc itself. There is also a change to the jobserver crate necessary and on arm64 a change to the memchr crate. Running rustc in miri has already found some UB: rust-lang#136579

cc rust-lang#135870 (comment)
@bjorn3 bjorn3 force-pushed the fix_thinvec_ext_ub branch from 9acabd4 to bd3fee2 Compare February 13, 2025 16:18
@petrochenkov
Copy link
Contributor

r? compiler
(I have too many assigned PRs)

@rustbot rustbot assigned jieyouxu and unassigned petrochenkov Feb 19, 2025
@jieyouxu
Copy link
Member

r? compiler

@rustbot rustbot assigned BoxyUwU and unassigned jieyouxu Feb 19, 2025
@bjorn3 bjorn3 force-pushed the fix_thinvec_ext_ub branch from bd3fee2 to 6cea018 Compare February 22, 2025 11:42
@BoxyUwU
Copy link
Member

BoxyUwU commented Feb 23, 2025

I can review this but itll take me a few days to get to it 👍

@BoxyUwU
Copy link
Member

BoxyUwU commented Feb 26, 2025 •
edited
Loading

Would it make sense to convert the macro to use the LeakGuard way of doing this to avoid the code duplication? Using macros for unsafe code is pretty spooky either way lol

r=me if there's a reason to not do that

@BoxyUwU BoxyUwU added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 26, 2025
@bjorn3
Fix UB in ThinVec::flat_map_in_place
3477297 
thin_vec.as_ptr() goes through the Deref impl of ThinVec, which will
not allow access to any memory as we did call set_len(0) first.
@bjorn3 bjorn3 force-pushed the fix_thinvec_ext_ub branch from 6cea018 to 3477297 Compare February 26, 2025 15:49
@bjorn3
Copy link
Member Author

bjorn3 commented Feb 26, 2025 •
edited
Loading

I wasn't sure what the best way was of defining the LeakGuard struct before. I figured something out now though. Please take a look.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 26, 2025
@BoxyUwU
Copy link
Member

BoxyUwU commented Feb 26, 2025

very cool :3

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 26, 2025

📌 Commit 3477297 has been approved by BoxyUwU

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 26, 2025
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Feb 26, 2025
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 27, 2025
@matthiaskrgr
Rollup merge of rust-lang#136579 - bjorn3:fix_thinvec_ext_ub, r=BoxyUwU
2ab38b2 
Fix UB in ThinVec::flat_map_in_place

`thin_vec.as_ptr()` goes through the `Deref` impl of `ThinVec`, which will not allow access to any memory as we did call `set_len(0)` first.

Found in the process of investigating rust-lang#135870.
This was referenced Feb 27, 2025
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 27, 2025
@bors
Auto merge of rust-lang#137710 - matthiaskrgr:rollup-3vmxxu9, r=matth…
8346456 
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#136542 ([`compiletest`-related cleanups 4/7] Make the distinction between root build directory vs test suite specific build directory in compiletest less confusing)
 - rust-lang#136579 (Fix UB in ThinVec::flat_map_in_place)
 - rust-lang#136688 (require trait impls to have matching const stabilities as the traits)
 - rust-lang#136846 (Make `AssocOp` more like `ExprKind`)
 - rust-lang#137304 (add `IntoBounds::intersect` and `RangeBounds::is_empty`)
 - rust-lang#137455 (Reuse machinery from `tail_expr_drop_order` for `if_let_rescope`)
 - rust-lang#137480 (Return unexpected termination error instead of panicing in `Thread::join`)
 - rust-lang#137694 (Spruce up `AttributeKind` docs)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 27, 2025
@matthiaskrgr
Rollup merge of rust-lang#136579 - bjorn3:fix_thinvec_ext_ub, r=BoxyUwU
a334f8b 
Fix UB in ThinVec::flat_map_in_place

`thin_vec.as_ptr()` goes through the `Deref` impl of `ThinVec`, which will not allow access to any memory as we did call `set_len(0)` first.

Found in the process of investigating rust-lang#135870.
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 27, 2025
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 27, 2025
@bors
Auto merge of rust-lang#137717 - matthiaskrgr:rollup-ejtd189, r=<try>
be046d7 
Rollup of 4 pull requests

Successful merges:

 - rust-lang#136579 (Fix UB in ThinVec::flat_map_in_place)
 - rust-lang#137455 (Reuse machinery from `tail_expr_drop_order` for `if_let_rescope`)
 - rust-lang#137480 (Return unexpected termination error instead of panicing in `Thread::join`)
 - rust-lang#137694 (Spruce up `AttributeKind` docs)

r? `@ghost`
`@rustbot` modify labels: rollup

try-job: i686-msvc-1
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 27, 2025
@bors
Auto merge of rust-lang#137710 - matthiaskrgr:rollup-3vmxxu9, r=matth…
b4657c0 
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#136542 ([`compiletest`-related cleanups 4/7] Make the distinction between root build directory vs test suite specific build directory in compiletest less confusing)
 - rust-lang#136579 (Fix UB in ThinVec::flat_map_in_place)
 - rust-lang#136688 (require trait impls to have matching const stabilities as the traits)
 - rust-lang#136846 (Make `AssocOp` more like `ExprKind`)
 - rust-lang#137304 (add `IntoBounds::intersect` and `RangeBounds::is_empty`)
 - rust-lang#137455 (Reuse machinery from `tail_expr_drop_order` for `if_let_rescope`)
 - rust-lang#137480 (Return unexpected termination error instead of panicing in `Thread::join`)
 - rust-lang#137694 (Spruce up `AttributeKind` docs)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 28, 2025
@bors
Auto merge of rust-lang#137710 - matthiaskrgr:rollup-3vmxxu9, r=matth…
cb08599 
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#136542 ([`compiletest`-related cleanups 4/7] Make the distinction between root build directory vs test suite specific build directory in compiletest less confusing)
 - rust-lang#136579 (Fix UB in ThinVec::flat_map_in_place)
 - rust-lang#136688 (require trait impls to have matching const stabilities as the traits)
 - rust-lang#136846 (Make `AssocOp` more like `ExprKind`)
 - rust-lang#137304 (add `IntoBounds::intersect` and `RangeBounds::is_empty`)
 - rust-lang#137455 (Reuse machinery from `tail_expr_drop_order` for `if_let_rescope`)
 - rust-lang#137480 (Return unexpected termination error instead of panicing in `Thread::join`)
 - rust-lang#137694 (Spruce up `AttributeKind` docs)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 18c47ad into rust-lang:master Feb 28, 2025
6 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Feb 28, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 28, 2025
@rust-timer
Unrolled build for rust-lang#136579
d9725a5 
Rollup merge of rust-lang#136579 - bjorn3:fix_thinvec_ext_ub, r=BoxyUwU

Fix UB in ThinVec::flat_map_in_place

`thin_vec.as_ptr()` goes through the `Deref` impl of `ThinVec`, which will not allow access to any memory as we did call `set_len(0)` first.

Found in the process of investigating rust-lang#135870.
@bjorn3 bjorn3 deleted the fix_thinvec_ext_ub branch February 28, 2025 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@BoxyUwU BoxyUwU

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.87.0
Development

Successfully merging this pull request may close these issues.
7 participants
@bjorn3 @rustbot @lqd @petrochenkov @jieyouxu @BoxyUwU @bors