conflict-set: move-assignment leaks the old impl

Continuing the theme of reflecting on how correctness issues arise, this one is a different flavor than the previous two. It is not a fault in the conflict-detection logic itself but a fault in the resource management around the ConflictSet handle: the move-assignment operator leaked the old internal implementation object. It was found by static inspection before anyone reported a symptom, which is exactly when you want to find a leak.

🔗 Summary

In every released version, the move-assignment operator of ConflictSet overwrote the internal impl pointer without first destroying the implementation it replaced, leaking all memory and resources owned by the left-hand side. Confirmed under LeakSanitizer: move-assigning a populated conflict set onto another leaks 3552 bytes across 6 allocations.

Affected versions: v0.0.1 through v0.0.13 (every released version). The fix is on main and will ship in the next release.

🔗 Timeline

🔗 2024-01-17

The bug is introduced with the initial PImpl scaffolding (commit c6ff1ff). The move constructor is correct — a freshly constructed object has no existing impl to destroy — but the same one-line body is reused for the move-assignment operator, which is not.

🔗 2024-03-29

v0.0.1 is released, the first release to contain the bug.

🔗 2026-06-21

weaselbot reports the bug by inspection. weaselbot is an autonomous contributor bot that, among other things, runs a "bug hunt" feature: it spends surplus weekly LLM tokens reading our repositories and filing issues for plausible defects it spots by inspection. The report pointed directly at the move-assignment operator, explained the leak, and included a minimal reproducer (issue #54).

🔗 2026-06-22

Root cause confirmed, fixed, and merged in PR #55.

🔗 Details

The shipped implementation (the radix tree in ConflictSet.cpp) defined move-assignment like this:

ConflictSet &ConflictSet::operator=(ConflictSet &&other) noexcept {
  impl = std::exchange(other.impl, nullptr);
  return *this;
}

The move constructor is fine because a newly constructed object has no existing impl to destroy, but move-assignment must first release the implementation owned by the left-hand side. Here is how to reproduce the leak. The report's reproducer advanced the version without writing anything, which allocates no tree state, so the version below writes a key so that impl actually owns something to leak:

weaselab::ConflictSet a(0);
uint8_t k = 'x';
weaselab::ConflictSet::WriteRange w{ {&k, 1}, {&k, 0} };
a.addWrites(&w, 1, 1);        // a.impl now owns some tree state

weaselab::ConflictSet b(1);
a = std::move(b);             // a.impl is overwritten without being destroyed.
                              // The tree state a used to own is leaked.

Building this against the last affected version and running it under AddressSanitizer with leak detection reports:

SUMMARY: AddressSanitizer: 3552 byte(s) leaked in 6 allocation(s).

and getBytes() on a drops to the size of b's empty state, confirming the old tree was orphaned. After the fix, the same program reports no leak.

Because the destructor does destroy impl when it is non-null, an ordinary moved-from object is left correctly empty (its impl was handed over). The defect is purely in the path that replaces a non-empty impl with another.

The report also flagged self-move-assignment. That case is a no-op: impl and other.impl alias, so std::exchange leaves impl unchanged. cppreference explicitly documents this self-move-assignment behavior, and LeakSanitizer confirms it: no leak, and the object remains usable.

The fix destroys the existing impl before taking ownership of other.impl, and guards against self-assignment for clarity:

ConflictSet &ConflictSet::operator=(ConflictSet &&other) noexcept {
  if (this != &other) {
    if (impl) {
      internal_destroy(impl);
    }
    impl = std::exchange(other.impl, nullptr);
  }
  return *this;
}

This bug cannot affect a program that never move-assigns an already-populated ConflictSet, and it never produces a wrong conflict-detection result — it only leaks memory (on ordinary move-assignment). That narrowness is part of why it survived so long.

🔗 Root Cause Analysis

The root cause was a sloppy copy-paste and missing test coverage: the move-assignment operator was copied verbatim from the move constructor, and no test ever move-assigned a populated ConflictSet.

🔗 Why not caught sooner?

The move-assignment operator was copied from the move constructor, which is correct, but the difference between construction (no existing state to release) and assignment (existing state that must be released) was never revisited. There is no test that move-assigns a populated ConflictSet, so nothing exercised the defective path. The conflict-detection fuzzing that has caught the previous bugs does not touch the value-semantic surface of the ConflictSet handle at all, so it could not have found this.

🔗 How was it discovered?

Unlike the previous two incidents, this was not discovered through a runtime symptom reported by a downstream project. It was found by static inspection of the move-assignment operator (issue #54), which pointed directly at the operator, explained the leak, and provided a minimal reproducer. So the path from report to root cause was short: the report was the root cause analysis.

🔗 Prevention

Add a regression test that move-assigns a populated ConflictSet into another and asserts the old state is released, run under a leak sanitizer so a regression fails the build. A closely related report (issue #48, fixed in PR #51) closed a sibling hole: ConflictSet was implicitly copyable in C++98/C++03, which would double-free. Both are consequences of managing impl by raw pointer¹ rather than letting the type system enforce the invariants.

🔗 What went well

• Caught by inspection before any user hit it, with a minimal reproducer included in the report
• The fix is small and localized
• The report distinguished the correct move constructor from the incorrect move-assignment operator, so the scope was clear from the start
• A leak caught by static inspection is much cheaper than a downstream memory-pressure report would have been