fix: stop tryAcquireNow leaving a dangling facade in QtRO's connect list

e9f82ac introduced a use-after-free. tryAcquireNow() acquired a dynamic replica
and, when it was not already Valid, deleted it. That is not safe: QtRO shares one
replica IMPLEMENTATION per object name per node, and while that implementation is
still waiting for the source's metaobject it records every facade built on it as a
RAW pointer in QConnectedReplicaImplementation::m_parentsNeedingConnect.
~QRemoteObjectReplica is an empty body, so destroying a facade never deregisters
it, and the implementation dereferences the whole list when the class definition
arrives.

So each probe of an unreachable module left one dangling pointer behind.

WHY IT HID. The first probe owns the only implementation and takes it down with
itself, so a single subscription is harmless. It needs a second subscription whose
implementation is pinned by an in-flight PendingAcquire before a freed facade can
outlive its implementation. A consumer subscribing once sees nothing; the QML
plugin shape -- a view registering every event it cares about up front -- dies.

REPRODUCED, 4 runs of 4, serially as well as in parallel, in
logos-view-module-runtime's existing suite (unchanged from master, and green there
against this same protocol checkout):

  LogosQmlBridge: subscription accepted for "echo_module" :: "ev13"
  Received signal 10 (SIGBUS), code 1, for address 0x5a

SIGBUS code 1 is BUS_ADRALN -- a misaligned atomic access on a garbage base read
out of a recycled heap block, in the event loop rather than at the call site,
which is why it reads as a mystery crash rather than as a subscription bug.

PROVEN, before writing this fix, by commenting out that single `delete replica`:
the same suite went 4 failures -> 6/6 with no other change. With this fix: 6/6.

THE FIX IS TO PARK, NOT TO FREE. One probe per object name, parented to
m_pendingAcquires -- which both the destructor and reconnect() already destroy
BEFORE the node, so the implementations die in the same breath and freeing them
there is safe. Ownership transfers out only when the replica reaches Valid, by
which point the implementation is configured and is no longer holding the facade.
It costs one idle replica per name until it goes Valid or the connection dies.

AND REMOVE THE MULTIPLIER: beginAcquire() probed on EVERY add(), ahead of
startAcquire() and therefore ahead of the m_acquiring one-acquire-per-object
guard. tick() already applies that filter; beginAcquire() was the one caller that
did not, which is what turned one probe per module into one per subscription.
While an acquire is in flight its PendingAcquire already holds a replica and will
arm every waiting entry at once, so the probe buys nothing there.

Not QML-specific: lp_subscribe reaches the same entry point.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Dario Gabriel Lipicar
2026-08-10 08:28:54 -03:00
co-authored by Claude Opus 5
parent e9f82ac688
commit 09f684fb94
3 changed files with 67 additions and 14 deletions
@@ -668,7 +668,9 @@ RemoteTransportConnection::RemoteTransportConnection(const QString& registryUrl)
RemoteTransportConnection::~RemoteTransportConnection()
{
// Pending replicas belong to the node; kill them FIRST so none outlives it.
// Pending replicas and parked probes belong to the node; kill them FIRST so
// none outlives it.
m_probes.clear();
delete m_pendingAcquires;
m_pendingAcquires = nullptr;
delete m_node;
@@ -728,6 +730,7 @@ bool RemoteTransportConnection::reconnect()
// Drop them first; their callbacks are cancelled with them, so the
// consumer's subscription registry re-arms against the new node
// (LogosAPIConsumer::reconnect -> reconnected()).
m_probes.clear();
delete m_pendingAcquires;
m_pendingAcquires = new QObject();
delete m_node;
@@ -838,20 +841,48 @@ LogosObject* RemoteTransportConnection::tryAcquireNow(const QString& objectName)
// whether this node is wired to the endpoint, not whether the peer is up.
if (!m_connected) return nullptr;
QRemoteObjectReplica* replica = m_node->acquireDynamic(objectName);
if (!replica) return nullptr;
// The ONLY case worth answering. A replica that is not already Valid would
// have to be waited on, and waiting is exactly what this must not do —
// requestObjectWhenAvailable owns that. Note QtRO shares one replica
// implementation per object name on a node, so when the caller already has
// a live handle for this module (it just made a call through it) this is
// Valid on the spot and costs no round trip.
if (replica->state() != QRemoteObjectReplica::Valid) {
delete replica;
return nullptr;
// PARK the probe; never free it while it is unconfigured. QtRO shares one
// replica implementation per object name on a node, and while that
// implementation is still waiting for the source's metaobject it records
// every facade built on it as a RAW pointer in m_parentsNeedingConnect.
// ~QRemoteObjectReplica is an empty body, so destroying a facade does not
// deregister it — and the implementation dereferences the whole list when
// the class definition finally arrives.
//
// The earlier acquire-then-delete form was therefore a use-after-free that
// left one dangling pointer per probe. It survived a single subscription
// (the probe owned the only implementation and took it down with itself)
// and crashed once a second subscription shared an implementation pinned by
// an in-flight PendingAcquire: SIGBUS with BUS_ADRALN, in the consumer's
// event loop rather than at the call site.
//
// Parking costs one idle replica per name until it goes Valid or the
// connection dies. Parented to m_pendingAcquires, which both ~RemoteTransportConnection
// and reconnect() destroy BEFORE the node — the ordering is what makes
// freeing them safe, since the implementations die in the same breath.
QPointer<QRemoteObjectReplica>& probe = m_probes[objectName];
if (!probe) {
QRemoteObjectReplica* fresh = m_node->acquireDynamic(objectName);
if (!fresh) {
m_probes.remove(objectName);
return nullptr;
}
if (m_pendingAcquires) fresh->setParent(m_pendingAcquires);
probe = fresh;
}
// Not Valid means "would have to be waited on", and waiting is exactly what
// this must not do — requestObjectWhenAvailable owns that. Leave the probe
// parked and answer no.
if (probe->state() != QRemoteObjectReplica::Valid)
return nullptr;
// Valid: the implementation is configured, so it is no longer holding this
// facade in m_parentsNeedingConnect and handing ownership over is safe.
QRemoteObjectReplica* replica = probe;
m_probes.remove(objectName);
replica->setParent(nullptr);
g_acquireCount.fetch_add(1, std::memory_order_relaxed);
return new RemoteLogosObject(replica, objectName);
}
@@ -3,10 +3,13 @@
#include "../../logos_transport.h"
#include "../../logos_object.h"
#include <QHash>
#include <QPointer>
#include <QString>
class QRemoteObjectRegistryHost;
class QRemoteObjectNode;
class QRemoteObjectReplica;
class RemoteTransportHost : public LogosTransportHost {
public:
@@ -54,6 +57,19 @@ private:
// it cancels them; it is reset explicitly at the TOP of the destructor so
// pending replicas die before the node they belong to.
QObject* m_pendingAcquires;
// Parked probes for tryAcquireNow(), one per object name, each parented to
// m_pendingAcquires so they die with it — BEFORE the node, in both the
// destructor and reconnect().
//
// They are parked rather than freed because destroying a dynamic replica
// whose shared implementation has not yet received the source's metaobject
// leaves a DANGLING RAW POINTER inside that implementation: QtRO records
// each such facade in QConnectedReplicaImplementation::m_parentsNeedingConnect
// and ~QRemoteObjectReplica is an empty body that never deregisters. The
// implementation then dereferences every entry when the class definition
// arrives. Probing repeatedly and freeing each probe is therefore a
// use-after-free with one dangling pointer per probe.
QHash<QString, QPointer<QRemoteObjectReplica>> m_probes;
// Does the registry endpoint currently have a listener?
//
// Separate from m_connected because connectToNode() cannot answer it: it
+7 -1
View File
@@ -230,7 +230,13 @@ private:
// window. tryAcquireNow() never blocks and answers nullptr whenever it
// would have to wait, so the deferred path below still owns every case
// where the module is not already there.
if (auto* async = dynamic_cast<LogosTransportAsyncAcquire*>(m_transport)) {
// Skipped while an acquire for this name is already in flight: that
// PendingAcquire holds a replica and will arm every waiting entry at
// once, so probing again buys nothing and only churns replicas. tick()
// already applies this filter; this was the one caller that did not,
// which is what turned one probe per module into one per subscription.
if (auto* async = dynamic_cast<LogosTransportAsyncAcquire*>(m_transport);
async && !m_acquiring.contains(objectName)) {
if (LogosObject* now = async->tryAcquireNow(objectName)) {
armAgainst(objectName, now);
return true;