diff --git a/src/helpers/MQTTReplyFormat.h b/src/helpers/MQTTReplyFormat.h new file mode 100644 index 00000000..bdb343d7 --- /dev/null +++ b/src/helpers/MQTTReplyFormat.h @@ -0,0 +1,43 @@ +#pragma once + +#include +#include +#include + +// 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 +} diff --git a/src/helpers/bridges/MQTTBridge.cpp b/src/helpers/bridges/MQTTBridge.cpp index 7c6b9776..893d4f1c 100644 --- a/src/helpers/bridges/MQTTBridge.cpp +++ b/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"); } } diff --git a/test/test_mqtt_reply_format/test_mqtt_reply_format.cpp b/test/test_mqtt_reply_format/test_mqtt_reply_format.cpp new file mode 100644 index 00000000..4b3726af --- /dev/null +++ b/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 +#include +#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(); +}