Skip to content

Conversation

@kadiwa4
Copy link
Contributor

@kadiwa4 kadiwa4 commented Jan 7, 2024

I noticed that the implementations of Rc/Arc create a lot of intermediate references to values that are not initialised or that shouldn't be accessed because of aliasing rules. This PR replaces some dubious cases with ptr::addr_of(_mut) and replaces Layout::for_value with Layout::for_value_raw.

@rustbot
Copy link
Collaborator

rustbot commented Jan 7, 2024

r? @joshtriplett

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 7, 2024
Comment on lines +2001 to +2002
for item in iter {
ptr::write(elems.add(guard.n_elems), item);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

drive-by simplification

@taiki-e
Copy link
Member

taiki-e commented Jan 7, 2024

I believe this is a duplicate of #119433.

@kadiwa4
Copy link
Contributor Author

kadiwa4 commented Jan 7, 2024

whoops, I missed that while searching for existing PRs :/
my PR has more changes however

// We are careful to *not* create a reference covering the "count" fields, as
// this would conflict with accesses to the reference counts (e.g. by `Weak`).
unsafe { &mut (*this.ptr.as_ptr()).value }
unsafe { &mut *ptr::addr_of_mut!((*this.ptr.as_ptr()).value) }
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this created any unnecessary references, the addr_of_mut roundtrip seems pointless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe you're right, but this does result in different MIR output.
I thought that they were different because the previous line created a reference to the entire struct, not just that one field

Copy link
Member

Choose a reason for hiding this comment

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

I thought that they were different because the previous line created a reference to the entire struct, not just that one field

IIUC, &mut (*ptr).field does not create a reference to the entire struct. See also the following lines in Weak::inner.

rust/library/alloc/src/sync.rs

Lines 2849 to 2852 in 87e1430

// We are careful to *not* create a reference covering the "data" field, as
// the field may be mutated concurrently (for example, if the last `Arc`
// is dropped, the data field will be dropped in-place).
Some(unsafe { WeakInner { strong: &(*ptr).strong, weak: &(*ptr).weak } })

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw that comment but I didn't quite trust it because it's apparently part of faulty code :) I guess I can close this tho

@taiki-e
Copy link
Member

taiki-e commented Jan 7, 2024

my PR has more changes however

IIUC, most of the differences appeared to be actually unnecessary changes. Like #119691 (comment).

@kadiwa4 kadiwa4 closed this Jan 7, 2024
@kadiwa4 kadiwa4 deleted the fix_rc_arc_pointer_ub branch January 23, 2024 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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