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; the test added here does the same sequence and trips ASan on current master.

The fix keeps LockedPtr at its original layout (an earlier revision stored a shared_ptr owner inside it, but that changes the size of a public type returned by value from exported functions and silently breaks ABI for existing binaries, as pointed out in review). Instead, entry removal is coordinated inside Blackboard: unset(), clear() and the removal path of cloneInto() move the entry out of the map first and wait on entry_mutex before letting it be destroyed, and getAnyLocked() re-checks the key under the lock so the entry cannot be dropped between lookup and lock. The re-check acquires entry_mutex then storage_mutex_, the same order already used by the script evaluation path.

One consequence: unset() now blocks until an outstanding AnyPtrLocked on that entry is released, so holding one and unsetting the same key from the same thread deadlocks rather than reading freed memory. The regression test runs unset() from a second thread for that reason; it still reports the same heap-use-after-free on unpatched master under ASan and passes clean with the patch.

[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.