Fix the NMEA checksum offset and harden the buffer writes around it - #11293
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughNMEA WPL and GGA builders now guard zero-sized buffers, clamp sentence offsets, and use the shared checksum helper. Tests cover checksums, CRLF handling, truncation, and empty buffers. Dropzone and LittleFS formatting use bounded writes, while RedirectablePrint removes an unnecessary format-string copy. ChangesBuffer safety hardening
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/gps/NMEAWPL.cpp`:
- Around line 42-43: Update the NMEA formatting paths around nmeaChecksum and
the corresponding writes at lines 51-56 and 93-101 to check each snprintf return
value before advancing len or appending the checksum. Detect negative results
and truncation (return value meeting or exceeding the remaining capacity), then
return the existing truncation/error result; only compute buf + len and bufsz -
len after confirming the prior write fit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e6fb466f-0c2f-4494-ab26-6d721645dbb2
📒 Files selected for processing (1)
src/gps/NMEAWPL.cpp
caveman99
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/gps/NMEAWPL.cpp`:
- Around line 49-51: Add an early return for bufsz == 0 before body formatting
in both printWPL() overloads and printGGA() in src/gps/NMEAWPL.cpp, preventing
nmeaChecksum from reading an uninitialized buffer; preserve existing formatting
behavior for nonzero buffer sizes.
In `@src/platform/stm32wl/STM32_LittleFS.cpp`:
- Around line 275-276: Update the errcode formatting in the surrounding STM32
LittleFS error-handling code to use a type-correct portable representation for
int32_t: include the appropriate integer-format header and format err with
PRId32, or explicitly cast it to long while retaining %ld. Preserve the existing
errcode buffer and snprintf behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9a119e11-547c-4dc0-b53c-df87f2877617
📒 Files selected for processing (4)
src/RedirectablePrint.cppsrc/gps/NMEAWPL.cppsrc/modules/DropzoneModule.cppsrc/platform/stm32wl/STM32_LittleFS.cpp
💤 Files with no reviewable changes (1)
- src/RedirectablePrint.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/gps/NMEAWPL.cpp (1)
43-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for buffer-boundary cases.
Please test zero-sized buffers, one-byte/truncated buffers, CRLF-prefixed WPL sentences, and checksum exclusion of trailing
*/line endings.Also applies to: 60-71, 98-120
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/gps/NMEAWPL.cpp` around lines 43 - 54, Extend regression coverage for the WPL formatting functions around the zero-size and truncated-buffer paths, including one-byte buffers, CRLF-prefixed sentences, and checksum validation that excludes the trailing '*' and CRLF. Cover both relevant WPL formatting implementations identified by the shared ranges, and verify safe output length handling without buffer overrun.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/gps/NMEAWPL.cpp`:
- Around line 43-54: Extend regression coverage for the WPL formatting functions
around the zero-size and truncated-buffer paths, including one-byte buffers,
CRLF-prefixed sentences, and checksum validation that excludes the trailing '*'
and CRLF. Cover both relevant WPL formatting implementations identified by the
shared ranges, and verify safe output length handling without buffer overrun.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 505de7ba-a0fc-46e7-8fb2-f961c7702446
📒 Files selected for processing (2)
src/gps/NMEAWPL.cppsrc/platform/stm32wl/STM32_LittleFS.cpp
⚡ Try this PR in the Web Flasher
Warning This is an automated, unreviewed CI test build. Back up your device configuration Supported boards built by this PR (31)
Build artifacts expire on 2026-08-29. Updated for |
|
Added test/test_nmea_wpl covering the requested cases: checksum computed from the $ delimiter for both printWPL overloads and printGGA, CRLF-prefixed sentences, zero-sized buffers, and truncated buffers down to one byte. Verified the tests discriminate. Against the pre-fix NMEAWPL.cpp, 4 of the 6 fail: test_wpl_lite_checksum_skips_leading_crlf: Expected 105 Was 71 105 ^ 71 is 0x2E, the newline and $ the old loop included. All 6 pass on this branch. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/test_nmea_wpl/test_main.cpp (1)
59-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse camelCase for the new test functions.
Rename the six
test_*functions and theirRUN_TESTreferences to camelCase equivalents. As per coding guidelines, C++ functions must use camelCase.Also applies to: 72-72, 83-83, 94-94, 107-107, 122-122, 142-147
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_nmea_wpl/test_main.cpp` at line 59, Rename all six newly added test functions in test_main.cpp from snake_case to camelCase, including test_wpl_lite_checksum_skips_leading_crlf and the functions at the referenced locations, and update each corresponding RUN_TEST reference to use the renamed symbol.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/test_nmea_wpl/test_main.cpp`:
- Around line 45-51: Update emittedChecksum to require exactly two hexadecimal
characters after the asterisk, validating both characters directly and
confirming the next character is the sentence terminator or NUL; retain the
existing assertion that the asterisk is present and return the parsed checksum
value.
- Around line 94-105: Update test_crlf_prefix_does_not_change_checksum to
isolate the CRLF behavior from overload and fixture differences: generate the
prefixed and unprefixed outputs from the same position type and equivalent
fixture, then compare their sentence bodies or validate each against a known
expected checksum rather than comparing only checksums from PositionLite and
Position.
---
Nitpick comments:
In `@test/test_nmea_wpl/test_main.cpp`:
- Line 59: Rename all six newly added test functions in test_main.cpp from
snake_case to camelCase, including test_wpl_lite_checksum_skips_leading_crlf and
the functions at the referenced locations, and update each corresponding
RUN_TEST reference to use the renamed symbol.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f48a7e2f-d581-458b-aec2-0ac9e1d1c53f
📒 Files selected for processing (2)
test/native-suite-counttest/test_nmea_wpl/test_main.cpp
| void test_crlf_prefix_does_not_change_checksum(void) | ||
| { | ||
| char withPrefix[128]; | ||
| char withoutPrefix[128]; | ||
| meshtastic_PositionLite lite = makePositionLite(); | ||
| meshtastic_Position pos = makePosition(); | ||
|
|
||
| printWPL(withPrefix, sizeof(withPrefix), lite, "Test", false); | ||
| printWPL(withoutPrefix, sizeof(withoutPrefix), pos, "Test", false); | ||
|
|
||
| TEST_ASSERT_EQUAL_UINT32(emittedChecksum(withoutPrefix), emittedChecksum(withPrefix)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Isolate the CRLF regression from overload differences.
This test compares PositionLite and Position overloads with different fixtures, then compares only their 8-bit checksums. A formatting difference between overloads—or an XOR collision—can make the result misleading. Compare the same sentence body with and without the prefix, or assert each output against a known expected checksum.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/test_nmea_wpl/test_main.cpp` around lines 94 - 105, Update
test_crlf_prefix_does_not_change_checksum to isolate the CRLF behavior from
overload and fixture differences: generate the prefixed and unprefixed outputs
from the same position type and equivalent fixture, then compare their sentence
bodies or validate each against a known expected checksum rather than comparing
only checksums from PositionLite and Position.
|
Both addressed in b016dff. Checksum parsing now requires exactly two hex digits followed by CR or NUL, so test_crlf_prefix_does_not_change_checksum now asserts each overload against a known checksum (0x69) rather than comparing the two to each other, which removes the overload-difference and XOR-collision concerns. Re-verified discrimination against the pre-fix NMEAWPL.cpp: 4 of 6 fail, including the reworked test at 0x69 vs 71. All 6 pass on this branch. On the camelCase suggestion, declining to keep the surrounding convention: the test tree is 271 snake_case test functions to 1 camelCase. |
b016dff to
6838e00
Compare
6838e00 to
c148d99
Compare
|
Ha, scope crept into STM32 LittleFS? Though it certainly should be fixed regardless of which PR it lands in. |
|
Fair cop. It came out of the same sweep: the NMEA fix was a checksum offset plus an unbounded write, and grepping for the same pattern turned up the remaining fixed-size buffers written with sprintf, STM32 LittleFS and DropzoneModule among them. They rode along in the "harden the remaining fixed buffers" commit rather than becoming three more PRs against a busy CI queue. Each one is its own commit, so if you would rather see the STM32 change land separately it splits out cleanly. No objection either way. |
|
Your call really ;)
It caught my eye because it might have some impact on an open PR I have for
STM32WL LittleFS #11230 but it’s trivial to fix merge conflicts if any, so
I was just amused.
…On Thu, 30 Jul 2026 at 23:17 Thomas Göttgens ***@***.***> wrote:
*caveman99* left a comment (meshtastic/firmware#11293)
<#11293 (comment)>
Fair cop. It came out of the same sweep: the NMEA fix was a checksum
offset plus an unbounded write, and grepping for the same pattern turned up
the remaining fixed-size buffers written with sprintf, STM32 LittleFS and
DropzoneModule among them. They rode along in the "harden the remaining
fixed buffers" commit rather than becoming three more PRs against a busy CI
queue.
Each one is its own commit, so if you would rather see the STM32 change
land separately it splits out cleanly. No objection either way.
—
Reply to this email directly, view it on GitHub
<#11293?email_source=notifications&email_token=AACAFACGLL7NRAYOUOVNKTD5HNRG7A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMJTGI3TGMZYGM4KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-5132733838>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACAFAFDXH6B6KI3PGMGIIL5HNRG7AVCNFSNUABFKJSXA33TNF2G64TZHMZDGNZWGQ3TENJYHNEXG43VMU5TKMBRHE4DGNBZGY4KC5QC>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Checked, and you are clear: no file overlap. #11230 touches littlefs/lfs_util.h, main-stm32wl.cpp and variants/stm32/stm32.ini, while this one only touches STM32_LittleFS.cpp. Neither should conflict with the other whichever lands first. Leaving it here then, since it is self-contained. |
The PositionLite printWPL() format begins with a CRLF, so the fixed start offset of 1 folded the newline and the $ into the checksum and every sentence went out with a wrong value. Locate the $ instead and stop at the terminator or a \*.
snprintf returns the length it would have written, so a truncated NMEA sentence made buf + len point past the buffer and bufsz - len underflow into a huge size for the checksum append. Clamp after each write. Also pulls in the rest of #11236: the two remaining Dropzone sprintf calls, the dead strcpy in mt_sprintf that wrote one byte past a zero-size allocation for an empty format, and the 10-byte errcode buffer that INT32_MIN overflows. Co-Authored-By: Andrew Yong <me@ndoo.sg>
snprintf writes nothing at all when bufsz is 0, not even a terminator, so the checksum helper would run strchr over whatever the buffer already held. Return before touching it. int32_t is not long on every target, so cast before formatting with %ld. Co-Authored-By: Andrew Yong <me@ndoo.sg>
Covers checksum computation from the $ delimiter for both printWPL overloads and printGGA, zero-sized buffers, and truncated buffers down to one byte. Co-Authored-By: Andrew Yong <me@ndoo.sg>
Require exactly two hex digits followed by the sentence terminator, and assert both WPL overloads against a known checksum instead of comparing them to each other.
6eabbfb to
4bdc54c
Compare
Upstream meshtastic#11293 added test_nmea_wpl and took develop's count to 43; this branch had independently reached 46. Merging develop resolved the counter textually, keeping 46, while the directory set became the union of both sides at 47. The suite-count CI gate fails on the mismatch, and it gates the native test jobs, so the tests themselves were being skipped.
Upstream meshtastic#11293 added test_nmea_wpl and took develop's count to 43; this branch had independently reached 46. Merging develop resolved the counter textually, keeping 46, while the directory set became the union of both sides at 47. The suite-count CI gate fails on the mismatch, and it gates the native test jobs, so the tests themselves were being skipped.
2 participants
Fixes #11242. Supersedes #11236, whose changes are included here with credit to @ndoo.
fd98e9f put a leading CRLF in the PositionLite
printWPL()format string, but the checksum loop still started at a fixed offset of 1, so it folded the newline and the$into the XOR and those sentences carried a wrong checksum.nmeaChecksum()locates the$and accumulates to the terminator or a\*. Verified against a reference implementation: the CRLF-prefixed sentence changes from 0x42 to 0x6C, matching the same sentence without the prefix, and the otherprintWPL()andprintGGA()are byte-identical to before.Reading to the terminator also keeps the checksum in bounds when snprintf truncated. The other half of that, raised by CodeRabbit on this PR and originally by #11236, is the append itself:
buf + lenpoints past the buffer andbufsz - lenunderflows whenlen >= bufsz.nmeaClamp()now bounds the length after each write. Checked at bufsz 128, 40, 20, 8, 2 and 1 with a sentinel-filled buffer: nothing is written past bufsz and the returned length always stays in bounds.Remaining pieces from #11236:
sprintfcalls left inDropzoneModule::sendConditions()strcpyinmt_sprintf(), immediately overwritten by the followingvsnprintfand writing one byte past a zero-size allocation for an empty format stringdbg_strerr_lfs's 10-byteerrcode, which INT32_MIN needs 12 bytes forSummary by CodeRabbit