btop's broken lock

I was having issues with btop. It would occasionally crash, and in January 2025 I opened issue #1012 with a core dump. The trigger seemed related to CPU cores being off-lined, but the failure was rare and I did not have a clean reproducer. I poked at it, found other sanitizer failures, opened #1042, and then mostly ignored it.

About a year later I came back to the races. I was tracing where synchronization was missing around the UI and runner thread, and I got curious about the custom atomic lock used throughout the codebase. I opened the implementation.

WTF. The mutex was not a mutex.

There were two separate concurrency bugs in the same small set of helpers: the blocking lock could admit multiple owners, and the runner's atomic state was being waited on with relaxed memory ordering even though callers treated that wait as a synchronization boundary.

The first bug: compare-exchange could admit a second owner

Before the fix, atomic_lock wrapped a std::atomic<bool>. Constructing it set the boolean to true; destroying it set the boolean back to false. With wait=true, construction used a compare-exchange loop so it was supposed to wait until it could change the state from unlocked to locked.

atomic_lock::atomic_lock(atomic<bool>& atom, bool wait) : atom(atom) {
    if (wait)
        while (not this->atom.compare_exchange_strong(this->not_true, true));
    else
        this->atom.store(true);

    this->atom.notify_all();
}

atomic_lock::~atomic_lock() noexcept {
    this->atom.store(false);
    this->atom.notify_all();
}

The suspicious member was not_true. It was a boolean initialized to false and then reused forever as the expected argument to compare_exchange_strong.

If you have not used C++ atomics directly, compare-exchange has one behavior that matters here:

atom.compare_exchange_strong(expected, desired)

If atom == expected, it stores desired and returns true. If they are not equal, it returns false and overwrites expected with the value it actually observed.

That last part is the entire bug.

Thread A changes false → true and enters the critical section. Thread B starts with expected == false, sees that the atomic is already true, and its first CAS correctly fails. But the failed CAS also writes true into expected.

The loop does not reset it.

So Thread B immediately tries again with expected == true. The atomic is still true because Thread A still owns the lock. The comparison succeeds. The desired value is also true, so the operation is effectively true → true. C++ reports success and Thread B leaves the loop.

Now both threads believe they own the lock.

Under contention, the implementation did not merely have weak fairness or a rare edge case. One failed compare-exchange could transform the next iteration into a successful no-op. Any code relying on atomic_lock(..., true) for mutual exclusion could execute concurrently.

What that meant in btop

At the May 2026 base revision, the waiting form of this lock guarded at least two important paths:

This is the part that made me wonder how the project had run as well as it had. The answer is mostly that concurrency bugs can have very low activation probability. If the second thread arrives after the first releases the flag, nothing bad happens. If two paths do not contend often, nothing bad happens. If the conflicting operations happen to touch disjoint state, nothing visible happens. Then one machine, one timing change, one sanitizer build, or one unusual event makes the latent bug real.

Undefined behavior is not obliged to fail consistently. A broken mutex can ship for years precisely because most executions never ask it the question that exposes the bug.

The second bug: an atomic flag is not a synchronization barrier

The broken compare-exchange was obvious once I read it carefully. The memory-ordering problem is subtler.

btop has a runner thread that collects data and draws the interface. The main/input thread also changes configuration and UI state. Runner::active was an atomic boolean used as a gate: the runner set it while it was doing non-thread-safe work, and main-thread code called atomic_wait(Runner::active) before touching state that could conflict with the runner.

The helper looked like this:

void atomic_wait(const atomic<bool>& atom, bool old) noexcept {
    atom.wait(old, std::memory_order_relaxed);
}

void atomic_wait_for(const atomic<bool>& atom, bool old, uint64_t wait_ms) noexcept {
    while (atom.load(std::memory_order_relaxed) == old && ...)
        sleep_ms(1);
}

relaxed is fine when all you care about is the atomic value itself. It gives atomicity and a modification order for that atomic. It does not make ordinary reads and writes around it synchronize with another thread.

That distinction matters because callers were not waiting just to learn a boolean. They were treating the transition of Runner::active to false as permission to touch ordinary shared objects after the runner had finished with them.

For this pattern, you need a release operation when publishing “I am done” and an acquire operation in the thread that observes it. If the acquire reads from the release sequence, operations sequenced before the release happen-before operations sequenced after the acquire.

In less standards-heavy language: the flag transition becomes the hand-off point. Everything the runner did before releasing the flag is ordered before what the UI does after acquiring it.

The first atomic fix changed waits to acquire and unlock/store operations to release:

void atomic_wait(const atomic<bool>& atom, bool old) noexcept {
    atom.wait(old, std::memory_order_acquire);
}

bool expected = false;
while (!atom.compare_exchange_strong(
    expected,
    true,
    std::memory_order_acquire,
    std::memory_order_relaxed
)) {
    expected = false;
}

// unlock
atom.store(false, std::memory_order_release);

The failed compare-exchange does not acquire anything, so its failure ordering can stay relaxed. The important part is to reset expected to false before retrying, acquire on successful ownership, and release when publishing the unlock.

A concrete race: current_preset

The path that initially pulled me into this was Config::current_preset, a std::optional<int> used by both input handling and drawing.

The runner's draw path read it while building the UI:

!Config::current_preset.has_value()
    ? "*"
    : to_string(Config::current_preset.value())

Meanwhile, pressing p or P mutated that same optional from the input thread. The code did contain an atomic_wait(Runner::active), but it happened after the reads and mutations:

const auto old_preset = Config::current_preset;

// reads and writes current_preset here
...

atomic_wait(Runner::active);
Config::apply_preset(...);

That guard was protecting the wrong side of the operation. By the time the code waited for the runner, the unsynchronized access had already happened.

Commit 557fbe5 moved the wait above the first access. Two more menu paths could reset current_preset while the runner was drawing, so 677336f added the same guard there.

This is also why fixing only the atomic primitive would not have been enough. A correct lock does nothing for code that touches the shared object before taking the lock.

Why I eventually stopped forcing the timed wait through an atomic

The first version of the fix repaired the atomic semantics. A day later I split the runner state into atomic_waiting_lock, backed by a normal std::mutex and std::condition_variable.

That was not an admission that atomics are bad. The requirement was awkward: btop wanted both ordinary waiting and timed waiting on the runner state. C++ atomic wait/notify does not give the same straightforward timed-wait API as a condition variable. The old atomic_wait_for worked around that by repeatedly loading the atomic and sleeping for 1 ms.

The replacement made the semantics explicit:

class atomic_waiting_lock {
    bool value{};
    mutable std::mutex mtx;
    mutable std::condition_variable cv;
    ...
};

void atomic_waiting_lock::wait_for(bool old, uint64_t ms) const noexcept {
    std::unique_lock lock{mtx};
    cv.wait_for(lock, std::chrono::milliseconds(ms),
                [this, old] { return value != old; });
}

The runner's active state was switched from atomic<bool> to that class, and the runner used an RAII guard from active.lock(). There is now one place that owns the state transition, waiting, timed waiting, notification, and lock lifetime.

The old generic atomic_lock remained for simple atomic-boolean guards, but its acquisition loop was fixed. The runner, which had the more complicated “state flag plus waits plus timeout” contract, moved to the mutex/condition-variable implementation.

Other fixes from the same sweep

Once I was already running sanitizers and reading the surrounding synchronization, a few other fixes came along with the PR.

Do not run teardown from a signal handler

The SIGINT handler sometimes called clean_quit(0) directly. That function stops and joins threads, writes configuration, logs, formats strings, touches the terminal, and walks through code that can allocate. Those are not operations you can safely execute from an asynchronous POSIX signal handler.

3843042 changed SIGINT to set state and call Input::interrupt(). That helper is just kill(getpid(), SIGUSR1), so normal program flow wakes up and performs the actual shutdown outside the asynchronous handler.

An Intel GPU failure-path leak

327f695 freed gpu_device_name on two early-return paths and freed the discovered engine structure when PMU initialization failed. This was separate from the locking issue, but it came from the same sanitizer pass.

The sequence of fixes

The lock had already been replaced once

The repository history adds useful context. In October 2021, btop replaced atomic-bool spinlocks with mutexes in commit 804fe60, explicitly to fix a rare deadlock. Three days later, commit 1601422 reverted the mutexes back to custom atomic-bool locks. The compare-exchange form that survived into 2026 dates back to that period, with later changes adding atomic wait/notify.

That does not make the original decision irrational. Hot-path locking, portability, older compiler/library behavior, and wanting a tiny state primitive are all plausible reasons to reach for atomics. The problem is that once you build a mutex-like abstraction yourself, you inherit the mutex contract: exactly one owner, correct publication of protected state, correct wakeups, correct lifetime semantics, and a memory model that still works on architectures less forgiving than the machine on your desk.

Atomics are small in syntax and large in semantics.

Why did it work for so long?

Because “contains a race” and “crashes every run” are nowhere near the same statement.

• The compare-exchange bug needs actual contention on a wait=true path.
• The relaxed-ordering bug needs overlapping non-atomic accesses where code relies on the atomic transition for synchronization.
• Scheduling changes from CPU count, terminal activity, I/O timing, optimization level, sanitizers, and kernel behavior all move those windows around.
• x86's relatively strong hardware memory model can make some bad C++ memory-ordering code look healthier than it is. The language-level data race is still undefined behavior.
• Even when two threads enter together, visible corruption depends on what they touch and when.

That is why ThreadSanitizer was useful here. It does not need the race to turn into a human-visible crash. It reports conflicting accesses without a valid happens-before relationship. The tool got me to the neighborhood; reading the actual synchronization code found the broken lock.

Why this matters for an OSS project

btop is widely used and has years of development behind it. This lock still stayed wrong for years.

I do not think the lesson is “how did the maintainers miss this?” The useful lesson is the opposite: this is what open source looks like when a project has far more code paths, platforms, hardware combinations and users than maintainer attention.

If you use btop, contribute in whatever way fits. Fix an issue. Reproduce a crash. Run a sanitizer build. Review a concurrency change. Improve a test. Sponsor the project.

I started with a crash I could barely reproduce. A year later, following the sanitizer reports and the synchronization around them led to the broken lock and the races in #1042.