Skip to content

RacyCell<T>: Data race allowed on T#390

Merged
phil-opp merged 1 commit intorust-osdev:mainfrom
kuzeyardabulut:main
Aug 4, 2023
Merged

RacyCell<T>: Data race allowed on T#390
phil-opp merged 1 commit intorust-osdev:mainfrom
kuzeyardabulut:main

Conversation

@kuzeyardabulut
Copy link
Contributor

@kuzeyardabulut kuzeyardabulut commented Aug 3, 2023

Hi,
I found a memory-safety/soundness issue in this crate while scanning Rust code for potential vulnerabilities. This PR contains a fix for the issue.

Issue Description

RacyCell<T> unconditionally implements Sync. This allows users to create data races on T: !Sync. Such data races can lead to undefined behavior.

bootloader/bios/common/src/racy_cell.rs

Line 21 in c5e5ed2

unsafe impl<T> Sync for RacyCell<T> {}

@bjorn3
Copy link
Contributor

bjorn3 commented Aug 3, 2023

RacyCell doesn't expose any safe method that allows access to the internal value through a shared reference, so the internal value doesn't need to be Sync for RacyCell to be sync.

The fact that this PR doesn't break CI suggests that RacyCell isn't needed however. I think all uses can be replaced with the unstable core::cell::SyncOnceCell which does have the T: Sync bound on it's Sync impl.

@kuzeyardabulut
Copy link
Contributor Author

kuzeyardabulut commented Aug 3, 2023

I agree with what you said. This may not cause a direct data race. But making the above changes can be useful.

@phil-opp
Copy link
Member

phil-opp commented Aug 4, 2023

The fact that this PR doesn't break CI suggests that RacyCell isn't needed however. I think all uses can be replaced with the unstable core::cell::SyncOnceCell which does have the T: Sync bound on it's Sync impl.

I guess you mean the SyncUnsafeCell? Yes, we could use that as well, but I would prefer to wait until it's stabilized.

phil-opp
phil-opp approved these changes Aug 4, 2023
View reviewed changes
Copy link
Member

@phil-opp phil-opp left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me, thanks for submitting!

@phil-opp phil-opp merged commit ef1d4a9 into rust-osdev:main Aug 4, 2023
@phil-opp phil-opp mentioned this pull request Sep 11, 2023
Closed
@phil-opp phil-opp mentioned this pull request Dec 28, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@phil-opp phil-opp phil-opp approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kuzeyardabulut @bjorn3 @phil-opp