mirror of
https://github.com/agessaman/MeshCore.git
synced 2026-08-29 03:18:30 +00:00
fix(mqtt): address review — stopped clients, late allocation, fail-open map check
Three defects found reviewing the preceding commits.
1. reconnectSlotClient() stranded a STOPPED client, reintroducing the very bug
this branch fixes. It only rebuilt when isStarted() was true and otherwise
fell through to reconnect(), which is a documented no-op on a stopped client
— so nothing restarted it, at any rung, including the breaker probe. The
WiFi-transition teardown reaches exactly this state: it calls the hard
disconnect(), clearing _started while initial_connect_done stays set, so
after WiFi returned the slot could never come back. Now a stopped client is
started with connect() before the rebuild/reuse decision is considered.
The post-NTP credential refresh had the same exposure — it called
reconnect() directly — so it now goes through the helper too, still reusing
the transport since its fault is stale credentials, not the transport.
2. Allocating the neighbors buffer on first use let a stopped bridge allocate.
A neighbour discovery started before a stop can complete after it, and
neither caller rechecks bridge state, so requestPublishNeighbors() would
allocate 4 KB after releaseRuntimeBuffers() had already run and strand
_neighbors_publish_pending with no task to consume it. end() then returns
early on !_initialized, retaining the buffer until a later begin/end or a
reboot. Guarded on isRunning(), the same flag end() checks.
The release/acquire handoff itself was confirmed sound: the allocation and
copy precede the release store, and the task loop reads the pointer only
after its acquire load, so a half-published pointer is not observable.
3. The post-link map check failed open, contradicting the fail-closed claim in
its own commit message. A missing map, an unrecognised map format, or a
partial archive list each warned and passed; and it hardcoded firmware.map
while the post-action target used ${PROGNAME}, so a renamed program could
inspect a stale or absent file and still succeed. All four now fail the
build, and it requires every one of the four archives to appear rather than
at least one.
Rebuilt Heltec_v3_repeater_observer_mqtt, Heltec_v3_repeater and
heltec_v4_repeater_observer_mqtt; the opt-in path still reports all 4 archives
linked from .mbedtls-4k/.
(cherry picked from commit 5b5f076e5e165997e8050f2be061c7c67340fcf7)
This commit is contained in:
+33
-8
@@ -102,11 +102,27 @@ print("reduced-TLS: linking mbedTLS from %s (verified)" % staged)
|
||||
|
||||
|
||||
def _verify_map(source, target, env):
|
||||
"""Confirm every mbedTLS archive in the link came from our directory."""
|
||||
map_path = os.path.join(env.subst("$BUILD_DIR"), "firmware.map")
|
||||
"""Confirm every mbedTLS archive in the link came from our directory.
|
||||
|
||||
Fails closed. Anything that stops this from *proving* the link — no map, an
|
||||
unparsable map, a short archive list — is a failure, not a warning. A warning
|
||||
here would leave exactly the hole the check exists to close: an opt-in build
|
||||
that succeeds while silently linking the framework's 16 KiB buffers.
|
||||
"""
|
||||
# Derive the map name from PROGNAME rather than hardcoding firmware.map, so a
|
||||
# renamed program cannot leave us inspecting a stale or absent file.
|
||||
map_path = os.path.join(env.subst("$BUILD_DIR"),
|
||||
env.subst("${PROGNAME}") + ".map")
|
||||
if not os.path.isfile(map_path):
|
||||
print("reduced-TLS: WARNING no firmware.map, cannot confirm the link",
|
||||
legacy = os.path.join(env.subst("$BUILD_DIR"), "firmware.map")
|
||||
map_path = legacy if os.path.isfile(legacy) else map_path
|
||||
if not os.path.isfile(map_path):
|
||||
print("\n*** reduced-TLS: no linker map at %s ***" % map_path,
|
||||
file=sys.stderr)
|
||||
print("Cannot prove the reduced-TLS archives were linked. Ensure the env "
|
||||
"emits a map (-Wl,-Map), or unset MESHCORE_REDUCED_TLS.",
|
||||
file=sys.stderr)
|
||||
env.Exit(1)
|
||||
return
|
||||
# The map records whatever the linker was given, which for a -L hit is a path
|
||||
# relative to the linker's cwd (the project dir). Resolve before comparing, or
|
||||
@@ -133,12 +149,21 @@ def _verify_map(source, target, env):
|
||||
for path in sorted(stray):
|
||||
print(" " + path, file=sys.stderr)
|
||||
env.Exit(1)
|
||||
if not seen:
|
||||
print("reduced-TLS: WARNING firmware.map names no mbedTLS archive",
|
||||
file=sys.stderr)
|
||||
return
|
||||
print("reduced-TLS: confirmed %d archives linked from %s"
|
||||
% (len(seen), staged))
|
||||
# Every required archive must appear. Seeing only some of them means the rest
|
||||
# resolved somewhere this parse did not recognise, which is not proof of anything.
|
||||
missing = [name for name in REQUIRED if name not in seen]
|
||||
if missing:
|
||||
print("\n*** reduced-TLS: %s names only %d of %d archives ***"
|
||||
% (os.path.basename(map_path), len(seen), len(REQUIRED)),
|
||||
file=sys.stderr)
|
||||
print(" missing: " + ", ".join(missing), file=sys.stderr)
|
||||
print("Either the map format changed or mbedTLS was resolved elsewhere; "
|
||||
"the reduced buffers cannot be assumed.", file=sys.stderr)
|
||||
env.Exit(1)
|
||||
return
|
||||
print("reduced-TLS: confirmed all %d archives linked from %s"
|
||||
% (len(REQUIRED), staged))
|
||||
|
||||
|
||||
env.AddPostAction("$BUILD_DIR/${PROGNAME}.elf", _verify_map)
|
||||
|
||||
@@ -1992,6 +1992,23 @@ void MQTTBridge::teardownSlot(int index) {
|
||||
slot.last_deferred_log_ms = 0;
|
||||
}
|
||||
|
||||
// A stopped client needs connect(): reconnect() is a documented no-op on one, so reaching
|
||||
// it here would strand the slot. The WiFi-transition teardown stops a client while leaving
|
||||
// initial_connect_done set, so the ladder does see this state.
|
||||
void MQTTBridge::reconnectSlotClient(int index) {
|
||||
if (index < 0 || index >= RUNTIME_MQTT_SLOTS) return;
|
||||
MQTTSlot& slot = _slots[index];
|
||||
if (slot.client == nullptr) return;
|
||||
|
||||
if (!slot.client->isStarted()) {
|
||||
MQTT_DEBUG_PRINTLN("MQTT%d start (client was stopped)", index + 1);
|
||||
slot.client->connect();
|
||||
return;
|
||||
}
|
||||
slot.client->reconnect();
|
||||
}
|
||||
|
||||
|
||||
void MQTTBridge::maintainSlotConnections() {
|
||||
if (!_identity) return;
|
||||
|
||||
@@ -3637,6 +3654,11 @@ void MQTTBridge::requestPublishNeighbors(const char* json, size_t len) {
|
||||
// Drop a new snapshot while one is still being published (Core 0 clears the
|
||||
// flag when done). Acquire pairs with the task loop's release store.
|
||||
if (_neighbors_publish_pending.load(std::memory_order_acquire)) return;
|
||||
// Allocating here means a stopped bridge must not: a discovery started before the
|
||||
// stop can finish after it, and releaseRuntimeBuffers() has already run, so the
|
||||
// allocation would be retained with no task left to consume it. isRunning() is the
|
||||
// same flag end() guards on.
|
||||
if (!isRunning()) return;
|
||||
// Allocated on first use so a node with neighbors off never pays the 4 KB.
|
||||
// Cross-core safe: the release store below publishes this pointer, and the task
|
||||
// loop only reads it after the matching acquire load.
|
||||
@@ -3959,7 +3981,9 @@ bool MQTTBridge::syncTimeWithNTP(bool force, bool primary_only) {
|
||||
if (createSlotAuthToken(i)) {
|
||||
_slots[i].client->setCredentials(_jwt_username, _slots[i].auth_token);
|
||||
}
|
||||
_slots[i].client->reconnect();
|
||||
// Reuse the transport — the fault is stale credentials, not the transport —
|
||||
// but via the helper, so a stopped client is started rather than no-opped.
|
||||
reconnectSlotClient(i);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -447,6 +447,9 @@ private:
|
||||
int activatedSlotCount() const;
|
||||
bool canActivateSlot(int index) const;
|
||||
void teardownSlot(int index); // Disconnect the slot's client (keeps the object alive)
|
||||
// Reconnect a slot, starting it instead when the client is stopped (reconnect() is a
|
||||
// no-op on a stopped client). See the definition.
|
||||
void reconnectSlotClient(int index);
|
||||
void maintainSlotConnections(); // Maintain all slot connections (token renewal, reconnect)
|
||||
void maintainSlotConnection(int index, unsigned long now_millis, unsigned long current_time, bool time_synced, bool& reconnect_attempted, bool& teardown_attempted);
|
||||
bool createSlotAuthToken(int index); // Create/renew JWT token for a slot
|
||||
|
||||
Reference in New Issue
Block a user