GPS: keep MicroNMEA's over-reads inside the provider's NMEA buffer

.github/workflows/run-unit-tests.yml

@@ -20,7 +20,7 @@ jobs:
         uses: ./.github/actions/setup-build-environment
 
       - name: Run Unit Tests
-        run: pio test -e native -e native_kiss_modem -vv
+        run: pio test -e native -e native_kiss_modem -e native_gps -vv
 
       - name: Upload Test Results
         # Upload test results even if the test step failed.

platformio.ini

@@ -167,7 +167,7 @@ build_flags = -std=c++17
   -I src
   -I test/mocks
 test_build_src = yes
-test_ignore = test_kiss_modem
+test_ignore = test_kiss_modem, test_gps_nmea
 build_src_filter =
   -<*>
   +<../src/Utils.cpp>
@@ -191,3 +191,26 @@ build_src_filter =
   +<../examples/kiss_modem/KissModem.cpp>
 lib_deps =
   google/googletest @ 1.17.0
+
+; GPS/NMEA provider tests compile the real provider header against the real
+; MicroNMEA library. The mocks must shadow src/ (same ordering as the KISS
+; modem suite). MicroNMEA is an Arduino-format library, so compatibility
+; checks are off to let it build for the native platform. Sanitizers make
+; out-of-bounds reads in the parser fail the test instead of passing silently.
+[env:native_gps]
+platform = native
+test_framework = googletest
+build_flags = -std=c++17
+  -I test/mocks
+  -I src
+  -fsanitize=address,undefined
+  -fno-sanitize-recover=all
+  -fno-omit-frame-pointer
+test_build_src = yes
+test_filter = test_gps_nmea
+build_src_filter =
+  -<*>
+lib_compat_mode = off
+lib_deps =
+  google/googletest @ 1.17.0
+  stevemarple/MicroNMEA @ 2.0.6

src/helpers/sensors/MicroNMEALocationProvider.h

@@ -39,7 +39,17 @@
 #endif
 
 class MicroNMEALocationProvider : public LocationProvider {
-    char _nmeaBuffer[100];
+public:
+    // MicroNMEA never writes past the length it is given, but its fixed-width
+    // field skips can read a few bytes past the terminating NUL of a malformed
+    // sentence whose checksum still passes. The zeroed tail behind the length
+    // the library sees makes those reads land on NUL bytes, so the parser stops
+    // there instead of reading the members that follow the buffer.
+    static constexpr uint8_t NMEA_BUFFER_LEN = 100;
+    static constexpr size_t NMEA_BUFFER_GUARD = 16;
+
+private:
+    char _nmeaBuffer[NMEA_BUFFER_LEN + NMEA_BUFFER_GUARD] = {};
     MicroNMEA nmea;
     mesh::RTCClock* _clock;
     Stream* _gps_serial;
@@ -90,7 +100,7 @@ class MicroNMEALocationProvider : public LocationProvider {
 
 public :
     MicroNMEALocationProvider(Stream& ser, mesh::RTCClock* clock = NULL, int pin_reset = GPS_RESET, int pin_en = GPS_EN,RefCountedDigitalPin* peripher_power=NULL) :
-    nmea(_nmeaBuffer, sizeof(_nmeaBuffer)), _clock(clock), _gps_serial(&ser), _peripher_power(peripher_power), _pin_reset(pin_reset), _pin_en(pin_en) {
+    nmea(_nmeaBuffer, NMEA_BUFFER_LEN), _clock(clock), _gps_serial(&ser), _peripher_power(peripher_power), _pin_reset(pin_reset), _pin_en(pin_en) {
         if (_pin_reset != -1) {
             pinMode(_pin_reset, OUTPUT);
             digitalWrite(_pin_reset, GPS_RESET_ACTIVE);

test/mocks/Arduino.h

@@ -2,6 +2,7 @@
 
 #include <cstdint>
 #include <cmath>
+#include <ctype.h>  // Arduino.h provides the C character classification functions
 #include "Stream.h"
 
 inline uint32_t g_mock_millis = 0;
@@ -15,3 +16,26 @@ inline uint32_t millis() {
 inline void delay(uint32_t ms) {
   g_mock_millis += ms;
 }
+
+// Busy-wait loops call yield(); let mock time move so they can time out.
+inline void yield() {
+  g_mock_millis += 1;
+}
+
+// Minimal digital I/O so headers that drive enable/reset pins compile natively.
+constexpr uint8_t LOW = 0;
+constexpr uint8_t HIGH = 1;
+constexpr uint8_t INPUT = 0;
+constexpr uint8_t OUTPUT = 1;
+
+inline uint8_t g_mock_pin_levels[64] = {};
+
+inline void pinMode(uint8_t, uint8_t) {}
+
+inline void digitalWrite(uint8_t pin, uint8_t level) {
+  if (pin < 64) g_mock_pin_levels[pin] = level;
+}
+
+inline int digitalRead(uint8_t pin) {
+  return pin < 64 ? g_mock_pin_levels[pin] : LOW;
+}

test/mocks/Mesh.h

@@ -15,6 +15,13 @@ class Radio {
   virtual int16_t getNoiseFloor() { return -120; }
 };
 
+class RTCClock {
+public:
+  virtual ~RTCClock() = default;
+  virtual uint32_t getCurrentTime() = 0;
+  virtual void setCurrentTime(uint32_t time) = 0;
+};
+
 class MainBoard {
 public:
   virtual ~MainBoard() = default;

test/mocks/RTClib.h

@@ -0,0 +1,39 @@
+#pragma once
+
+#include <stdint.h>
+
+// Minimal DateTime stand-in for native tests. unixtime() matches RTClib for
+// valid dates from 2000 to 2099, the range RTClib supports (seconds since
+// 1970-01-01 UTC). Out-of-range fields map to a deterministic value instead of
+// indexing past the month table, so tests can compare two parses of the same
+// bytes; such values are not what RTClib would return.
+class DateTime {
+  uint16_t _year;
+  uint8_t _month, _day, _hour, _minute, _second;
+
+  static bool leap(uint16_t year) {
+    return year % 4 == 0 && (year % 100 != 0 || year % 400 == 0);
+  }
+
+public:
+  DateTime(uint16_t year, uint8_t month, uint8_t day, uint8_t hour,
+           uint8_t minute, uint8_t second)
+      : _year(year), _month(month), _day(day), _hour(hour),
+        _minute(minute), _second(second) {}
+
+  uint32_t unixtime() const {
+    uint32_t days = 0;
+    for (uint16_t year = 1970; year < _year; ++year)
+      days += leap(year) ? 366 : 365;
+    if (_month >= 1 && _month <= 12) {
+      const uint8_t month_days[] = {31, 28, 31, 30, 31, 30,
+                                    31, 31, 30, 31, 30, 31};
+      for (uint8_t month = 1; month < _month; ++month)
+        days += month_days[month - 1] + (month == 2 && leap(_year) ? 1 : 0);
+      days += _day - 1;
+    } else {
+      days += 31UL * _month + _day;  // deterministic, not calendar-correct
+    }
+    return days * 86400UL + _hour * 3600UL + _minute * 60UL + _second;
+  }
+};

test/test_gps_nmea/gps_test_support.h

@@ -0,0 +1,49 @@
+#pragma once
+
+// Shared helpers for the native GPS/NMEA tests: the real
+// MicroNMEALocationProvider and MicroNMEA, fed through a mock UART.
+
+#include <gtest/gtest.h>
+
+#include <MicroNMEA.h>
+#include <helpers/sensors/MicroNMEALocationProvider.h>
+
+#include <cstdio>
+#include <cstring>
+#include <deque>
+#include <string>
+
+
+struct MockSerial : Stream {
+  std::deque<char> queue;
+  int available() override { return (int)queue.size(); }
+  int read() override {
+    if (queue.empty()) return -1;
+    char c = queue.front();
+    queue.pop_front();
+    return (unsigned char)c;
+  }
+  void feed(const std::string& text) {
+    for (char c : text) queue.push_back(c);
+  }
+};
+
+// "$" + body + "*" + two hex digits, the checksum MicroNMEA expects.
+inline std::string withChecksum(const std::string& body) {
+  uint8_t sum = 0;
+  for (char c : body) sum ^= (uint8_t)c;
+  char hex[4];
+  snprintf(hex, sizeof(hex), "%02X", sum);
+  return "$" + body + "*" + hex;
+}
+
+inline void feedAndLoop(MicroNMEALocationProvider& gps, MockSerial& serial,
+                 const std::string& line) {
+  serial.feed(line + "\r\n");
+  gps.loop();
+}
+
+inline constexpr const char* kGoodRmc =
+    "GPRMC,120034.00,A,4716.1234,N,12233.5678,W,0.12,34.5,171026,003.1,W,A";
+inline constexpr const char* kGoodGga =
+    "GPGGA,120034.00,4716.1234,N,12233.5678,W,1,08,0.9,125.0,M,-17.0,M,,";

test/test_gps_nmea/test_buffer_guard.cpp

@@ -0,0 +1,163 @@
+#include "gps_test_support.h"
+
+#include <memory>
+#include <new>
+#include <random>
+
+namespace {
+
+constexpr size_t kLen = MicroNMEALocationProvider::NMEA_BUFFER_LEN;
+constexpr size_t kGuard = MicroNMEALocationProvider::NMEA_BUFFER_GUARD;
+
+// The library's checksum test stops at the first '*', while parseTime()
+// skips a fixed nine characters. A sentence whose checksum covers only
+// "GPRMC,1" therefore passes, and the bytes after the checksum are parsed as
+// fields. Both sentences below are 99 characters, the longest the buffer
+// holds, so the sentence NUL sits at index 99 and the last fixed-width reads
+// land past it.
+
+// Validity flag at index 97: processRMC steps onto the NUL and the latitude
+// parse reads indices 99 and 100.
+std::string overreadAtLatitude() {
+  std::string s = withChecksum("GPRMC,1");  // 11 chars
+  while (s.size() < 96) s.push_back('X');
+  s += ",AB";                                // ',' @96, 'A' @97, 'B' @98
+  return s;
+}
+
+// Date field at index 98: parseDate reads six bytes from index 98 (to 103)
+// and then looks for the next field at index 104.
+std::string overreadAtDate() {
+  std::string s = withChecksum("GPRMC,1");  // idx 0..10
+  s += "AAAAA";                              // rest of the 9-char time field
+  s += ",A,4716.1,N,";                       // validity, latitude, hemisphere
+  const std::string tail = ",W,0.1,0.2,";    // longitude end .. course
+  s += std::string(98 - tail.size() - s.size(), 'X');  // longitude filler
+  s += tail;
+  s += "1";                                  // date field starts at idx 98
+  return s;
+}
+
+// A line longer than the buffer: the library keeps overwriting its last byte.
+std::string longJunkLine() { return std::string(180, '7'); }
+
+// Parse the same bytes with a library instance whose buffer is followed by
+// NUL bytes, which is what the provider's guard tail must guarantee.
+struct ReferenceParse {
+  char buffer[100 + 64];
+  MicroNMEA nmea;
+  ReferenceParse() : buffer{}, nmea(buffer, 100) {}
+  void feed(const std::string& line) {
+    for (char c : line) nmea.process(c);
+    nmea.process('\n');
+  }
+};
+
+// Build the provider in memory that is not zero, so the test does not rely on
+// a freshly zeroed stack or heap to make the guard tail look clean.
+struct DirtyProvider {
+  alignas(MicroNMEALocationProvider) unsigned char storage[sizeof(MicroNMEALocationProvider)];
+  MicroNMEALocationProvider* gps;
+  explicit DirtyProvider(Stream& serial) {
+    memset(storage, 0xA5, sizeof(storage));
+    gps = new (storage) MicroNMEALocationProvider(serial, nullptr, -1, -1);
+  }
+  ~DirtyProvider() { gps->~MicroNMEALocationProvider(); }
+};
+
+// Checksum-valid sentences whose fields run up to the end of the buffer.
+// Field values stay short: MicroNMEA's parseFloat() also overflows a signed
+// long on 19+ digit fields, a separate library bug this suite does not cover.
+std::string malformedNearEnd(std::mt19937& rng) {
+  static const char alphabet[] = ",,,,AANNWWSSEE0123456789..**X";
+  std::string s = withChecksum(rng() % 2 ? "GPRMC,1" : "GPGGA,1");
+  const size_t length = 80 + rng() % 20;  // 80..99 characters
+  size_t run = 0;
+  while (s.size() < length) {
+    char c = alphabet[rng() % (sizeof(alphabet) - 1)];
+    run = (c >= '0' && c <= '9') ? run + 1 : 0;
+    if (run > 12) c = ',', run = 0;
+    s.push_back(c);
+  }
+  return s;
+}
+
+}  // namespace
+
+TEST(MicroNMEAProvider, SentenceLongerThanTheBufferIsStillRejected) {
+  // The guard tail must not change which sentences are accepted: a sentence
+  // of 100 or more characters is truncated and fails its checksum, as before.
+  std::string body = kGoodRmc;
+  while (body.size() < 100 - 4) body.push_back('Z');
+  const std::string sentence = withChecksum(body);
+  ASSERT_EQ(100u, sentence.size());
+
+  g_mock_millis = 1000;
+  MockSerial serial;
+  MicroNMEALocationProvider gps(serial, nullptr, -1, -1);
+  feedAndLoop(gps, serial, sentence);
+  EXPECT_FALSE(gps.isValid());
+}
+
+TEST(MicroNMEALibrary, NeverWritesPastTheLengthItIsGiven) {
+  // The premise of the guard tail: MicroNMEA 2.0.6 clamps writes at
+  // buffer[len - 1]. Long, truncated and malformed lines must leave the tail
+  // untouched, or it would stop being a run of NUL bytes.
+  std::unique_ptr<char[]> buffer(new char[kLen + kGuard]());
+  MicroNMEA nmea(buffer.get(), kLen);
+  std::mt19937 rng(7);
+  for (int i = 0; i < 2000; ++i) {
+    std::string line = (i % 3 == 0) ? longJunkLine() : malformedNearEnd(rng);
+    for (size_t k = rng() % 120; k > 0; --k) line.push_back((char)(' ' + rng() % 95));
+    for (char c : line) nmea.process(c);
+    nmea.process((i % 2) ? '\n' : '\r');
+    for (size_t k = kLen; k < kLen + kGuard; ++k) {
+      ASSERT_EQ(0, buffer[k]) << "tail byte " << k << " written at line " << i;
+    }
+  }
+}
+
+TEST(MicroNMEALibrary, ReadsStayInsideTheGuardTail) {
+  // Heap buffer of exactly the size the provider reserves. With the address
+  // sanitizer on, any parser read past the guard aborts this test.
+  std::unique_ptr<char[]> buffer(new char[kLen + kGuard]());
+  MicroNMEA nmea(buffer.get(), kLen);
+  auto feed = [&nmea](const std::string& line) {
+    for (char c : line) nmea.process(c);
+    nmea.process('\n');
+  };
+  feed(overreadAtLatitude());
+  feed(overreadAtDate());
+  std::mt19937 rng(11);
+  for (int i = 0; i < 20000; ++i) feed(malformedNearEnd(rng));
+  SUCCEED();
+}
+
+TEST(MicroNMEAProvider, MalformedSentenceParsesAsIfFollowedByNulBytes) {
+  // Without the guard tail the latitude parse reads the first byte of the
+  // MicroNMEA object that follows the buffer (the buffer length, 100), so the
+  // result depends on object layout. With it, the provider must parse the
+  // same latitude as a reference whose buffer is followed by NULs, also after
+  // a line long enough to have filled the whole buffer. Only raw parsed
+  // values are compared: isValid() and getTimestamp() are the provider's own
+  // decisions. The date-field over-read is covered by ReadsStayInsideTheGuardTail.
+  for (bool after_long_line : {false, true}) {
+    SCOPED_TRACE(after_long_line ? "after a long line" : "first line");
+    const std::string sentence = overreadAtLatitude();
+    ASSERT_EQ(99u, sentence.size());
+    g_mock_millis = 1000;
+    MockSerial serial;
+    DirtyProvider dirty(serial);
+    MicroNMEALocationProvider& gps = *dirty.gps;
+    ReferenceParse reference;
+    if (after_long_line) {
+      feedAndLoop(gps, serial, longJunkLine());
+      reference.feed(longJunkLine());
+    }
+    feedAndLoop(gps, serial, sentence);
+    reference.feed(sentence);
+    ASSERT_TRUE(reference.nmea.isValid());  // the sentence does reach the latitude parse
+    EXPECT_EQ(reference.nmea.getLatitude(), gps.getLatitude());
+    EXPECT_EQ(reference.nmea.getLongitude(), gps.getLongitude());
+  }
+}

test/test_gps_nmea/test_main.cpp

@@ -0,0 +1,9 @@
+#include "gps_test_support.h"
+
+// Defined in EnvironmentSensorManager.cpp, which is not part of this build.
+void LocationProvider::sendSentence(const char*) {}
+
+int main(int argc, char** argv) {
+  ::testing::InitGoogleTest(&argc, argv);
+  return RUN_ALL_TESTS();
+}