fix use-after-free of a blackboard entry held by getAnyLocked

[aysha-afrah26]

Blackboard::getAnyLocked builds the returned AnyPtrLocked from a shared_ptr<Entry> that is a local variable, so the caller ends up holding a raw Any* and a raw std::mutex* into an entry that nothing keeps alive. unset() and clear() take storage_mutex_ but never entry_mutex, so erasing the key drops the last reference and destroys the entry while its mutex is still locked, which makes the next read through the pointer a heap-use-after-free and leaves ~LockedPtr unlocking a mutex that is already gone. The path I ran into it on is the Groot2 server thread, where ExportBlackboardToJSON calls getAnyLocked per key while an UnsetBlackboard action on the tick thread frees the entry underneath it.

AnyPtrLocked is returned by value from exported functions and its size is not part of the mangled symbols, so it can't grow an owner handle without silently breaking existing binaries (an earlier revision did that, see review). The fix therefore keeps LockedPtr at its original layout and coordinates entry removal inside Blackboard: unset(), clear(), the stale-key removal in cloneInto() and ~Blackboard() take the entry out of the storage and destroy it only if its entry_mutex can be acquired with try_lock(); otherwise the entry is parked in a list inside blackboard.cpp and destroyed by a later removal that finds it unlocked. getAnyLocked() re-checks the key under the lock, so an entry can't be dropped between lookup and lock. Nothing blocks, so removing an entry while holding a lock on it, from the same or another thread, can't deadlock. unset() and the destructor move out of the header so future changes to this path reach callers through the library.

Regression tests cover unset(), clear(), cloneInto() and destruction of the blackboard while an AnyPtrLocked is alive, two threads each removing the entry the other one holds, and the deferred destruction of a released entry. Each of them reports a heap-use-after-free under ASan on the unpatched code (the last one fails outright) and passes with the patch; the full suite is 517/517.

[facontidavide]

I don't understand how this isn't crashing

[aysha-afrah26]

Mostly because freed memory tends to keep looking valid. When unset() erases the key, the Entry block goes back to the allocator but nothing scrubs it, so reading through the stale Any* just returns the old bytes, and unlocking the already-destroyed std::mutex writes into memory that still holds a plausible mutex state. Both are UB that silently "works" until the allocator hands that block to someone else. It also needs a key to actually be erased while a LockedPtr is alive, which in practice means the Groot2 server thread dumping the blackboard while the tick thread runs UnsetBlackboard, so it shows up as a rare crash or garbage in the dump under load rather than a deterministic segfault.

ASan makes it deterministic. On current master, building with -DBTCPP_ENABLE_ASAN=ON and running the test from this PR (--gtest_filter=BlackboardTest.AnyPtrLockedSurvivesUnset) reports a heap-use-after-free: a read 40 bytes into the freed 232-byte Blackboard::Entry (the any::empty() check inside cast()), freed by unset() erasing the map entry and dropping the last shared_ptr. With the patch the same test passes clean.

[aysha-afrah26]

any update?

[fallenmi]

The lifetime fix works for newly rebuilt callers, but adding owner_ breaks the ABI of the existing shared-library API. LockedPtr<Any> is public and is returned by value from the exported Blackboard::getAnyLocked() functions; the return type is not part of their mangled symbols. This patch changes sizeof(AnyPtrLocked) from 16 to 32 bytes, so an already compiled caller still provides a 16-byte return slot while the new library constructs the added shared_ptr into the following stack memory.

I reproduced this at exact head 2e934c777cd6f690ca66960c4826da81cf8523b8 against base 879522c75cce5e67f3dd7b5591bcb96eb3557a42 with ASan/UBSan:

• base headers + base library: passes, value=42 size=16;
• the same base-header client + this head library: ASan stack-buffer-overflow, writing from the shared_ptr<const void> move constructor called by LockedPtr at locked_reference.hpp:31; ASan identifies the overflow immediately after the caller's 16-byte locked return slot;
• head headers + head library: passes, value=42 size=32.

The source-level regression test is otherwise sound: the identical AnyPtrLockedSurvivesUnset oracle aborts on base with a heap-use-after-free and passes on head, and all 28 Blackboard/thread-safety tests pass on head under ASan/UBSan. Please keep that lifetime fix without changing the layout of this return-by-value public type (for example, by coordinating entry removal inside Blackboard), or handle the ABI break explicitly instead of making existing binaries memory-unsafe.

Reviewed by OpenAI Codex for @fallenmi. No repository files were changed on GitHub.

[aysha-afrah26]

Good catch. The return type isn't part of the mangled symbols, so the size change is a silent break for anything compiled against the old headers, which is worse than the bug it fixes.

Reworked it the way you suggested: LockedPtr is back to its original layout and the lifetime is handled inside Blackboard instead. unset(), clear() and the removal path of cloneInto() now move the entry out of the map and wait on entry_mutex before it can be destroyed, and getAnyLocked() re-checks the key under the lock so an entry can't be dropped between lookup and lock. The re-check takes entry_mutex then storage_mutex_, the same order the script evaluation path already uses (documented in cloneInto), so no new lock inversion.

One behavior change worth flagging: unset() now blocks until an outstanding AnyPtrLocked on that entry is released, so holding one and calling unset() for the same key on the same thread deadlocks instead of reading freed memory. The regression test runs unset() from a second thread accordingly; it still trips ASan with the same heap-use-after-free on unpatched master and passes clean with the patch. Full suite is 512/512 on a Debug build, and the Blackboard tests are green under ASan/UBSan.

[fallenmi]

The return-object ABI is restored on exact head 0a812865, but existing 4.10 binaries still do not receive the lifetime fix because Blackboard::unset() is inline in the public header. I compiled the same ASan/UBSan client against tag 4.10.0 headers and linked it to the exact-head dylib. An ordinary call now passes (value=42 size=16), but concurrent old-inline unset() plus exact-head getAnyLocked() still reproduces the original heap-use-after-free in the 232-byte Blackboard::Entry. nm confirms that getAnyLocked() resolves from the new library while unset() remains a local client symbol. The layout is ABI-compatible again, but the behavioral fix is not applied to the already compiled callers that motivated this rewrite.

For rebuilt callers, making lifetime depend solely on a currently held mutex also adds unbounded waits and leaves other lifetime holes. On this head, each of same-thread unset(), clear(), and removal through cloneInto() timed out while an AnyPtrLocked was held; two threads holding entries A/B and each unsetting the other’s key also deadlocked. Destroying the Blackboard while the holder remained still produced an ASan heap-use-after-free, and the balanced public sequence unlock() -> unset() -> lock() aborted on the freed mutex. The previous owner-carrying head fixed these cases but broke the return layout.

Please keep the released layout without relying only on mutex ownership for entry lifetime, and add regressions for an old-header/new-library client plus teardown/temporary-unlock and deadlock cases. The focused exact-head Blackboard suites are otherwise green under ASan/UBSan (28/28) and TSan (27/27).

Review prepared with OpenAI Codex for @fallenmi; the exact revisions and mixed-version client were reproduced locally.

[aysha-afrah26]

Reworked the removal side so nothing waits on the mutex anymore. unset(), clear(), the stale-key removal in cloneInto() and now ~Blackboard() take the entry out of the storage and try_lock() it: if that succeeds the entry is destroyed right away, otherwise it's parked in a list inside blackboard.cpp and destroyed by the next removal that finds it unlocked. getAnyLocked() keeps the re-check under the lock, so an entry can't disappear between lookup and lock. That covers every blocking case you listed: same-thread unset()/clear()/cloneInto() while holding the lock and the two-thread A/B cross-unset all complete now, and a holder that outlives the blackboard reads valid memory. There's a test for each of those plus one that checks the parked entry really is freed once the lock is released. All six trip ASan (or fail outright) on the unpatched sources and pass with the patch; the Blackboard suites are clean under ASan/UBSan and the full suite is 517/517.

On the old-header client: agreed that an already compiled inline unset() can't pick up a fix, but that's true of any change to a header-inline function and it's the same behavior as current master, not a regression from this patch. I've moved unset() and the destructor out of the header so this path is fixable through the library from here on. A mixed-version client isn't something the gtest suite can express (it needs a second copy of the headers at a pinned tag and a separate link step), so I haven't added one there.

On unlock() -> unset() -> lock(): with the 16-byte layout, the lock state is the only thing the handle communicates back to the library, so nothing can pin the entry while the caller has explicitly released the lock. Only an owning handle makes that pattern safe, and that's the layout change. Nothing in the tree calls LockedPtr::unlock() directly; I've documented the limit in the LockedPtr header comment instead.