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

fix(api)!: fix safety issue with raw::Mutex locking API #17

Merged
merged 4 commits into from
Nov 2, 2024

Conversation

pedromfedricci
Copy link
Owner

@pedromfedricci pedromfedricci commented Nov 1, 2024

This PR introduces breaking changes to fix a safety issue with the current API. Currently, users may invoke undefined behavior with a safe and allowed call of core::mem::forget over a instance of raw::MutexGuard. See the example bellow for one possible order of events that will trigger UB under the current API. From this version onwards, users don't have any access to a raw::MutexGuard instance. To access the mutex's protected data, user's will get a &mut T as the closure parameter for locking operations under raw::Mutex. No raw locking API will return a guard instance.

BREAKING

  • Remove raw::MutexGuard type, raw::Mutex::lock and raw::Mutex::try_lock methods
  • Rename raw::Mutex::lock_with method to raw::Mutex::lock_then
  • Rename raw::Mutex::try_lock_with method to raw::Mutex::try_lock_then
  • Rename raw::Mutex::lock_with_local method to raw::Mutex::lock_with_local_then
  • Rename raw::Mutex::lock_with_local_unchecked method to raw::Mutex::lock_with_local_then_unchecked
  • Rename raw::Mutex::try_lock_with_local method to raw::Mutex::try_lock_with_local_then
  • Rename raw::Mutex::try_lock_with_local_unchecked method to raw::Mutex::try_lock_with_local_then_unchecked
  • Rename barging::Mutex::lock_with method to barging::Mutex::lock_then
  • Rename barging::Mutex::try_lock_with method to barging::Mutex::try_lock_then
  • Change closure parameters of raw::Mutex's then (previous with) locking methods from MutexGuard to &mut T

Added

  • Add new raw::Mutex::lock_with_then method
  • Add new raw::Mutex::try_lock_with_then method

Undefined behavior with current API:

// Version <= 0.3.0

use std::sync::{mpsc::channel, Arc};
use std::thread;

use mcslock::raw::{spins::Mutex, MutexNode};

fn main() {
        let (tx1, rx1) = channel();
        let (tx2, rx2) = channel();
        let data = Arc::new(Mutex::new(0));
        let handle = Arc::clone(&data);
        let mut node = MutexNode::new();
        let guard = data.lock(&mut node);

        thread::spawn(move || {
            // wait for main to transfer the lock ownership
            rx2.recv().unwrap();
            // allocate a node in this thread's stack
            let mut node = MutexNode::new();
            // the node pointer is added to the queue's tail
            let guard = handle.lock(&mut node);
            // the `forget` call prevents the guard from dropping (unlocking)
            // and so this node pointer **is not** removed from the queue
            core::mem::forget(guard);
            tx1.send(()).unwrap();
        });

        // unlock to transfer lock ownership to spawned thread
        drop(guard);
        tx2.send(()).unwrap();
        rx1.recv().unwrap();
        // previous node is uninitialized, since the thread exited
        // this lock operation access the predecessor's dangling pointer
        // therefore resulting in undefined behavior 
        let guard = data.lock(&mut node);
        assert_eq!(*guard, 0);
}

@codecov-commenter
Copy link

codecov-commenter commented Nov 1, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 98.60465% with 3 lines in your changes missing coverage. Please review.

Project coverage is 98.90%. Comparing base (ff456f5) to head (a055319).

Files with missing lines Patch % Lines
src/inner/raw/mod.rs 96.92% 2 Missing ⚠️
src/inner/barging/mod.rs 97.56% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #17      +/-   ##
==========================================
- Coverage   99.51%   98.90%   -0.61%     
==========================================
  Files           5        9       +4     
  Lines         205      275      +70     
==========================================
+ Hits          204      272      +68     
- Misses          1        3       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pedromfedricci pedromfedricci changed the title Locking api changes fix(api)!: fix safety issue with raw::Mutex locking API Nov 2, 2024
@pedromfedricci pedromfedricci merged commit 9622d92 into main Nov 2, 2024
10 checks passed
@pedromfedricci pedromfedricci deleted the locking-api-changes branch November 2, 2024 14:32
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