Skip to content

Race Condition and Performance Regression in wasm-bindgen util.rs #4502

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.

Sign up for GitHub

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

Jump to bottom
Open
Chain-Fox opened this issue May 5, 2025 · 0 comments · May be fixed by #4503
Open

Race Condition and Performance Regression in wasm-bindgen util.rs #4502

Chain-Fox opened this issue May 5, 2025 · 0 comments · May be fixed by #4503
Labels
bug

Comments

@Chain-Fox
Copy link

Chain-Fox commented May 5, 2025 •
edited
Loading

Describe the Bug

There is a race condition and a related performance issue in the following section of the wasm-bindgen codebase:

wasm-bindgen/crates/backend/src/util.rs

Lines 135 to 161 in c35cc93

impl<T: Hash> fmt::Display for ShortHash<T> {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
static HASHED: AtomicBool = AtomicBool::new(false);
static HASH: AtomicUsize = AtomicUsize::new(0);
// Try to amortize the cost of loading env vars a lot as we're gonna be
// hashing for a lot of symbols.
if !HASHED.load(SeqCst) {
let mut h = DefaultHasher::new();
env::var("CARGO_PKG_NAME")
.expect("should have CARGO_PKG_NAME env var")
.hash(&mut h);
env::var("CARGO_PKG_VERSION")
.expect("should have CARGO_PKG_VERSION env var")
.hash(&mut h);
// This may chop off 32 bits on 32-bit platforms, but that's ok, we
// just want something to mix in below anyway.
HASH.store(h.finish() as usize, SeqCst);
HASHED.store(true, SeqCst);
}
let mut h = DefaultHasher::new();
HASH.load(SeqCst).hash(&mut h);
self.0.hash(&mut h);
write!(f, "{:016x}", h.finish())
}
}

  1. Race Condition:
    The affected code attempts to lazily compute and cache a hash value, but the current implementation is not thread-safe. Consider the following interleaving of two threads:
Thread 1 Thread 2
check Hashed check Hashed
recompute Hash recompute Hash

If two threads call the fmt function concurrently, they may both observe the Hashed flag as false and proceed to recompute the hash independently. This violates the intent of computing the hash only once and leads to redundant work. While the result is ultimately consistent (assuming a stable environment), this introduces unnecessary overhead.

  1. Memory Ordering Issue
    The code currently uses Ordering::SeqCst, which is overly strict for this scenario. A combination of Ordering::Acquire and Ordering::Release is sufficient to ensure correct synchronization while allowing better performance on most architectures. But for now OnceLock is the most concise implementation.

Steps to Reproduce

Found from static analysis

Expected Behavior

No race condition. Performance increase.

Actual Behavior

In a benchmark on x86 Linux, replacing SeqCst atomics with OnceLock not only eliminated potential race conditions by ensuring the hash is computed only once, but also yielded a slight performance improvement.

Additional Context

I will submit a PR with a fix
@Chain-Fox Chain-Fox added the bug label May 5, 2025
@Chain-Fox Chain-Fox linked a pull request May 5, 2025 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Replace atomic operations with Once in backend/util Chain-Fox/wasm-bindgen
1 participant
@Chain-Fox