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

Maybe unsound in watchdog::descriptor::confirm #1683

Closed
lwz23 opened this issue Dec 24, 2024 · 4 comments
Closed

Maybe unsound in watchdog::descriptor::confirm #1683

lwz23 opened this issue Dec 24, 2024 · 4 comments
Assignees
Labels
bug Something isn't working

Comments

@lwz23
Copy link

lwz23 commented Dec 24, 2024

Describe the bug

Hello, thank you for your contribution in this project, I am scanning the unsoundness problem in rust project.
I notice the following code:

pub struct OwnedDescriptor {
    segment: Arc<Segment>,
    pub atomic: *const AtomicU64,
    pub mask: u64,
}

pub fn confirm(&self) {
        unsafe {
            (*self.atomic).fetch_or(self.mask, std::sync::atomic::Ordering::SeqCst);
        };
    }

Considering that pub mod watchdog, pub struct OwnedDescriptor + pub atomic: *const AtomicU64,and confirm is also a pub function. I assume that users can directly call this function and control the atomic field. This potential situation could result in (*self.atomic) being operating on a null pointer, I suppose it might trigger undefined behavior (UB). For safety reasons, I felt it necessary to report this issue. If you have performed checks elsewhere that ensure this is safe, please don’t take offense at my raising this issue.
I suggest Several possible fixes:

  1. If there is no external usage for OwnedDescriptor, it should not marked as pub at least its atomic field should not be pub.
  2. confirm method should add additional check for invalid pointer.
  3. mark confirm method as unsafe and proper doc to let users know that they should provide valid Pointers.

To reproduce

  1. crate a OwnedDescriptor struct.
  2. set the atomic field as null pointer.
  3. call confirm method.

System info

I think it can happen in any platform.

@lwz23 lwz23 added the bug Something isn't working label Dec 24, 2024
@JEnoch
Copy link
Member

JEnoch commented Dec 26, 2024

Thanks for you report!
@yellowhatter could you please analyse and comment ?

@yellowhatter
Copy link
Contributor

yellowhatter commented Dec 26, 2024

Hello, @lwz23 !
Thanks for this report.

OwnedDescriptor is a part of zenoh-internal crate zenoh-shm which is not intended to be used by the user and there is no way to get buffer descriptor instances through public zenoh API.

@lwz23
Copy link
Author

lwz23 commented Dec 26, 2024

FYI: Since watchdog is a pub mod, this structure and function are also pub. According to the rust security specification, any rust function that may trigger UB should be marked as unsafe or the field should be set to private. Although UB may not be triggered by users in the current project, since this mod may be used by other projects or engineering, for safety reasons, I still recommend setting this field to private if there is no reason to set it to pub.

@yellowhatter
Copy link
Contributor

FYI: Since watchdog is a pub mod, this structure and function are also pub. According to the rust security specification, any rust function that may trigger UB should be marked as unsafe or the field should be set to private. Although UB may not be triggered by users in the current project, since this mod may be used by other projects or engineering, for safety reasons, I still recommend setting this field to private if there is no reason to set it to pub.

Yes, you are right. I'll change it to be unsafe, thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants