PositionModule::sendLostAndFoundText: use stack buffer, eliminate heap alloc (#10251)

* PositionModule::sendLostAndFoundText: use stack buffer, eliminate heap alloc

The lost-and-found message was built with an unnecessary heap allocation:

    char *message = new char[60];
    sprintf(message, "..."...);
    ...
    delete[] message;

Two problems:

1. **Buffer too small.** The format string expands with two %f (IEEE 754
   doubles), which `sprintf` prints with full precision — easily 15+
   digits each plus separators — so the actual rendered string can run
   40-50 characters before even considering the emoji (4 UTF-8 bytes)
   and the embedded BEL. A pathological lat/lon can overflow 60 bytes
   and corrupt heap metadata. Unbounded `sprintf` with no size check.

2. **Heap churn in a GPS callback.** This function is called from the
   position-update path which is already heap-sensitive. An infrequent
   60-byte transient alloc isn't catastrophic, but stack is trivially
   available here and removes the failure mode entirely.

Fix: replace with a 128-byte stack buffer and `snprintf` bounded by
`sizeof(message)`. Drop the matching `delete[]` since there's nothing
to delete.

Behavior is identical on the happy path; the overflow case now
truncates safely instead of scribbling over heap.

* Potential fix for pull request finding

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

* PositionModule.cpp: add trailing newline for clang-format

* Address Copilot review: cleaner snprintf size handling

Review feedback from @Copilot on PR #10251: the ternary-plus-static-cast
form mixed signed/unsigned types (int written vs. pb_size_t payload.size
vs. size_t sizeof(message)) and was harder to read than necessary.

Cleaner form:
  const size_t msg_len = std::min(static_cast<size_t>(written), sizeof(message) - 1);
  p->decoded.payload.size = msg_len;

Same behaviour (clamp to buffer-minus-NUL) with one explicit cast and
a size_t variable that names the meaning. Handles the encoding-error
path (written < 0) separately so no bad values leak into payload.size.

* Trunk

---------

Co-authored-by: Ben Meadors <benmmeadors@gmail.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This commit is contained in:
nightjoker7
2026-04-23 18:20:07 -05:00
committed by GitHub
co-authored by GitHub Ben Meadors Copilot Autofix powered by AI
parent 83a98c81f6
commit d9195944df
+15 -6
View File
@@ -492,15 +492,24 @@ void PositionModule::sendLostAndFoundText()
{
meshtastic_MeshPacket *p = allocDataPacket();
p->to = NODENUM_BROADCAST;
char *message = new char[60];
sprintf(message, "🚨I'm lost! Lat / Lon: %f, %f\a", (lastGpsLatitude * 1e-7), (lastGpsLongitude * 1e-7));
char message[128];
int written = snprintf(message, sizeof(message), "🚨I'm lost! Lat / Lon: %f, %f\a", (lastGpsLatitude * 1e-7),
(lastGpsLongitude * 1e-7));
p->decoded.portnum = meshtastic_PortNum_TEXT_MESSAGE_APP;
p->want_ack = false;
p->decoded.payload.size = strlen(message);
memcpy(p->decoded.payload.bytes, message, p->decoded.payload.size);
if (written < 0) {
// snprintf encoding error — send an empty payload rather than uninitialized bytes.
p->decoded.payload.size = 0;
} else {
// Clamp to buffer capacity (snprintf returns "would-have-written" which can exceed the buffer).
const size_t msg_len = std::min(static_cast<size_t>(written), sizeof(message) - 1);
p->decoded.payload.size = msg_len;
if (msg_len > 0) {
memcpy(p->decoded.payload.bytes, message, msg_len);
}
}
service->sendToMesh(p, RX_SRC_LOCAL, true);
delete[] message;
}
// Helper: return imprecise (truncated + centered) lat/lon as int32 using current precision
@@ -580,4 +589,4 @@ void PositionModule::handleNewPosition()
}
}
#endif
#endif