mirror of
https://github.com/logos-co/logos-protocol.git
synced 2026-08-30 21:41:10 +00:00
Dario Gabriel LipicarandClaude Opus 5
731e579064
Joining the per-call waiters (rather than detaching them) closed a real
use-after-free: the waiter captures `this`, and release() used to `delete this`
underneath it. But joinWaiters() could only join. It had no way to ASK a waiter
to stop, so destroying a PlainLogosObject with a call in flight blocked for the
remainder of that call's timeout — up to 20s on the protocol default. A module
unloading mid-call stalled the unload for that long, on the releasing thread.
Measured, 8s call timeout, provider parked:
release() before after
future wait (site 1) 7804 ms 11 ms
deferred completion (site 2) 7703 ms 0 ms
Both blocking sites are now interruptible, and they need different treatment:
* the std::future wait cannot be interrupted at all, so it is SLICED: one
deadline computed up front, waited in 25ms increments, stop flag checked
between them. Teardown latency is one slice; the timeout the caller asked
for is unchanged, because the last slice ends exactly on the deadline. 25ms
is under two frames (so a module unload stays imperceptible) and costs 40
wakeups/second per in-flight call, which is nothing beside the Qt event loop
these threads already sit next to.
* awaitCompletion's condition_variable is interruptible by construction:
widen the predicate, notify_all. No latency floor at all — hence 0 ms. The
flag is published under m_completionMu so a waiter cannot evaluate the
predicate, decide to sleep, and then miss the notify.
A CANCELLED CALL STILL DELIVERS, EXACTLY ONCE. This is the part a naive fix
breaks: callMethodAsyncWithError and lp_invoke_async promise the callback fires
exactly once, so a waiter that simply returns on stop trades a bounded stall for
an unbounded hang in every caller awaiting it. Proven by building that naive
variant: it passes the latency test and fails three exactly-once tests with the
callback never arriving.
The code is "transport_error", from the existing vocabulary rather than a new
one, since these codes are the wire contract. logos_call_error.h defines it as
"the connection failed or was torn down mid-call", which is precisely what
happened — the consumer tore its own end down. The alternatives all misattribute
it: "object_unavailable" says the module is absent (it is not, and callers
re-acquire on that code), "call_failed" blames the peer for a dispatch it
performed fine, and "timeout" — what this used to report, after waiting the
deadline out — claims a deadline elapsed that did not. It is also already what
the wire produces for the same event seen from the other side (callErrorFromWire
maps TRANSPORT_CLOSED to transport_error).
Delivering during teardown is safe because postToQtEventLoop touches nothing
owned by the object: it is a free function taking the callback, value and error
BY VALUE, and the waiter copies objectName/method up front. That was already
true and is now load-bearing, so it is documented at the function. The queued
lambda runs after the object may be gone; everything the waiter reaches through
`this` runs before the join returns, which is why the join must stay.
Also closes the registration window it opens: a call arriving after the stop
would push a thread onto an m_waiters that teardown has already swapped out, so
it would never be joined. It is answered as cancelled instead.
The UAF is verified still closed under macOS Guard Malloc rather than ASan —
libclang_rt livelocks in its own initializer before main on this toolchain, for
both ASan and TSan, on a hello-world. Under Guard Malloc the race test is clean
across 5 runs and SIGSEGVs immediately when the join is turned back into a
detach, so the check is a real detector and not a vacuous pass.
Tests: 277/277 (was 270; 7 new). CallErrorAfterAcquireTest hammered 40x, 0
failures — it was ~2/50 flaky before this branch's earlier fixes.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>