refactor(mqtt): optimize MQTT status and diagnostic reply formatting

Replace multiple snprintf calls with a single replyAppendf function to
improve buffer management and prevent overflow. This change enhances
the readability and maintainability of the code while ensuring that
the reply buffer is handled safely across various MQTT status and
diagnostic replies.

src/helpers/MQTTReplyFormat.h

@@ -0,0 +1,43 @@
+#pragma once
+
+#include <stddef.h>
+#include <stdarg.h>
+#include <stdio.h>
+
+// Bounded, clamping printf-append for the fixed-size CLI reply buffers used by
+// MQTTBridge's status/stats/diag formatters. Factored out of MQTTBridge so the
+// bound is provable on the host instead of holding "by input-size accident"
+// (see the A1 out-of-bounds-write finding, 2026-07-19 MQTT observer review).
+//
+// Appends `fmt...` to `buf` starting at offset `*pos`, then advances `*pos` by
+// the number of characters actually written, CLAMPED to [0, bufsize-1]. Because
+// snprintf returns the *would-have-written* length, the naive
+// `*pos += snprintf(buf + *pos, bufsize - *pos, ...)` idiom can push `*pos` past
+// `bufsize` after a truncated append; the next append then computes
+// `bufsize - *pos` as a huge size_t and `buf + *pos` past the end, writing out of
+// bounds. Clamping `*pos` here makes every subsequent append a safe no-op once
+// the buffer is full.
+//
+// buf is always left NUL-terminated (vsnprintf guarantees this for bufsize > 0).
+// No-ops on null buf/pos or bufsize == 0. A negative incoming *pos is treated as
+// 0. Typical use: `int pos = 0;` then a sequence of replyAppendf() calls.
+static inline void replyAppendf(char* buf, size_t bufsize, int* pos, const char* fmt, ...) {
+  if (!buf || !pos || bufsize == 0) return;
+  if (*pos < 0) *pos = 0;
+  // Full: no room for anything but the terminator. Keep buf NUL-terminated and
+  // leave *pos pinned at the last writable index.
+  if ((size_t)*pos >= bufsize - 1) {
+    *pos = (int)bufsize - 1;
+    buf[*pos] = '\0';
+    return;
+  }
+  size_t remaining = bufsize - (size_t)*pos;
+  va_list args;
+  va_start(args, fmt);
+  int n = vsnprintf(buf + *pos, remaining, fmt, args);
+  va_end(args);
+  // Encoding error: vsnprintf still NUL-terminated buf + *pos; leave *pos as-is.
+  if (n < 0) return;
+  *pos += n;
+  if ((size_t)*pos >= bufsize) *pos = (int)bufsize - 1;  // clamp truncated append
+}

src/helpers/bridges/MQTTBridge.cpp

@@ -2,6 +2,7 @@
 #include "../MQTTConnectionPolicy.h"
 #include "../MQTTMessageBuilder.h"
 #include "../MQTTPacketQueuePolicy.h"
+#include "../MQTTReplyFormat.h"
 #include "../MQTTRuntimeBufferLifecycle.h"
 #include "../MQTTTopicRouter.h"
 #include "../TxtDataHelpers.h"
@@ -241,8 +242,11 @@ void MQTTBridge::formatMqttStatusReply(char* buf, size_t bufsize, const MQTTPref
   q = b->_queue_count;
 #endif
 
-  int pos = snprintf(buf, bufsize, "> msgs: %s", msgs);
-  for (int i = 0; i < RUNTIME_MQTT_SLOTS && pos < (int)bufsize - 1; i++) {
+  // replyAppendf clamps pos into the buffer on every call, so no per-append
+  // guard or trailing clamp is needed (see MQTTReplyFormat.h / A1).
+  int pos = 0;
+  replyAppendf(buf, bufsize, &pos, "> msgs: %s", msgs);
+  for (int i = 0; i < RUNTIME_MQTT_SLOTS; i++) {
     const MQTTSlot& slot = b->_slots[i];
     const char* name = nullptr;
     const char* state = nullptr;
@@ -265,17 +269,13 @@ void MQTTBridge::formatMqttStatusReply(char* buf, size_t bufsize, const MQTTPref
       name = slot.preset ? slot.preset->name : "custom";
       state = "disc";
     }
-    pos += snprintf(buf + pos, bufsize - pos, ", %d: %s (%s)", i + 1, name, state);
+    replyAppendf(buf, bufsize, &pos, ", %d: %s (%s)", i + 1, name, state);
   }
-  // snprintf returns the would-be length, so a full buffer can push pos past
-  // bufsize; clamp before the remaining appends so bufsize - pos can't underflow.
-  if (pos >= (int)bufsize) pos = (int)bufsize - 1;
-  pos += snprintf(buf + pos, bufsize - pos, ", q:%d", q);
-  if (pos >= (int)bufsize) pos = (int)bufsize - 1;
+  replyAppendf(buf, bufsize, &pos, ", q:%d", q);
 
 #if defined(WITH_MQTT_NEIGHBORS)
   // Periodic neighbors: time to next publish + how the last one went.
-  if (obs && obs->mqtt_neighbors_enabled && pos < (int)bufsize - 1) {
+  if (obs && obs->mqtt_neighbors_enabled) {
     char when[16];
     switch (b->_neighbors_phase.load(std::memory_order_relaxed)) {
       case NBR_ACTIVE: strcpy(when, "active"); break;
@@ -291,7 +291,7 @@ void MQTTBridge::formatMqttStatusReply(char* buf, size_t bufsize, const MQTTPref
       case NBR_RESULT_FAIL: last = "failed"; break;
       default:              last = "none"; break;
     }
-    snprintf(buf + pos, bufsize - pos, ", nbr: %s/%s", when, last);
+    replyAppendf(buf, bufsize, &pos, ", nbr: %s/%s", when, last);
   }
 #endif
 }
@@ -324,14 +324,15 @@ void MQTTBridge::formatMqttStatsReply(char* buf, size_t bufsize) {
     if (b->_slots[i].client) outbox_total += b->_slots[i].client->getOutboxSize();
   }
 
-  int pos = snprintf(buf, bufsize, "> Free=%d Max=%d q:%d/%d Outbox=%u |",
-                     (int)ESP.getFreeHeap(), (int)ESP.getMaxAllocHeap(),
-                     q, MAX_QUEUE_SIZE, (unsigned)outbox_total);
-  for (int i = 0; i < RUNTIME_MQTT_SLOTS && pos < (int)bufsize - 1; i++) {
+  int pos = 0;
+  replyAppendf(buf, bufsize, &pos, "> Free=%d Max=%d q:%d/%d Outbox=%u |",
+               (int)ESP.getFreeHeap(), (int)ESP.getMaxAllocHeap(),
+               q, MAX_QUEUE_SIZE, (unsigned)outbox_total);
+  for (int i = 0; i < RUNTIME_MQTT_SLOTS; i++) {
     if (!b->_slots[i].enabled || !b->_slots[i].client) continue;
-    pos += snprintf(buf + pos, bufsize - pos, " s%d=%lu/%lu", i + 1,
-                    b->_slots[i].client->getPublishOk(),
-                    b->_slots[i].client->getPublishErr());
+    replyAppendf(buf, bufsize, &pos, " s%d=%lu/%lu", i + 1,
+                 b->_slots[i].client->getPublishOk(),
+                 b->_slots[i].client->getPublishErr());
   }
 }
 
@@ -467,18 +468,21 @@ void MQTTBridge::formatSlotDiagReply(char* buf, size_t bufsize, int slot_index)
     state = "disc";
   }
 
-  int pos = snprintf(buf, bufsize, "> mqtt%d: %s", slot_index + 1, state);
+  // replyAppendf clamps pos on every call, so the chained appends below can't
+  // walk past the reply buffer even if the accumulated text exceeds it (A1).
+  int pos = 0;
+  replyAppendf(buf, bufsize, &pos, "> mqtt%d: %s", slot_index + 1, state);
   if (slot.disconnect_count > 0) {
-    pos += snprintf(buf + pos, bufsize - pos, ", dc:%lu", (unsigned long)slot.disconnect_count);
+    replyAppendf(buf, bufsize, &pos, ", dc:%lu", (unsigned long)slot.disconnect_count);
     if (slot.first_disconnect_time > 0) {
       unsigned long first_disc_age_sec = (millis() - slot.first_disconnect_time) / 1000;
-      pos += snprintf(buf + pos, bufsize - pos, ", first_disc:%lus", first_disc_age_sec);
+      replyAppendf(buf, bufsize, &pos, ", first_disc:%lus", first_disc_age_sec);
     }
   }
 
   // If connected with no errors, we're done
   if (slot.connected && slot.last_error_time == 0) {
-    snprintf(buf + pos, bufsize - pos, ", no errors");
+    replyAppendf(buf, bufsize, &pos, ", no errors");
     return;
   }
 
@@ -488,30 +492,30 @@ void MQTTBridge::formatSlotDiagReply(char* buf, size_t bufsize, int slot_index)
     if (slot.last_tls_err != 0) {
       const char* desc = tlsErrorStr(slot.last_tls_err);
       if (desc) {
-        pos += snprintf(buf + pos, bufsize - pos, ", %s (0x%04X)", desc, (unsigned)slot.last_tls_err);
+        replyAppendf(buf, bufsize, &pos, ", %s (0x%04X)", desc, (unsigned)slot.last_tls_err);
       } else {
-        pos += snprintf(buf + pos, bufsize - pos, ", tls:0x%04X", (unsigned)slot.last_tls_err);
+        replyAppendf(buf, bufsize, &pos, ", tls:0x%04X", (unsigned)slot.last_tls_err);
       }
     }
     // mbedTLS stack error (shown as negative hex per convention)
     if (slot.last_tls_stack_err != 0) {
-      pos += snprintf(buf + pos, bufsize - pos, ", mbedtls:-0x%04X", (unsigned)(-slot.last_tls_stack_err));
+      replyAppendf(buf, bufsize, &pos, ", mbedtls:-0x%04X", (unsigned)(-slot.last_tls_stack_err));
     }
     // Socket errno
     if (slot.last_sock_errno != 0) {
-      pos += snprintf(buf + pos, bufsize - pos, ", sock:%d", slot.last_sock_errno);
+      replyAppendf(buf, bufsize, &pos, ", sock:%d", slot.last_sock_errno);
     }
     // Time ago
     unsigned long ago_sec = (millis() - slot.last_error_time) / 1000;
     if (ago_sec < 60) {
-      snprintf(buf + pos, bufsize - pos, ", %lus ago", ago_sec);
+      replyAppendf(buf, bufsize, &pos, ", %lus ago", ago_sec);
     } else if (ago_sec < 3600) {
-      snprintf(buf + pos, bufsize - pos, ", %lum ago", ago_sec / 60);
+      replyAppendf(buf, bufsize, &pos, ", %lum ago", ago_sec / 60);
     } else {
-      snprintf(buf + pos, bufsize - pos, ", %luh ago", ago_sec / 3600);
+      replyAppendf(buf, bufsize, &pos, ", %luh ago", ago_sec / 3600);
     }
   } else if (!slot.connected) {
-    snprintf(buf + pos, bufsize - pos, ", no error info");
+    replyAppendf(buf, bufsize, &pos, ", no error info");
   }
 }
 

test/test_mqtt_reply_format/test_mqtt_reply_format.cpp

@@ -0,0 +1,132 @@
+// Host tests for replyAppendf (src/helpers/MQTTReplyFormat.h), the bounded
+// clamping printf-append used by MQTTBridge's status/stats/diag CLI formatters.
+// Proves the A1 out-of-bounds-write bound holds regardless of input size, rather
+// than "by input-size accident" (see the 2026-07-19 MQTT observer review).
+#include <gtest/gtest.h>
+#include <cstring>
+#include "helpers/MQTTReplyFormat.h"
+
+// Lay a canary past the logical buffer so any write at buf[bufsize..] is caught.
+// `logical` bytes are the buffer handed to replyAppendf; the trailing GUARD bytes
+// must stay 0xAA.
+static const size_t GUARD = 8;
+struct Canaried {
+  static const size_t CAP = 256;
+  char mem[CAP + GUARD];
+  size_t logical;
+  explicit Canaried(size_t n) : logical(n) {
+    memset(mem, 0xAA, sizeof(mem));
+    mem[0] = '\0';
+  }
+  char* buf() { return mem; }
+  bool guardIntact() const {
+    for (size_t i = logical; i < logical + GUARD; i++) {
+      if ((unsigned char)mem[i] != 0xAA) return false;
+    }
+    return true;
+  }
+};
+
+TEST(ReplyAppendf, BasicAppendWithinBounds) {
+  char buf[64];
+  int pos = 0;
+  replyAppendf(buf, sizeof(buf), &pos, "> msgs: %s", "on");
+  EXPECT_STREQ("> msgs: on", buf);
+  EXPECT_EQ(pos, 10);
+}
+
+TEST(ReplyAppendf, SequenceAccumulates) {
+  char buf[64];
+  int pos = 0;
+  replyAppendf(buf, sizeof(buf), &pos, "> msgs: %s", "off");
+  replyAppendf(buf, sizeof(buf), &pos, ", %d: %s (%s)", 1, "denmesh", "ok");
+  replyAppendf(buf, sizeof(buf), &pos, ", q:%d", 3);
+  EXPECT_STREQ("> msgs: off, 1: denmesh (ok), q:3", buf);
+  EXPECT_EQ(pos, (int)strlen(buf));
+}
+
+TEST(ReplyAppendf, TruncationClampsPosAndTerminates) {
+  char buf[16];
+  int pos = 0;
+  replyAppendf(buf, sizeof(buf), &pos, "%s", "0123456789ABCDEF_TOO_LONG");
+  // Written up to 15 chars + NUL; pos pinned at bufsize-1.
+  EXPECT_EQ(pos, 15);
+  EXPECT_EQ(buf[15], '\0');
+  EXPECT_STREQ("0123456789ABCDE", buf);
+}
+
+TEST(ReplyAppendf, AppendAfterFullIsNoOp) {
+  char buf[8];
+  int pos = 0;
+  replyAppendf(buf, sizeof(buf), &pos, "%s", "AAAAAAAAAAAA");  // overflows
+  EXPECT_EQ(pos, 7);
+  char snapshot[8];
+  memcpy(snapshot, buf, sizeof(buf));
+  // Further appends must not write anything and must keep pos clamped.
+  replyAppendf(buf, sizeof(buf), &pos, ", more");
+  replyAppendf(buf, sizeof(buf), &pos, ", q:%d", 9);
+  EXPECT_EQ(pos, 7);
+  EXPECT_EQ(0, memcmp(snapshot, buf, sizeof(buf)));
+}
+
+// The A1 reproduction: the old `pos += snprintf(...)` idiom would let pos exceed
+// bufsize after a truncated append, so the *next* append wrote at buf+pos with a
+// wrapped size_t length. replyAppendf must never touch the guard bytes.
+TEST(ReplyAppendf, NoWritePastBufferAcrossOverflowingChain) {
+  Canaried c(24);
+  int pos = 0;
+  // Mimic formatSlotDiagReply's chain, sized to blow well past 24 bytes.
+  replyAppendf(c.buf(), c.logical, &pos, "> mqtt%d: %s", 6, "no client");
+  replyAppendf(c.buf(), c.logical, &pos, ", dc:%lu", 4294967295UL);
+  replyAppendf(c.buf(), c.logical, &pos, ", first_disc:%lus", 4294967295UL);
+  replyAppendf(c.buf(), c.logical, &pos, ", %s (0x%04X)", "cert verify failed", 0x800Bu);
+  replyAppendf(c.buf(), c.logical, &pos, ", mbedtls:-0x%04X", 0x8010u);
+  replyAppendf(c.buf(), c.logical, &pos, ", sock:%d", -2147483647);
+  replyAppendf(c.buf(), c.logical, &pos, ", %luh ago", 1193046UL);
+  EXPECT_TRUE(c.guardIntact());
+  EXPECT_LE(pos, (int)c.logical - 1);
+  EXPECT_EQ(c.buf()[c.logical - 1], '\0');  // still NUL-terminated
+}
+
+TEST(ReplyAppendf, ExactFitBoundary) {
+  char buf[11];  // room for exactly "0123456789" + NUL
+  int pos = 0;
+  replyAppendf(buf, sizeof(buf), &pos, "%s", "0123456789");
+  EXPECT_EQ(pos, 10);
+  EXPECT_STREQ("0123456789", buf);
+  EXPECT_EQ(buf[10], '\0');
+}
+
+TEST(ReplyAppendf, NullAndZeroSizeAreNoOps) {
+  int pos = 0;
+  replyAppendf(nullptr, 16, &pos, "x");  // null buf
+  EXPECT_EQ(pos, 0);
+  char buf[8] = {'k', 0};
+  replyAppendf(buf, 0, &pos, "x");       // zero size
+  EXPECT_STREQ("k", buf);
+  replyAppendf(buf, sizeof(buf), nullptr, "x");  // null pos
+  EXPECT_STREQ("k", buf);
+}
+
+TEST(ReplyAppendf, BufsizeOneJustTerminates) {
+  char buf[1];
+  buf[0] = 'Z';
+  int pos = 0;
+  replyAppendf(buf, sizeof(buf), &pos, "anything");
+  EXPECT_EQ(pos, 0);
+  EXPECT_EQ(buf[0], '\0');
+}
+
+TEST(ReplyAppendf, NegativeIncomingPosTreatedAsZero) {
+  char buf[16];
+  memset(buf, 'x', sizeof(buf));
+  int pos = -5;
+  replyAppendf(buf, sizeof(buf), &pos, "hi");
+  EXPECT_EQ(pos, 2);
+  EXPECT_STREQ("hi", buf);
+}
+
+int main(int argc, char** argv) {
+  ::testing::InitGoogleTest(&argc, argv);
+  return RUN_ALL_TESTS();
+}