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

Resolve UB in Arc/Weak interaction (2) #72533

Merged
merged 1 commit into from
May 27, 2020
Merged

Conversation

Diggsey
Copy link
Contributor

@Diggsey Diggsey commented May 24, 2020

Use raw pointers to avoid making any assertions about the data field.

Follow up from #72479, see that PR for more detail on the motivation.

@RalfJung I was able to avoid a lot of the changes to Weak, by making a helper type (WeakInner) - because of auto-deref and because the fields have the same name, the rest of the code continues to compile.

@rust-highfive
Copy link
Collaborator

r? @dtolnay

(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 May 24, 2020
@Diggsey Diggsey changed the title Fix UB in Arc Resolve UB in Arc/Weak interaction May 24, 2020
@Diggsey Diggsey changed the title Resolve UB in Arc/Weak interaction Resolve UB in Arc/Weak interaction (2) May 24, 2020
src/liballoc/sync.rs Outdated Show resolved Hide resolved
src/liballoc/sync.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

I was able to avoid a lot of the changes to Weak, by making a helper type (WeakInner) - because of auto-deref and because the fields have the same name, the rest of the code continues to compile.

That's a very clever trick, I like it. :)

src/liballoc/sync.rs Outdated Show resolved Hide resolved
@Diggsey Diggsey force-pushed the db-fix-arc-ub2 branch 2 times, most recently from e351799 to 177e098 Compare May 25, 2020 10:27
src/liballoc/sync.rs Outdated Show resolved Hide resolved
Use raw pointers to avoid making any assertions about the data field.
Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

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

LGTM!

I'd prefer if someone else would also review this --the more eyes, the better.

@dtolnay dtolnay added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label May 26, 2020
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Looks good to me as well. Thanks for the helpful round of review.

@dtolnay
Copy link
Member

dtolnay commented May 26, 2020

@bors r+

@bors
Copy link
Contributor

bors commented May 26, 2020

📌 Commit ee6e705 has been approved by dtolnay

@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 May 26, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 26, 2020
Resolve UB in Arc/Weak interaction (2)

Use raw pointers to avoid making any assertions about the data field.

Follow up from rust-lang#72479, see that PR for more detail on the motivation.

@RalfJung I was able to avoid a lot of the changes to `Weak`, by making a helper type (`WeakInner`) - because of auto-deref and because the fields have the same name, the rest of the code continues to compile.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 27, 2020
Resolve UB in Arc/Weak interaction (2)

Use raw pointers to avoid making any assertions about the data field.

Follow up from rust-lang#72479, see that PR for more detail on the motivation.

@RalfJung I was able to avoid a lot of the changes to `Weak`, by making a helper type (`WeakInner`) - because of auto-deref and because the fields have the same name, the rest of the code continues to compile.
bors added a commit to rust-lang-ci/rust that referenced this pull request May 27, 2020
Rollup of 6 pull requests

Successful merges:

 - rust-lang#72348 (Fix confusing error message for comma typo in multiline statement)
 - rust-lang#72533 (Resolve UB in Arc/Weak interaction (2))
 - rust-lang#72548 (Add test for old compiler ICE when using `Borrow`)
 - rust-lang#72606 (Small cell example update)
 - rust-lang#72610 (Remove font-display settings)
 - rust-lang#72626 (Add remark regarding DoubleEndedIterator)

Failed merges:

r? @ghost
@bors bors merged commit 8f95dc8 into rust-lang:master May 27, 2020
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.

5 participants