Skip to content

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

Merged
thebentern merged 6 commits into
meshtastic:developfrom
nightjoker7:fix/positionmodule-heap-alloc
Apr 23, 2026
Merged

PositionModule::sendLostAndFoundText: use stack buffer, eliminate heap alloc#10251
thebentern merged 6 commits into
meshtastic:developfrom
nightjoker7:fix/positionmodule-heap-alloc

Conversation

@nightjoker7

Copy link
Copy Markdown
Contributor

Problem

src/modules/PositionModule.cpp::sendLostAndFoundText() uses an unnecessary 60-byte heap allocation + unbounded sprintf:

char *message = new char[60];
sprintf(message, "🚨I'm lost! Lat / Lon: %f, %f\a", (lastGpsLatitude * 1e-7), (lastGpsLongitude * 1e-7));
...
delete[] message;

Two issues with that:

  1. 60 bytes is too small in worst cases. The %f specifier renders IEEE-754 doubles at default precision — each %f alone can exceed 10–15 characters in the pathological case, plus the emoji is 4 UTF-8 bytes, plus the \a BEL, plus the \0 terminator. With two %fs plus the surrounding text, a typical render is ~40–50 bytes but the upper bound is not controlled. And sprintf (not snprintf) has no bounds check.

  2. 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:

char message[128];
snprintf(message, sizeof(message), "...", ...);

128 bytes is comfortably above the practical maximum and matches the sizing convention used elsewhere in PositionModule (canned_message_module etc.). Drop the matching delete[] since there's nothing to delete.

Risk

  • Happy path: byte-identical output.
  • Overflow path: was undefined-behavior heap corruption, is now safe truncation at 127 bytes + NUL.
  • Stack usage: +128 bytes in a function that's called infrequently (only when 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.

…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.
@github-actions github-actions Bot added the bugfix Pull request that fixes bugs label Apr 23, 2026
@thebentern thebentern requested a review from Copilot April 23, 2026 02:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 sprintf with snprintf to bound writes to the buffer.

Comment thread src/modules/PositionModule.cpp Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@thebentern thebentern requested a review from Copilot April 23, 2026 11:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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[] + sprintf with a fixed stack buffer and snprintf
  • Use snprintf’s return value to set payload size with safe truncation handling
  • Remove the corresponding delete[]

Comment thread src/modules/PositionModule.cpp
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.
@nightjoker7

Copy link
Copy Markdown
Contributor Author

Addressed in c7bdfa0bd: cleaner form using std::min(static_cast<size_t>(written), sizeof(message) - 1) for the size clamp. Handles snprintf's 'would-have-written' return semantics and the encoding-error (<0) path.

@thebentern

Copy link
Copy Markdown
Contributor

@copilot trunk fmt

@thebentern thebentern merged commit d919594 into meshtastic:develop Apr 23, 2026
1 check passed
mariotti pushed a commit to mariotti/firmware that referenced this pull request May 6, 2026
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix Pull request that fixes bugs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants