PositionModule::sendLostAndFoundText: use stack buffer, eliminate heap alloc#10251
Conversation
…p 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.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR hardens PositionModule::sendLostAndFoundText() by removing a small heap allocation and replacing unbounded formatting with a bounded stack buffer to prevent potential overflows.
Changes:
- Replaced
new[]/delete[]message buffer with a fixed-size stack buffer. - Replaced
sprintfwithsnprintfto bound writes to the buffer.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR removes an unsafe/undersized heap allocation in PositionModule::sendLostAndFoundText() by replacing sprintf with bounded snprintf into a stack buffer, preventing buffer overflow and avoiding heap use in the call path.
Changes:
- Replace
new[]+sprintfwith a fixed stack buffer andsnprintf - Use
snprintf’s return value to set payload size with safe truncation handling - Remove the corresponding
delete[]
Review feedback from @Copilot on PR meshtastic#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.
|
Addressed in c7bdfa0bd: cleaner form using |
|
@copilot trunk fmt |
…p alloc (meshtastic#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 meshtastic#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>
Problem
src/modules/PositionModule.cpp::sendLostAndFoundText()uses an unnecessary 60-byte heap allocation + unboundedsprintf:Two issues with that:
60 bytes is too small in worst cases. The
%fspecifier renders IEEE-754 doubles at default precision — each%falone can exceed 10–15 characters in the pathological case, plus the emoji is 4 UTF-8 bytes, plus the\aBEL, plus the\0terminator. With two%fs plus the surrounding text, a typical render is ~40–50 bytes but the upper bound is not controlled. Andsprintf(notsnprintf) has no bounds check.Unnecessary heap allocation in a GPS-callback path. This code runs in a heap-sensitive context. A stack buffer is trivially available here.
Fix
Replace the heap buffer with a 128-byte stack buffer and use bounded
snprintf:128 bytes is comfortably above the practical maximum and matches the sizing convention used elsewhere in PositionModule (
canned_message_moduleetc.). Drop the matchingdelete[]since there's nothing to delete.Risk
sendLostAndFoundText()is explicitly invoked), not in any tight loop.Related
Part of a broader "eliminate unnecessary heap allocs in module hot-paths" cleanup I've been working through on my deployment.