Merge pull request #182 from ladyada-eagleclaw/fix-copy-warnings-144

Fix length-derived string-copy warnings and field termination

extras/tests/parse_str/parse_str.ino

@@ -1,6 +1,12 @@
-// Two-sentence regression test. Runs without a GPS receiver.
+// String-copy and sentence bounds regression. Runs without a GPS receiver.
 
+// Test the private field helper without changing the library's public API.
+#include <Arduino.h>
+#include <SPI.h>
+#include <Wire.h>
+#define private public
 #include <Adafruit_GPS.h>
+#undef private
 
 Adafruit_GPS GPS;
 bool testsPassed = false;
@@ -41,14 +47,42 @@ void setup() {
     return;
   }
   Serial.println("PASS: previous result preserved");
+
+  // Each destination starts with nonzero bytes so missing terminators fail.
+  if (!checkField("abc,tail", 8, "abc") ||
+      !checkField("abc*00", 8, "abc") ||
+      !checkField("abc", 8, "abc") ||
+      !checkField("abcdef,tail", 4, "abc") ||
+      !checkField("abcdef*00", 4, "abc") ||
+      !checkField("abcdef", 4, "abc") ||
+      !checkField("abc", 1, "") ||
+      !checkField("", 8, "")) {
+    return;
+  }
   testsPassed = true;
 }
 
 void loop() {
   if (testsPassed) {
-    Serial.println("PASS: two-sentence regression");
+    Serial.println("PASS: string-copy regression");
   } else {
-    Serial.println("FAIL: two-sentence regression");
+    Serial.println("FAIL: string-copy regression");
   }
   delay(2000);
 }
+
+bool checkField(const char *value, uint8_t capacity, const char *expected) {
+  char input[24];
+  char result[9];
+  strcpy(input, value);
+  memset(result, '?', sizeof(result));
+  if (GPS.parseStr(result, input, capacity) != result ||
+      memcmp(result, expected, strlen(expected) + 1) != 0 ||
+      result[capacity] != '?') {
+    Serial.println("FAIL: copied field or destination bounds");
+    return false;
+  }
+  Serial.print("PASS: copied field ");
+  Serial.println(value);
+  return true;
+}

src/NMEA_build.cpp

@@ -73,10 +73,12 @@ char *Adafruit_GPS::build(char *nmea, const char *thisSource,
     return NULL;
   *nmea = '$';
   char *p = nmea + 1; // Pointer to move through the sentence
-  strncpy(p, thisSource, strlen(thisSource));
-  p += strlen(thisSource);
-  strncpy(p, thisSentence, strlen(thisSentence));
-  p += strlen(thisSentence);
+  size_t len = strlen(thisSource);
+  memcpy(p, thisSource, len);
+  p += len;
+  len = strlen(thisSentence);
+  memcpy(p, thisSentence, len);
+  p += len;
   *p = ',';
   p += 1; // Now $XXSSS, and need to add argument fields
   // This may look inefficient, but an M0 will get down the list in about 1 us /

src/NMEA_parse.cpp

@@ -769,12 +769,12 @@ bool Adafruit_GPS::parseCoord(char *pStart, nmea_float_t *angleDegrees,
   char *p = pStart;
   if (!isEmpty(p)) {
     // get the number in DDDMM.mmmm format and break into components
-    char degreebuff[10] = {0}; // Ensure string is terminated after strncpy
+    char degreebuff[10] = {0}; // Ensure string is terminated after copying
     char *e = strchr(p, '.');
     char *comma = strchr(p, ',');
     if (e == NULL || comma == NULL || e > comma || e - p > 6)
-      return false;                // no decimal point in range
-    strncpy(degreebuff, p, e - p); // get DDDMM
+      return false;               // no decimal point in range
+    memcpy(degreebuff, p, e - p); // get DDDMM
     long dddmm = atol(degreebuff);
     long degrees = (dddmm / 100);         // truncate the minutes
     long minutes = dddmm - degrees * 100; // remove the degrees
@@ -837,22 +837,17 @@ bool Adafruit_GPS::parseCoord(char *pStart, nmea_float_t *angleDegrees,
 /**************************************************************************/
 char *Adafruit_GPS::parseStr(char *buff, char *p, int n) {
   char *e = strchr(p, ',');
-  int len = 0;
-  if (e) {
-    len = min(int(e - p), n - 1);
-    strncpy(buff, p, len); // copy up to the comma
-    buff[len] = 0;
-  } else {
+  if (!e) {
     e = strchr(p, '*');
-    if (e) {
-      len = min(int(e - p), n - 1);
-      strncpy(buff, p, len); // or up to the *
-      buff[len] = 0;
-    } else {
-      len = min((int)strlen(p), n - 1);
-      strncpy(buff, p, len); // or to the end or max capacity
-    }
   }
+  int len;
+  if (e) {
+    len = min((int)(e - p), n - 1);
+  } else {
+    len = min((int)strlen(p), n - 1);
+  }
+  memcpy(buff, p, len);
+  buff[len] = 0;
   return buff;
 }