fix: NMEA checksum validation and strtol error handling

[gdellis]

Summary

Fixes issues #39 and #40:

Issue #39 - NMEA checksum accepts missing checksum:

• Changed nmea_validate_checksum() to return false when checksum is absent
• Updated docstring to reflect new behavior

Issue #40 - strtol error handling broken:

• Added proper endptr checking to detect conversion failures
• Returns false if strtol fails to parse the checksum hex digits

Additional fix:

• GGA fix quality now accepts only valid range '1'-'6' instead of '1'-'9'

Changes

• firmware/main/nmea_parser.hpp:
  • Return false when * delimiter not found (line ~80)
  • Added endptr checking for strtol (line ~94)
  • Fixed GGA fix quality check to '1'-'6' (line ~115)

Testing

Build passes for ESP32-S3 target.

[github-actions]

PR #53 Review: fix: NMEA checksum validation and strtol error handling

CI Status

GitHub Actions checks still IN PROGRESS (Build Firmware, Host Tests, CodeQL analyses). Cannot confirm CI passed yet.

---

Code Review

1. Docstring Behavior Change

File: firmware/main/nmea_parser.hpp:72-80

* @return true if checksum valid, false if invalid or absent

[BEST PRACTICE] [LOW]: The docstring now correctly documents that absent checksum returns false, but this is a breaking change in behavior. Previously callers could rely on "no * = valid" (implicit accept). Any code depending on the old behavior will silently break. Consider documenting this as a breaking change in the PR description if this function is used elsewhere in the codebase.

---

2. strtol Conversion - Silent Truncation Risk

File: firmware/main/nmea_parser.hpp:94-97

char* endptr = NULL;
uint8_t expected = (uint8_t)strtol (expected_str, &endptr, 16);
if (endptr == expected_str || *endptr != '\0') {
    return false;
}

[IMPROVEMENT] [LOW]: The fix for #40 is correct - properly detecting strtol failures via endptr. However, the cast (uint8_t)strtol(...) silently truncates values > 255. While this is safe given NMEA checksums are always 2 hex digits ('00'-'FF'), a strtoul or explicit range check would be more robust:

unsigned long expected = strtoul(expected_str, &endptr, 16);
if (endptr == expected_str || *endptr != '\0' || expected > 255) {
    return false;
}

---

3. GGA Fix Quality Range - Good Fix

File: firmware/main/nmea_parser.hpp:115

data.has_fix = (field[0] >= '1' && field[0] <= '6');

[BEST PRACTICE] [LOW]: Correct fix aligning with NMEA spec for GGA fix quality (0=invalid, 1=GPS fix, 2=DGPS, 3=PPS, 4=RTK, 5=Float RTK, 6=Estimated). Accepting '1'-'9' was incorrect as 7, 8, 9 are undefined.

---

4. Test Naming

File: firmware/tests/test_nmea_parser.cpp:17

SECTION("No checksum star fails") {

[BEST PRACTICE] [LOW]: Test section name uses present tense "fails" which is unconventional. Consider "absent checksum fails" or past tense "absent checksum returned true" (old behavior). Minor nitpick.

---

Summary

Category	Count	Severity
BUG	0	-
SECURITY	0	-
IMPROVEMENT	1	LOW
BEST PRACTICE	3	LOW

CI not yet complete - Build Firmware and Host Tests still in progress. No blocking issues found. The fixes correctly address issues #39 and #40, and the GGA fix quality change is a good improvement.

opencode session  |  github run

[gdellis]

All comments addressed:

1. strtol silent truncation risk (item 2): Changed from (uint8_t)strtol(...) to strtoul with explicit range check expected > 255 before casting to uint8_t. This prevents silent truncation if a malformed checksum > 255 were encountered.

2. Test naming (item 4): Renamed "No checksum star fails" to "Absent checksum fails" - clearer and uses consistent naming.

3. Docstring change (item 1): Behavior change from "no * = valid" to "absent = false" is intentional per issue bug: NMEA checksum validation accepts missing checksum #39. The docstring already reflects this.

4. GGA fix quality (item 3): Already correctly implemented (accepting only '1'-'6' per NMEA spec).

Changes pushed to fix/nmea-checksum-strtol branch (commit d58283a).

[github-actions]

PR #53 Code Review

CI Status

Check	Status
CodeQL Analysis	✅ SUCCESS
Lint (pre-commit)	✅ SUCCESS
Lint C++	✅ SUCCESS
Check Firmware Changed	✅ SUCCESS
Build Firmware	⏳ IN PROGRESS
Host Tests	⏳ IN PROGRESS
review / opencode	⏳ IN PROGRESS

CI checks not yet complete - Build Firmware and Host Tests still running.

---

Code Review of Changes

1. Checksum Absent Behavior Change

File: firmware/main/nmea_parser.hpp:80-81

if (!checksum_ptr) {
    return false;
}

[BEST PRACTICE] [LOW]: This is a breaking behavior change for callers. Previously nmea_validate_checksum() returned true when no checksum was present (implicit accept). Now it returns false. The PR description mentions this fixes issue #39, and line 193 shows nmea_parse() already treats missing/invalid checksum the same (returns false). This is consistent, but verify all other call sites handle the new return value correctly.

---

2. strtoul Error Handling - Good Fix

File: firmware/main/nmea_parser.hpp:94-99

char expected_str[3] = { checksum_ptr[1], checksum_ptr[2], '\0' };
char* endptr = NULL;
unsigned long expected = strtoul (expected_str, &endptr, 16);
if (endptr == expected_str || *endptr != '\0' || expected > 255) {
    return false;
}
return calculated == (uint8_t)expected;

[BEST PRACTICE] [LOW]: Excellent fix. Uses strtoul (unsigned, avoids potential negative value issues), properly validates endptr, and checks expected > 255 to prevent silent truncation before casting to uint8_t. Well-implemented.

---

3. GGA Fix Quality Range - Correct Fix

File: firmware/main/nmea_parser.hpp:115

data.has_fix = (field[0] >= '1' && field[0] <= '6');

[BEST PRACTICE] [LOW]: Correct alignment with NMEA spec. Values 1-6 represent: 1=GPS fix, 2=DGPS, 3=PPS, 4=RTK, 5=Float RTK, 6=Estimated. Values 7, 8, 9 are undefined. Good fix.

---

4. Test Naming - Good Update

File: firmware/tests/test_nmea_parser.cpp:17

SECTION("Absent checksum fails") {

[BEST PRACTICE] [LOW]: Clearer naming. "Absent" is more descriptive than "No checksum star".

---

5. Potential NULL Pointer Dereference - Pre-existing

File: firmware/main/nmea_parser.hpp:78

inline bool
nmea_validate_checksum (const char* nmea, size_t len) {
    const char* checksum_ptr = (const char*)memchr (nmea, '*', len);

[SECURITY] [LOW]: No NULL check on nmea before passing to memchr. If nmea is NULL, this is undefined behavior. However, this is pre-existing - not introduced by this PR. The nmea_parse() function (line 189) does check len < 6 || nmea[0] != '$' before calling this, but if nmea is NULL, nmea[0] would UB before that check.

---

Summary

Category	Count	Severity
BUG	0	-
SECURITY	1 (pre-existing)	LOW
IMPROVEMENT	0	-
BEST PRACTICE	3	LOW

CI not yet complete - no blocking issues found. The fixes correctly address issues #39 and #40. The strtoul error handling is particularly well-implemented with proper endptr validation and overflow checking. The only security note is a pre-existing issue, not introduced by this PR.

opencode session  |  github run