Fix the NMEA checksum offset and harden the buffer writes around it (#11293)

* 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 \*.

* 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 <me@ndoo.sg>

* 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 <me@ndoo.sg>

* 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 <me@ndoo.sg>

* 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.

* Bump native suite count to 43

---------

Co-authored-by: Andrew Yong <me@ndoo.sg>

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);

src/gps/NMEAWPL.cpp

@@ -2,8 +2,27 @@
 #include "NMEAWPL.h"
 #include "GeoCoord.h"
 #include "gps/RTC.h"
+#include <string.h>
 #include <time.h>
 
+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;
+    const char *c = strchr(buf, '$');
+    if (c) {
+        for (c++; *c && *c != '*'; c++)
+            chk ^= (uint8_t)*c;
+    }
+    return chk;
+}
+
 /* -------------------------------------------
  *        1       2 3        4 5    6
  *        |       | |        | |    |
@@ -21,33 +40,35 @@
 
 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(),
                             (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];
-    }
-    len += snprintf(buf + len, bufsz - len, "*%02X\r\n", chk);
+    len = nmeaClamp(len, bufsz);
+    uint32_t chk = nmeaChecksum(buf);
+    len = nmeaClamp(len + snprintf(buf + len, bufsz - len, "*%02X\r\n", chk), bufsz);
     return len;
 }
 
 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(),
                             (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];
-    }
-    len += snprintf(buf + len, bufsz - len, "*%02X\r\n", chk);
+    len = nmeaClamp(len, bufsz);
+    uint32_t chk = nmeaChecksum(buf);
+    len = nmeaClamp(len + snprintf(buf + len, bufsz - len, "*%02X\r\n", chk), bufsz);
     return len;
 }
 /* -------------------------------------------
@@ -74,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;
 
@@ -91,11 +115,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);
 
-    uint32_t chk = 0;
-    for (uint32_t i = 1; i < len; i++) {
-        chk ^= buf[i];
-    }
-    len += snprintf(buf + len, bufsz - len, "*%02X\r\n", chk);
+    len = nmeaClamp(len, bufsz);
+    uint32_t chk = nmeaChecksum(buf);
+    len = nmeaClamp(len + snprintf(buf + len, bufsz - len, "*%02X\r\n", chk), bufsz);
     return len;
 }
 

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

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", (long)err);
         return errcode;
     }
 

test/native-suite-count

@@ -1 +1 @@
-42
+43

test/test_nmea_wpl/test_main.cpp

@@ -0,0 +1,158 @@
+#include "GeoCoord.h"
+#include "NMEAWPL.h"
+#include "TestUtil.h"
+#include "mesh-pb-constants.h"
+#include <cctype>
+#include <cstdint>
+#include <cstdio>
+#include <cstring>
+#include <unity.h>
+
+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);
+    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;
+}
+
+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)
+{
+    const uint32_t fixtureChecksum = 0x69;
+    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(fixtureChecksum, emittedChecksum(withPrefix));
+    TEST_ASSERT_EQUAL_UINT32(fixtureChecksum, emittedChecksum(withoutPrefix));
+}
+
+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() {}