From 764e1e50d83ec652c9d1959d693dd1d4a1e895c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20G=C3=B6ttgens?= Date: Thu, 30 Jul 2026 14:44:52 +0200 Subject: [PATCH 1/6] Checksum NMEA sentences from the $ delimiter 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 \*. --- src/gps/NMEAWPL.cpp | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/src/gps/NMEAWPL.cpp b/src/gps/NMEAWPL.cpp index 5259427e635..5844bc77f0d 100644 --- a/src/gps/NMEAWPL.cpp +++ b/src/gps/NMEAWPL.cpp @@ -2,8 +2,20 @@ #include "NMEAWPL.h" #include "GeoCoord.h" #include "gps/RTC.h" +#include #include +static uint32_t nmeaChecksum(const char *buf) +{ + uint32_t chk = 0; + const char *c = strchr(buf, '$'); + if (c) { + for (c++; *c && *c != '*'; c++) + chk ^= (uint8_t)*c; + } + return chk; +} + /* ------------------------------------------- * 1 2 3 4 5 6 * | | | | | | @@ -27,10 +39,7 @@ uint32_t printWPL(char *buf, size_t bufsz, const meshtastic_PositionLite &pos, c (abs(geoCoord.getLatitude()) - geoCoord.getDMSLatDeg() * 1e+7) * 6e-6, geoCoord.getDMSLatCP(), geoCoord.getDMSLonDeg(), (abs(geoCoord.getLongitude()) - geoCoord.getDMSLonDeg() * 1e+7) * 6e-6, geoCoord.getDMSLonCP(), name); - uint32_t chk = 0; - for (uint32_t i = 1; i < len; i++) { - chk ^= buf[i]; - } + uint32_t chk = nmeaChecksum(buf); len += snprintf(buf + len, bufsz - len, "*%02X\r\n", chk); return len; } @@ -43,10 +52,7 @@ uint32_t printWPL(char *buf, size_t bufsz, const meshtastic_Position &pos, const (abs(geoCoord.getLatitude()) - geoCoord.getDMSLatDeg() * 1e+7) * 6e-6, geoCoord.getDMSLatCP(), geoCoord.getDMSLonDeg(), (abs(geoCoord.getLongitude()) - geoCoord.getDMSLonDeg() * 1e+7) * 6e-6, geoCoord.getDMSLonCP(), name); - uint32_t chk = 0; - for (uint32_t i = 1; i < len; i++) { - chk ^= buf[i]; - } + uint32_t chk = nmeaChecksum(buf); len += snprintf(buf + len, bufsz - len, "*%02X\r\n", chk); return len; } @@ -91,10 +97,7 @@ uint32_t printGGA(char *buf, size_t bufsz, const meshtastic_Position &pos) (abs(geoCoord.getLongitude()) - geoCoord.getDMSLonDeg() * 1e+7) * 6e-6, geoCoord.getDMSLonCP(), pos.fix_quality, pos.sats_in_view, pos.HDOP, geoCoord.getAltitude(), 'M', pos.altitude_geoidal_separation, 'M', 0, 0); - uint32_t chk = 0; - for (uint32_t i = 1; i < len; i++) { - chk ^= buf[i]; - } + uint32_t chk = nmeaChecksum(buf); len += snprintf(buf + len, bufsz - len, "*%02X\r\n", chk); return len; } From 9bb5ca4c7560834fa464b8949ab330bed5322bdd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20G=C3=B6ttgens?= Date: Thu, 30 Jul 2026 14:53:25 +0200 Subject: [PATCH 2/6] Clamp truncated writes and harden the remaining fixed buffers 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 --- src/RedirectablePrint.cpp | 1 - src/gps/NMEAWPL.cpp | 16 +++++++++++++--- src/modules/DropzoneModule.cpp | 6 +++--- src/platform/stm32wl/STM32_LittleFS.cpp | 4 ++-- 4 files changed, 18 insertions(+), 9 deletions(-) diff --git a/src/RedirectablePrint.cpp b/src/RedirectablePrint.cpp index e53806ac77a..f6641fdbdce 100644 --- a/src/RedirectablePrint.cpp +++ b/src/RedirectablePrint.cpp @@ -386,7 +386,6 @@ std::string RedirectablePrint::mt_sprintf(const std::string fmt_str, ...) va_list ap; while (1) { formatted.reset(new char[n]); /* Wrap the plain char array into the unique_ptr */ - strcpy(&formatted[0], fmt_str.c_str()); va_start(ap, fmt_str); int final_n = vsnprintf(&formatted[0], n, fmt_str.c_str(), ap); va_end(ap); diff --git a/src/gps/NMEAWPL.cpp b/src/gps/NMEAWPL.cpp index 5844bc77f0d..02d7549d653 100644 --- a/src/gps/NMEAWPL.cpp +++ b/src/gps/NMEAWPL.cpp @@ -5,6 +5,13 @@ #include #include +static uint32_t nmeaClamp(uint32_t len, size_t bufsz) +{ + if (len >= bufsz) + return bufsz > 0 ? (uint32_t)(bufsz - 1) : 0; + return len; +} + static uint32_t nmeaChecksum(const char *buf) { uint32_t chk = 0; @@ -39,8 +46,9 @@ uint32_t printWPL(char *buf, size_t bufsz, const meshtastic_PositionLite &pos, c (abs(geoCoord.getLatitude()) - geoCoord.getDMSLatDeg() * 1e+7) * 6e-6, geoCoord.getDMSLatCP(), geoCoord.getDMSLonDeg(), (abs(geoCoord.getLongitude()) - geoCoord.getDMSLonDeg() * 1e+7) * 6e-6, geoCoord.getDMSLonCP(), name); + len = nmeaClamp(len, bufsz); uint32_t chk = nmeaChecksum(buf); - len += snprintf(buf + len, bufsz - len, "*%02X\r\n", chk); + len = nmeaClamp(len + snprintf(buf + len, bufsz - len, "*%02X\r\n", chk), bufsz); return len; } @@ -52,8 +60,9 @@ uint32_t printWPL(char *buf, size_t bufsz, const meshtastic_Position &pos, const (abs(geoCoord.getLatitude()) - geoCoord.getDMSLatDeg() * 1e+7) * 6e-6, geoCoord.getDMSLatCP(), geoCoord.getDMSLonDeg(), (abs(geoCoord.getLongitude()) - geoCoord.getDMSLonDeg() * 1e+7) * 6e-6, geoCoord.getDMSLonCP(), name); + len = nmeaClamp(len, bufsz); uint32_t chk = nmeaChecksum(buf); - len += snprintf(buf + len, bufsz - len, "*%02X\r\n", chk); + len = nmeaClamp(len + snprintf(buf + len, bufsz - len, "*%02X\r\n", chk), bufsz); return len; } /* ------------------------------------------- @@ -97,8 +106,9 @@ uint32_t printGGA(char *buf, size_t bufsz, const meshtastic_Position &pos) (abs(geoCoord.getLongitude()) - geoCoord.getDMSLonDeg() * 1e+7) * 6e-6, geoCoord.getDMSLonCP(), pos.fix_quality, pos.sats_in_view, pos.HDOP, geoCoord.getAltitude(), 'M', pos.altitude_geoidal_separation, 'M', 0, 0); + len = nmeaClamp(len, bufsz); uint32_t chk = nmeaChecksum(buf); - len += snprintf(buf + len, bufsz - len, "*%02X\r\n", chk); + len = nmeaClamp(len + snprintf(buf + len, bufsz - len, "*%02X\r\n", chk), bufsz); return len; } diff --git a/src/modules/DropzoneModule.cpp b/src/modules/DropzoneModule.cpp index 4658cb5b6ec..16bd8384969 100644 --- a/src/modules/DropzoneModule.cpp +++ b/src/modules/DropzoneModule.cpp @@ -82,11 +82,11 @@ meshtastic_MeshPacket *DropzoneModule::sendConditions() auto windDirection = telemetry.variant.environment_metrics.wind_direction; auto temp = telemetry.variant.environment_metrics.temperature; auto baro = UnitConversions::HectoPascalToInchesOfMercury(telemetry.variant.environment_metrics.barometric_pressure); - sprintf(replyStr, "%s @ %02d:%02d:%02dz\nWind %.2f kts @ %d°\nBaro %.2f inHg %.2f°C", dropzoneStatus, hour, min, sec, - windSpeed, windDirection, baro, temp); + snprintf(replyStr, sizeof(replyStr), "%s @ %02d:%02d:%02dz\nWind %.2f kts @ %d°\nBaro %.2f inHg %.2f°C", dropzoneStatus, + hour, min, sec, windSpeed, windDirection, baro, temp); } else { LOG_ERROR("No sensor found"); - sprintf(replyStr, "%s @ %02d:%02d:%02d\nNo sensor found", dropzoneStatus, hour, min, sec); + snprintf(replyStr, sizeof(replyStr), "%s @ %02d:%02d:%02d\nNo sensor found", dropzoneStatus, hour, min, sec); } LOG_DEBUG("Conditions reply: %s", replyStr); reply->decoded.payload.size = strlen(replyStr); // You must specify how many bytes are in the reply diff --git a/src/platform/stm32wl/STM32_LittleFS.cpp b/src/platform/stm32wl/STM32_LittleFS.cpp index 97e79e61e7e..e9feee40aca 100644 --- a/src/platform/stm32wl/STM32_LittleFS.cpp +++ b/src/platform/stm32wl/STM32_LittleFS.cpp @@ -272,8 +272,8 @@ const char *dbg_strerr_lfs(int32_t err) return "LFS_ERR_NOMEM"; default: - static char errcode[10]; - sprintf(errcode, "%ld", err); + static char errcode[13]; + snprintf(errcode, sizeof(errcode), "%ld", err); return errcode; } From 8f008753736e90e1476971735f22db32b4082f8b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20G=C3=B6ttgens?= Date: Thu, 30 Jul 2026 15:00:00 +0200 Subject: [PATCH 3/6] Bail out on a zero-sized buffer and cast err for %ld 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 --- src/gps/NMEAWPL.cpp | 9 +++++++++ src/platform/stm32wl/STM32_LittleFS.cpp | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/gps/NMEAWPL.cpp b/src/gps/NMEAWPL.cpp index 02d7549d653..72ed6e5cd08 100644 --- a/src/gps/NMEAWPL.cpp +++ b/src/gps/NMEAWPL.cpp @@ -40,6 +40,9 @@ static uint32_t nmeaChecksum(const char *buf) uint32_t printWPL(char *buf, size_t bufsz, const meshtastic_PositionLite &pos, const char *name, bool isCaltopoMode) { + if (bufsz == 0) + return 0; + GeoCoord geoCoord(pos.latitude_i, pos.longitude_i, pos.altitude); char type = isCaltopoMode ? 'P' : 'N'; uint32_t len = snprintf(buf, bufsz, "\r\n$G%cWPL,%02d%07.4f,%c,%03d%07.4f,%c,%s", type, geoCoord.getDMSLatDeg(), @@ -54,6 +57,9 @@ uint32_t printWPL(char *buf, size_t bufsz, const meshtastic_PositionLite &pos, c uint32_t printWPL(char *buf, size_t bufsz, const meshtastic_Position &pos, const char *name, bool isCaltopoMode) { + if (bufsz == 0) + return 0; + GeoCoord geoCoord(pos.latitude_i, pos.longitude_i, pos.altitude); char type = isCaltopoMode ? 'P' : 'N'; uint32_t len = snprintf(buf, bufsz, "$G%cWPL,%02d%07.4f,%c,%03d%07.4f,%c,%s", type, geoCoord.getDMSLatDeg(), @@ -89,6 +95,9 @@ uint32_t printWPL(char *buf, size_t bufsz, const meshtastic_Position &pos, const uint32_t printGGA(char *buf, size_t bufsz, const meshtastic_Position &pos) { + if (bufsz == 0) + return 0; + GeoCoord geoCoord(pos.latitude_i, pos.longitude_i, pos.altitude); time_t timestamp = pos.timestamp; diff --git a/src/platform/stm32wl/STM32_LittleFS.cpp b/src/platform/stm32wl/STM32_LittleFS.cpp index e9feee40aca..6d804d742b6 100644 --- a/src/platform/stm32wl/STM32_LittleFS.cpp +++ b/src/platform/stm32wl/STM32_LittleFS.cpp @@ -273,7 +273,7 @@ const char *dbg_strerr_lfs(int32_t err) default: static char errcode[13]; - snprintf(errcode, sizeof(errcode), "%ld", err); + snprintf(errcode, sizeof(errcode), "%ld", (long)err); return errcode; } From 63be16d133e7bee5f5527a83c7cce61be7394821 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20G=C3=B6ttgens?= Date: Thu, 30 Jul 2026 15:29:59 +0200 Subject: [PATCH 4/6] Add NMEA sentence regression tests 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 --- test/test_nmea_wpl/test_main.cpp | 151 +++++++++++++++++++++++++++++++ 1 file changed, 151 insertions(+) create mode 100644 test/test_nmea_wpl/test_main.cpp diff --git a/test/test_nmea_wpl/test_main.cpp b/test/test_nmea_wpl/test_main.cpp new file mode 100644 index 00000000000..820550e0633 --- /dev/null +++ b/test/test_nmea_wpl/test_main.cpp @@ -0,0 +1,151 @@ +#include "GeoCoord.h" +#include "NMEAWPL.h" +#include "TestUtil.h" +#include "mesh-pb-constants.h" +#include +#include +#include + +void setUp(void) {} +void tearDown(void) {} + +static meshtastic_PositionLite makePositionLite() +{ + meshtastic_PositionLite pos = meshtastic_PositionLite_init_default; + pos.latitude_i = 472852133; + pos.longitude_i = 85652500; + pos.altitude = 400; + pos.time = 42; + return pos; +} + +static meshtastic_Position makePosition() +{ + meshtastic_Position pos = meshtastic_Position_init_default; + pos.has_latitude_i = true; + pos.latitude_i = 472852133; + pos.has_longitude_i = true; + pos.longitude_i = 85652500; + pos.has_altitude = true; + pos.altitude = 400; + pos.timestamp = 43; + return pos; +} + +static uint32_t expectedChecksum(const char *sentence) +{ + uint32_t chk = 0; + const char *c = strchr(sentence, '$'); + TEST_ASSERT_NOT_NULL(c); + for (c++; *c && *c != '*'; c++) + chk ^= (uint8_t)*c; + return chk; +} + +static uint32_t emittedChecksum(const char *buf) +{ + const char *star = strrchr(buf, '*'); + TEST_ASSERT_NOT_NULL(star); + unsigned parsed = 0; + TEST_ASSERT_EQUAL_INT(1, sscanf(star + 1, "%02X", &parsed)); + return parsed; +} + +static void assertChecksumMatchesBody(const char *buf) +{ + TEST_ASSERT_EQUAL_UINT32(expectedChecksum(buf), emittedChecksum(buf)); +} + +void test_wpl_lite_checksum_skips_leading_crlf(void) +{ + char buf[128]; + meshtastic_PositionLite pos = makePositionLite(); + uint32_t len = printWPL(buf, sizeof(buf), pos, "Test", false); + + TEST_ASSERT_TRUE(len < sizeof(buf)); + TEST_ASSERT_EQUAL_CHAR('\r', buf[0]); + TEST_ASSERT_EQUAL_CHAR('\n', buf[1]); + TEST_ASSERT_EQUAL_CHAR('$', buf[2]); + assertChecksumMatchesBody(buf); +} + +void test_wpl_position_checksum(void) +{ + char buf[128]; + meshtastic_Position pos = makePosition(); + uint32_t len = printWPL(buf, sizeof(buf), pos, "Test", false); + + TEST_ASSERT_TRUE(len < sizeof(buf)); + TEST_ASSERT_EQUAL_CHAR('$', buf[0]); + assertChecksumMatchesBody(buf); +} + +void test_gga_checksum(void) +{ + char buf[160]; + meshtastic_Position pos = makePosition(); + uint32_t len = printGGA(buf, sizeof(buf), pos); + + TEST_ASSERT_TRUE(len < sizeof(buf)); + TEST_ASSERT_EQUAL_CHAR('$', buf[0]); + assertChecksumMatchesBody(buf); +} + +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)); +} + +void test_zero_sized_buffer_writes_nothing(void) +{ + char buf[64]; + memset(buf, 'A', sizeof(buf)); + meshtastic_PositionLite lite = makePositionLite(); + meshtastic_Position pos = makePosition(); + + TEST_ASSERT_EQUAL_UINT32(0, printWPL(buf, 0, lite, "Test", false)); + TEST_ASSERT_EQUAL_UINT32(0, printWPL(buf, 0, pos, "Test", false)); + TEST_ASSERT_EQUAL_UINT32(0, printGGA(buf, 0, pos)); + + for (size_t i = 0; i < sizeof(buf); i++) + TEST_ASSERT_EQUAL_CHAR('A', buf[i]); +} + +void test_truncated_buffers_stay_in_bounds(void) +{ + const size_t sizes[] = {1, 2, 8, 20, 40}; + meshtastic_PositionLite lite = makePositionLite(); + + for (size_t s = 0; s < sizeof(sizes) / sizeof(sizes[0]); s++) { + char buf[128]; + memset(buf, 0x7E, sizeof(buf)); + uint32_t len = printWPL(buf, sizes[s], lite, "Test", false); + + TEST_ASSERT_TRUE(len < sizes[s]); + for (size_t i = sizes[s]; i < sizeof(buf); i++) + TEST_ASSERT_EQUAL_HEX8(0x7E, (uint8_t)buf[i]); + } +} + +void setup() +{ + initializeTestEnvironment(); + UNITY_BEGIN(); + RUN_TEST(test_wpl_lite_checksum_skips_leading_crlf); + RUN_TEST(test_wpl_position_checksum); + RUN_TEST(test_gga_checksum); + RUN_TEST(test_crlf_prefix_does_not_change_checksum); + RUN_TEST(test_zero_sized_buffer_writes_nothing); + RUN_TEST(test_truncated_buffers_stay_in_bounds); + exit(UNITY_END()); +} + +void loop() {} From 0e55c03c79a1ff20105ac08b16c5227333e9acc0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20G=C3=B6ttgens?= Date: Thu, 30 Jul 2026 15:48:57 +0200 Subject: [PATCH 5/6] Tighten checksum parsing and pin the WPL fixture checksum 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. --- test/test_nmea_wpl/test_main.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/test/test_nmea_wpl/test_main.cpp b/test/test_nmea_wpl/test_main.cpp index 820550e0633..4cef767200e 100644 --- a/test/test_nmea_wpl/test_main.cpp +++ b/test/test_nmea_wpl/test_main.cpp @@ -2,7 +2,9 @@ #include "NMEAWPL.h" #include "TestUtil.h" #include "mesh-pb-constants.h" +#include #include +#include #include #include @@ -46,6 +48,9 @@ static uint32_t emittedChecksum(const char *buf) { const char *star = strrchr(buf, '*'); TEST_ASSERT_NOT_NULL(star); + TEST_ASSERT_TRUE(isxdigit((unsigned char)star[1])); + TEST_ASSERT_TRUE(isxdigit((unsigned char)star[2])); + TEST_ASSERT_TRUE(star[3] == '\r' || star[3] == '\0'); unsigned parsed = 0; TEST_ASSERT_EQUAL_INT(1, sscanf(star + 1, "%02X", &parsed)); return parsed; @@ -93,6 +98,7 @@ void test_gga_checksum(void) void test_crlf_prefix_does_not_change_checksum(void) { + const uint32_t fixtureChecksum = 0x69; char withPrefix[128]; char withoutPrefix[128]; meshtastic_PositionLite lite = makePositionLite(); @@ -101,7 +107,8 @@ void test_crlf_prefix_does_not_change_checksum(void) printWPL(withPrefix, sizeof(withPrefix), lite, "Test", false); printWPL(withoutPrefix, sizeof(withoutPrefix), pos, "Test", false); - TEST_ASSERT_EQUAL_UINT32(emittedChecksum(withoutPrefix), emittedChecksum(withPrefix)); + TEST_ASSERT_EQUAL_UINT32(fixtureChecksum, emittedChecksum(withPrefix)); + TEST_ASSERT_EQUAL_UINT32(fixtureChecksum, emittedChecksum(withoutPrefix)); } void test_zero_sized_buffer_writes_nothing(void) From 4bdc54cfd1e2778078c1fc12d0dd389525d79cba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20G=C3=B6ttgens?= Date: Thu, 30 Jul 2026 17:07:24 +0200 Subject: [PATCH 6/6] Bump native suite count to 43 --- test/native-suite-count | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/native-suite-count b/test/native-suite-count index d81cc0710eb..920a1396648 100644 --- a/test/native-suite-count +++ b/test/native-suite-count @@ -1 +1 @@ -42 +43