BurtonQin / lockbud

Statically detect memory, concurrency bugs and possible panic locations for Rust.
BSD 3-Clause "New" or "Revised" License
445 stars 25 forks source link

Detection on `atomic-violation` toy code emits a possible AtomicityViolation on getrandom #61

Closed zjp-CN closed 3 months ago

zjp-CN commented 3 months ago

Thanks for this tool. I'm no expert on concurrency programming.

When using lockbud on its atomic-violation example, I noticed there is a bug reporting for getrandom crate:

{
  "AtomicityViolation": {
    "bug_kind": "AtomicityViolation",
    "possibility": "Possibly",
    "diagnosis": {
      "fn_name": "util::LazyUsize::unsync_init",
      "atomic_reader": "/root/.cargo/registry/src/rsproxy.cn-0dccff568467c15b/getrandom-0.2.7/src/util.rs:44:23: 44:43 (#0)",
      "atomic_writer": "/root/.cargo/registry/src/rsproxy.cn-0dccff568467c15b/getrandom-0.2.7/src/util.rs:47:13: 47:39 (#0)",
      "dep_kind": "Both"
    },
    "explanation": "atomic::store is data/control dependent on atomic::load"
  }
}

which points to this piece of code on docs.rs. On latest getrandom codebase, it's here, which has almost the same logic.

    // Runs the init() function at most once, returning the value of some run of
    // init(). Multiple callers can run their init() functions in parallel.
    // init() should always return the same value, if it succeeds.
    fn unsync_init(&self, init: impl FnOnce() -> usize) -> usize {
        #[cold]
        fn do_init(this: &LazyUsize, init: impl FnOnce() -> usize) -> usize {
            let val = init();
            this.0.store(val, Ordering::Relaxed);
            val
        }

        // Relaxed ordering is fine, as we only have a single atomic variable.
        let val = self.0.load(Ordering::Relaxed);
        if val != Self::UNINIT {
            val
        } else {
            do_init(self, init)
        }
    }

So I'm reporting here to wonder which is correct between the documented code of getrandom and the result of lockbud.

BurtonQin commented 3 months ago

Without other sync mechanism, unsync_init alone could not hold the promise of running init once across multithreads as the comment says. Since two threads could find the state is UNINIT at the same time and then both execute init. So it satisfies the definition of atomic violation. However, if init is constrained to be a pure function and always returns the same value without any other side effects, then running init multiple times only leads to some overhead and no logic error might occur.

zjp-CN commented 3 months ago

Cool. I'm grateful for your concise and clear explanation.

Closing this.