Skip to content

TypeHint<T>: Data race allowed on T#240

Closed
kuzeyardabulut wants to merge 1 commit into
rxRust:masterfrom
kuzeyardabulut:master
Closed

TypeHint<T>: Data race allowed on T#240
kuzeyardabulut wants to merge 1 commit into
rxRust:masterfrom
kuzeyardabulut:master

Conversation

@kuzeyardabulut
Copy link
Copy Markdown

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

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

rxRust/src/type_hint.rs

Lines 18 to 19 in a54dbd7

unsafe impl<T> Sync for TypeHint<T> {}
unsafe impl<T> Send for TypeHint<T> {}

@M-Adoo
Copy link
Copy Markdown
Collaborator

M-Adoo commented Aug 7, 2023

@kuzeyardabulut Thanks for your help!

But the TypeHint is not contained in the struct, is just used to keep the type information. and should not effect its parent Send or Sync. So we can't just add the Send and Sync conditional in the implementations.

The better way is to remove the TypeHint<T>, and use PhantomData<fn() -> T> instead in every struct used TypeHint<T>.

@kuzeyardabulut
Copy link
Copy Markdown
Author

@kuzeyardabulut Thanks for your help!

But the TypeHint is not contained in the struct, is just used to keep the type information. and should not effect its parent Send or Sync. So we can't just add the Send and Sync conditional in the implementations.

The better way is to remove the TypeHint<T>, and use PhantomData<fn() -> T> instead in every struct used TypeHint<T>.

Hey,
Thanks for your response! I am not very familiar with this project. If you want I can close this PR and create a issue. To solve this problem, you can create a new PR and assign it to that issue.

@M-Adoo
Copy link
Copy Markdown
Collaborator

M-Adoo commented Aug 9, 2023

@kuzeyardabulut Thanks for your help!
But the TypeHint is not contained in the struct, is just used to keep the type information. and should not effect its parent Send or Sync. So we can't just add the Send and Sync conditional in the implementations.
The better way is to remove the TypeHint<T>, and use PhantomData<fn() -> T> instead in every struct used TypeHint<T>.

Hey, Thanks for your response! I am not very familiar with this project. If you want I can close this PR and create a issue. To solve this problem, you can create a new PR and assign it to that issue.

Thanks, this is not a bug, just an implementation that is not elegant enough, you can leave this pr, I will deal with it when I have time, no need to open a new issue to track it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants