From 814a2d61fad1ec018439b67c2e1c3cde22aa6fa5 Mon Sep 17 00:00:00 2001 From: James Puderer Date: Sat, 9 Mar 2024 10:14:28 -0600 Subject: [PATCH 1/2] Prevents invalid sentences from causing crash Improves the current implementation of the parse() method to scan the entire sentence and count the fields before attempting to read the fields. This prevents accidental invalid pointer accesses when parsing invalid sentences with valid checksums. The new parse implementation also has a smaller compiled size. Includes a sketch in the "extras" directory (ignored by the Arduino IDE according to the library spec) that was used for testing the parsing of all sentences known to the parser. --- extras/NMEA_Parse_Test/NMEA_Parse_Test.ino | 395 ++++++++++++++++++ src/Adafruit_GPS.h | 1 + src/NMEA_parse.cpp | 439 ++++++++++----------- 3 files changed, 614 insertions(+), 221 deletions(-) create mode 100644 extras/NMEA_Parse_Test/NMEA_Parse_Test.ino diff --git a/extras/NMEA_Parse_Test/NMEA_Parse_Test.ino b/extras/NMEA_Parse_Test/NMEA_Parse_Test.ino new file mode 100644 index 0000000..04d802b --- /dev/null +++ b/extras/NMEA_Parse_Test/NMEA_Parse_Test.ino @@ -0,0 +1,395 @@ +#include +// +// Test code for the Adafruit GPS library. +// +// Specifically this code was used to test a refactoring of the library to make +// it more robust while ensuring that the parsing of correct sentences remained +// unchanged. +// +// This code will crash when used with the old parsing implementation unless +// TEST_INVALID_SENTENCES is undefined. + +Adafruit_GPS GPS; + +void setup() { + Serial.begin(115200); + delay(3000); // Wait a bit for USB serial to enumerate on some devices + Serial.println("Adafruit GPS parse test."); + // Use 0 as a seed, since we don't actually want truly random numbers + randomSeed(0); +} + +// Only set this if we are testing the new library, otherwise +// it guarantees a crash accessing and invalid pointer +#define TEST_INVALID_SENTENCES + +// How many generated sentances to make +#define GENERATE_COUNT 100 + +const char* GPS_SENTENCES_REAL[] = { + // These came from my actual Ultimate GPS module + "$GPGGA,200344.000,1329.6513,N,08923.3574,W,2,11,0.88,40.3,M,-0.7,M,0000,0000*5C", + "$GPRMC,200344.000,A,1329.6513,N,08923.3574,W,0.01,353.13,080324,,,D*7E", + "$GPGGA,200345.000,1329.6513,N,08923.3574,W,2,11,0.88,40.3,M,-0.7,M,0000,0000*5D", + "$GPRMC,200345.000,A,1329.6513,N,08923.3574,W,0.02,24.34,080324,,,D*4A", + "$GPGGA,200346.000,1329.6513,N,08923.3574,W,2,11,0.88,40.3,M,-0.7,M,0000,0000*5E", + "$GPRMC,200346.000,A,1329.6513,N,08923.3574,W,0.01,353.59,080324,,,D*72", + "$GPGGA,200347.000,1329.6513,N,08923.3574,W,2,11,0.88,40.3,M,-0.7,M,0000,0000*5F", + "$GPRMC,200347.000,A,1329.6513,N,08923.3574,W,0.00,99.87,080324,,,D*44", + "$GPGGA,200348.000,1329.6513,N,08923.3574,W,2,11,0.88,40.3,M,-0.7,M,0000,0000*50", + "$GPRMC,200348.000,A,1329.6513,N,08923.3574,W,0.01,179.99,080324,,,D*7A", + "$GPGGA,200349.000,1329.6513,N,08923.3574,W,2,11,0.88,40.3,M,-0.7,M,0000,0000*51", + "$GPRMC,200349.000,A,1329.6513,N,08923.3574,W,0.01,136.30,080324,,,D*73", + "$GPGGA,200350.000,1329.6513,N,08923.3574,W,2,11,0.88,40.3,M,-0.7,M,0000,0000*59", + "$GPRMC,200350.000,A,1329.6513,N,08923.3574,W,0.01,174.78,080324,,,D*71", + "$GPVTG,128.54,T,,M,0.21,N,0.39,K,D*3B", + + // These came from the output example on this Github project: + // https://github.com/luk-kop/nmea-gps-emulator + "$GPGGA,173124.00,5430.000,N,01921.029,E,1,09,0.92,15.2,M,32.5,M,,*6C", + "$GPGSA,A,3,22,11,27,01,03,02,10,21,19,,,,1.56,0.92,1.25*02", + "$GPGSV,4,1,15,26,25,138,53,16,25,091,67,01,51,238,77,02,45,085,41*79", + "$GPGSV,4,2,15,03,38,312,01,30,68,187,37,11,22,049,44,09,67,076,71*77", + "$GPGSV,4,3,15,10,14,177,12,19,86,235,37,21,84,343,95,22,77,040,66*79", + "$GPGSV,4,4,15,08,50,177,60,06,81,336,46,27,63,209,83*4C", + "$GPGLL,5430.000,N,01921.029,E,173124.000,A,A*59", + "$GPRMC,173124.000,A,5430.000,N,01921.029,E,10.500,90.0,051121,,,A*65", + "$GPHDT,90.0,T*0C", + "$GPVTG,90.0,T,,M,10.5,N,19.4,K*51", + "$GPZDA,173124.000,05,11,2021,0,0*50", + + // Sample from (https://learn.adafruit.com/adafruit-ultimate-gps-featherwing/antenna-options) for PGTOP" + "$PGTOP,11,3*6F", + "$PGTOP,11,2*6E", +}; + +// These came from my GPS when I wasn't reading the buffer quickly enough +const char* GPS_SENTENCES_INVALID[] = { + "$GPGGA,215937.5.1022,W,0.34,222.98,280124,,,D*73" +}; + +// This is the list of sentences types the library knows how to build +const char* BUILT_SENTENCES[] = {"GGA", "GLL", "RMC", "HDM", + "HDT", "MWV", "RMB", "TXT", + "VHW", "VPW", "WCV" }; + +// This is the list of full sentences created by hand to test the parser. +// These are sentences that are understood by the parsers, but are not built +// by the build() function, and not otherwise covered by any of the the REAL +// sentences declared above. +// +// These were generated by ChatGPT, so they might be nonsense. I expect +// the checksum is wrong. +const char* SYNTHETIC_TEST_SENTENCES[] = { + // DBT - Depth Below Transducer + "$IIDBT,030.5,f,009.3,M,005.0,F*18", + // MDA - Meteorological Composite + "$WIMDA,30.03,I,1.018,B,25.0,C,,,50.0,,2.5,C,1018.2,,*46", + // MTW - Water Temperature + "$IIMTW,15.6,C*11", + // VLW - Distance Traveled through Water + "$IIVLW,01234.56,N,000.0,N*7A", + // VWR - Wind Speed and Angle + "$IIVWR,045.,L,010.5,N,005.4,M,019.4,K*6F", + // XTE - Cross-Track Error + "$GPXTE,A,A,0.10,L,N*6F" +}; + +void loop() { + char *input; + char output[200]; + char invalid[200]; + char source[3]; + char sentence[4]; + boolean rval; + int array_length; + + Serial.println("********** REAL SENTENCES **********"); + // Test known valid sentences (from an actual GPS) + // We compare these manually, as the exact fromat from our builder will differ + array_length = sizeof(GPS_SENTENCES_REAL) / sizeof(GPS_SENTENCES_REAL[0]); + for (int i = 0; i < array_length; i++) { + input = (char*)GPS_SENTENCES_REAL[i]; + rval = GPS.parse(input); + source[0] = input[1]; source[1] = input[2]; source[2] = 0; + sentence[0] = input[3]; sentence[1] = input[4]; sentence[2] = input[5]; sentence[3] = 0; + + Serial.println("Input: " + String(input) + " - Parsed: " + (rval?"Ok":"Error")); + if (!rval) { + Serial.println(); + } else if (GPS.build(output, source, sentence)) { + Serial.println("Output: " + String(output)); + } else { + Serial.println("No build implementation.\n"); + } + } + + Serial.println("********** SYNTHETIC SENTENCES **********"); + // This is the list of full sentences created by hand to test the parser. + // In these cases there isn't a readily available example, and the library doesn't + // build these types of sentances + array_length = sizeof(SYNTHETIC_TEST_SENTENCES) / sizeof(SYNTHETIC_TEST_SENTENCES[0]); + for (int i = 0; i < array_length; i++) { + input = (char*)SYNTHETIC_TEST_SENTENCES[i]; + rval = GPS.parse(input); + source[0] = input[1]; source[1] = input[2]; source[2] = 0; + sentence[0] = input[3]; sentence[1] = input[4]; sentence[2] = input[5]; sentence[3] = 0; + + Serial.println("Input: " + String(input) + " - Parsed: " + (rval?"Ok":"Error")); + } + Serial.println(); + + Serial.println("********** BUILT SENTENCES (generated) **********"); + // Test sentences that this library knows how to build. Sadly, it doesn't seem to know how + // to build everything it knows how to parse + array_length = // Subtract one because of a weird terminator string + sizeof(BUILT_SENTENCES) / sizeof(BUILT_SENTENCES[0]); + uint32_t inputHash, outputHash; + bool match; + for (int j = 0; j < GENERATE_COUNT; j++) { + for (int i = 0; i < array_length; i++) { + updateGPS(); // Update GPS data with arbitrary values + inputHash = hash(GPS); // Get a hash of the GPS data + if (GPS.build(output, "GP", BUILT_SENTENCES[i])) { + output[strlen(output) - 1] = 0; // Remove carriage-return + Serial.print("Build output: " + String(output)); + rval = GPS.parse(output); + Serial.print(" - Parsed: " + String(rval?"Ok":"Error")); + outputHash = hash(GPS); // Get a hash of the GPS data + // The input and output hash should match. + // + // NOTE:That the TXT message hashes will not match the first time, + // because the build code for the TXT message adds some random hardcoded + // test data. + match = (inputHash == outputHash); + Serial.println(", Hash Match?: " + String(match?"Yes":"No")); + } else { + Serial.println("Build Error: " + String(BUILT_SENTENCES[i])); + } + } + } + Serial.println(); + + #ifdef TEST_INVALID_SENTENCES + Serial.println("********** INVALID SENTENCES (prefabbed) **********"); + // Test invalid sentences (ideally some with valid checksums) + // We compare these manually, as the exact fromat from our builder will differ + array_length = sizeof(GPS_SENTENCES_INVALID) / sizeof(GPS_SENTENCES_INVALID[0]); + for (int i = 0; i < array_length; i++) { + input = (char*)GPS_SENTENCES_INVALID[i]; + rval = GPS.parse(input); + source[0] = input[1]; source[1] = input[2]; source[2] = 0; + sentence[0] = input[3]; sentence[1] = input[4]; sentence[2] = input[5]; sentence[3] = 0; + + Serial.println("Input: " + String(input) + " - Parsed: " + (rval?"Ok":"Error")); + if (!rval) { + Serial.println(); + } else if (GPS.build(output, source, sentence)) { + Serial.println("Output: " + String(output)); + } else { + Serial.println("No build implementation.\n"); + } + } + + Serial.println("********** INVALID SENTENCES (generated) **********"); + // Test sentences that this library knows how to build. Sadly, it doesn't seem to know how + // to build everything it knows how to parse + array_length = // Subtract one because of a weird terminator string + sizeof(BUILT_SENTENCES) / sizeof(BUILT_SENTENCES[0]); + int index; + for (int j = 0; j < GENERATE_COUNT; j++) { + for (int i = 0; i < array_length; i++) { + updateGPS(); // Update GPS data with arbitrary values + inputHash = hash(GPS); // Get a hash of the GPS data + if (GPS.build(output, "GP", BUILT_SENTENCES[i])) { + // Randomly remove a character from the built output + int skip = random(strlen(output)); + int index = 0; + for (int k = 0; k < strlen(output); k++) { + if (k != skip) { + invalid[index] = output[k]; + index++; + } + } + // Remove then recalculate the checksum (this gets the sentence further + // in the parser and will cause a crash with older versions of the library) + char *p = strchr(invalid, '*'); + if (p != NULL) *p = '\0'; + GPS.addChecksum(invalid); + Serial.print("Invalid output: " + String(invalid)); + Serial.flush(); + rval = GPS.parse(invalid); + Serial.println(" - Parsed: " + String(rval?"Ok":"Error")); + } else { + Serial.println("Build Error: " + String(BUILT_SENTENCES[i])); + } + } + } + Serial.println(); + + #endif + + Serial.println("###########################################################"); + Serial.println("###########################################################"); + Serial.println("###########################################################"); + + while(1) delay(1); +} + +// Update the GPS with arbitrary data. We use this data to test the parse +// function by building NMEA sentences and then parsing them, then building them +// again using the results of the parsing and then making sure the sentences are +// the same. +void updateGPS() { + double t = millis() / 1000.; + double theta = t / 100.; // slow + double gamma = theta * 10; // faster + GPS.latitude = 4400 + sin(theta) * 60; + GPS.lat = 'N'; + GPS.longitude = 7600 + cos(theta) * 60; + GPS.lon = 'W'; + GPS.speed = 3 + sin(gamma); + GPS.hour = abs(cos(theta)) * 24; + GPS.minute = 30 + sin(theta / 2) * 30; + GPS.seconds = 30 + sin(gamma) * 30; + GPS.milliseconds = 500 + sin(gamma) * 500; + GPS.year = 1 + abs(sin(theta)) * 25; + GPS.month = 1 + abs(sin(gamma)) * 11; + GPS.day = 1 + abs(sin(gamma)) * 26; + GPS.satellites = abs(cos(gamma)) * 10; + + GPS.geoidheight = 1 + sin(theta); + GPS.altitude = 200 + cos(gamma) * 10; + GPS.angle = abs(sin(theta)) * 360; + GPS.magvariation = abs(cos(theta)) * 360; + GPS.HDOP = 5 + sin(gamma); + GPS.VDOP = 6 + cos(gamma); + GPS.PDOP = 7 + sin(gamma); + GPS.mag = (sin(theta) < 0)?'W':'E'; + GPS.fix = 1; + GPS.fixquality = 2; + GPS.fixquality_3d = 3; + + GPS.depthToKeel = 2 + cos(theta); + GPS.depthToTransducer = 4 + cos(theta); +} + +/* + * The FNV Hash, or more precisely the "FNV-1a alternate algorithm" + * See: http://www.isthe.com/chongo/tech/comp/fnv/ + * https://en.wikipedia.org/wiki/Fowler–Noll–Vo_hash_function + */ + +/* See the FNV parameters at www.isthe.com/chongo/tech/comp/fnv/#FNV-param */ +const uint32_t FNV_32_PRIME = 0x01000193; /* 16777619 */ +const uint32_t FNV_32_OFFSET = 0x811c9dc5; /* 2166136261 */ + +uint32_t hash(const char *str) { + size_t len = strlen(str); + unsigned char *s = (unsigned char *)str; /* unsigned string */ + + /* See the FNV parameters at www.isthe.com/chongo/tech/comp/fnv/#FNV-param */ + + uint32_t h = FNV_32_OFFSET; + while (len--) { + /* xor the bottom with the current octet */ + h ^= *s++; + /* multiply by the 32 bit FNV magic prime mod 2^32 */ + h *= FNV_32_PRIME; + } + return h; +} + +uint32_t hash(float f) { + uint32_t ui; + memcpy( &ui, &f, sizeof( float ) ); + uint32_t h = FNV_32_OFFSET; + + // Note: we ignore the least significant bits of the float + h ^= (ui & 0xff000000) >> 24; + h *= FNV_32_PRIME; + h ^= (ui & 0x00ff0000) >> 16; + h *= FNV_32_PRIME; + h ^= (ui & 0x0000ff00) >> 8; + h *= FNV_32_PRIME; + + return h; +} + +uint32_t hash(uint8_t x) { + uint32_t h = FNV_32_OFFSET; + return h ^ x * FNV_32_PRIME; +} + +uint32_t hash(char c) { + return hash((uint8_t)c); +} + +uint32_t hash(bool b) { + return hash((uint8_t)b); +} + +uint32_t hash(uint16_t x) { + uint32_t h = FNV_32_OFFSET; + h ^= x & 0xff; + h *= FNV_32_PRIME; + h ^= (x >> 8) & 0xff; + h *= FNV_32_PRIME; + return h; +} + +uint32_t hash(int x) { + uint32_t ui = (uint32_t)x; + + uint32_t h = FNV_32_OFFSET; + for (int i=0; i < sizeof(int); i++ ) { + h ^= ui & 0xff; + h *= FNV_32_PRIME; + ui = ui >> 8; + } + return h; +} + +// Generate a hash of the relevant GPS information. We use this to see if the GPS +// information is identical or is substantially similar. +uint32_t hash(Adafruit_GPS gps) { + uint32_t h = 0; + + h ^= hash(gps.hour); + h ^= hash(gps.minute); + h ^= hash(gps.seconds); + // Hash only the *tens* of milliseconds. Our build routine only outputs two digits + // of precision, so we won't parse the same number of milliseconds when we preparse + // our output. + h ^= hash(gps.milliseconds / 10); + h ^= hash(gps.year); + h ^= hash(gps.month); + h ^= hash(gps.day); + h ^= hash(gps.latitude); + h ^= hash(gps.longitude); + h ^= hash(gps.geoidheight); + h ^= hash(gps.altitude); + h ^= hash(gps.speed); + h ^= hash(gps.angle); + h ^= hash(gps.magvariation); + h ^= hash(gps.HDOP); + h ^= hash(gps.VDOP); + h ^= hash(gps.PDOP); + h ^= hash(gps.lat); + h ^= hash(gps.lon); + h ^= hash(gps.mag); + h ^= hash(gps.fix); + h ^= hash(gps.fixquality); + h ^= hash(gps.fixquality_3d); + h ^= hash(gps.satellites); + h ^= hash(gps.antenna); + h ^= hash(gps.depthToKeel); + h ^= hash(gps.depthToTransducer); + h ^= hash(gps.toID); + h ^= hash(gps.fromID); + h ^= hash(gps.txtTXT); + h ^= hash(gps.txtTot); + h ^= hash(gps.txtID); + h ^= hash(gps.txtN); + return h; +} diff --git a/src/Adafruit_GPS.h b/src/Adafruit_GPS.h index 60c5738..d304c01 100644 --- a/src/Adafruit_GPS.h +++ b/src/Adafruit_GPS.h @@ -243,6 +243,7 @@ class Adafruit_GPS : public Print { // NMEA_data.cpp void data_init(); // NMEA_parse.cpp + int findFields(char * fields[], char * nmea); const char *tokenOnList(char *token, const char **list); bool parseCoord(char *p, nmea_float_t *angleDegrees = NULL, nmea_float_t *angle = NULL, int32_t *angle_fixed = NULL, diff --git a/src/NMEA_parse.cpp b/src/NMEA_parse.cpp index 71ae51c..88c2424 100644 --- a/src/NMEA_parse.cpp +++ b/src/NMEA_parse.cpp @@ -22,6 +22,14 @@ #include +int Adafruit_GPS::findFields(char * fields[], char * nmea) { + char *p = nmea; + int count = 0; + while ((p = strchr(p, ',')) != NULL) + fields[count++] = ++p; + return count; +} + /**************************************************************************/ /*! @brief Parse a standard NMEA string and update the relevant variables. @@ -53,9 +61,10 @@ bool Adafruit_GPS::parse(char *nmea) { return false; // passed the check, so there's a valid source in thisSource and a valid // sentence in thisSentence - char *p = nmea; // Pointer to move through the sentence -- good parsers are - // non-destructive - p = strchr(p, ',') + 1; // Skip to char after the next comma, then check. + + char * fields[32]; // An array of pointers to the fields in the NMEA sentence + // Get indexes to each field in the nmea sentance + int fieldCount = findFields(fields, nmea); // This may look inefficient, but an M0 will get down the list in about 1 us / // strcmp()! Put the GPS sentences from Adafruit_GPS at the top to make @@ -63,66 +72,58 @@ bool Adafruit_GPS::parse(char *nmea) { // reading. if (!strcmp(thisSentence, "GGA")) { //************************************GGA // Adafruit from Actisense NGW-1 from SH CP150C - parseTime(p); - p = strchr(p, ',') + 1; // parse time with specialized function + // GGA sentences have 14 fields. If the number is otherwise, fail + if (fieldCount != 14) + return false; + parseTime(fields[0]); // parse out both latitude and direction, then go to next field, or fail - if (parseCoord(p, &latitudeDegrees, &latitude, &latitude_fixed, &lat)) + if (parseCoord(fields[1], &latitudeDegrees, &latitude, &latitude_fixed, &lat)) newDataValue(NMEA_LAT, latitudeDegrees); - p = strchr(p, ',') + 1; - p = strchr(p, ',') + 1; + // parse out both longitude and direction, then go to next field, or fail - if (parseCoord(p, &longitudeDegrees, &longitude, &longitude_fixed, &lon)) + if (parseCoord(fields[3], &longitudeDegrees, &longitude, &longitude_fixed, &lon)) newDataValue(NMEA_LON, longitudeDegrees); - p = strchr(p, ',') + 1; - p = strchr(p, ',') + 1; - if (!isEmpty(p)) { // if it's a , (or a * at end of sentence) the value is - // not included - fixquality = atoi(p); // needs additional processing + if (!isEmpty(fields[5])) { // if it's a , (or a * at end of sentence) the value is + // not included + fixquality = atoi(fields[5]); // needs additional processing if (fixquality > 0) { fix = true; lastFix = sentTime; } else fix = false; } - p = strchr(p, ',') + 1; // then move on to the next // Most can just be parsed with atoi() or atof(), then move on to the next. - if (!isEmpty(p)) - satellites = atoi(p); - p = strchr(p, ',') + 1; - if (!isEmpty(p)) - newDataValue(NMEA_HDOP, HDOP = atof(p)); - p = strchr(p, ',') + 1; - if (!isEmpty(p)) - altitude = atof(p); - p = strchr(p, ',') + 1; - p = strchr(p, ',') + 1; // skip the units - if (!isEmpty(p)) - geoidheight = atof(p); // skip the rest + if (!isEmpty(fields[6])) + satellites = atoi(fields[6]); + if (!isEmpty(fields[7])) + newDataValue(NMEA_HDOP, HDOP = atof(fields[7])); + if (!isEmpty(fields[8])) + altitude = atof(fields[8]); + // skip the units + if (!isEmpty(fields[10])) + geoidheight = atof(fields[10]); + // skip the rest } else if (!strcmp(thisSentence, "RMC")) { //*****************************RMC // in Adafruit from Actisense NGW-1 from SH CP150C - parseTime(p); - p = strchr(p, ',') + 1; - parseFix(p); - p = strchr(p, ',') + 1; + // RMC sentences have 11 or 12 fields (FAA mode indicator in NMEA 2.3 and later) + // If the number is otherwise, fail + if ((fieldCount != 11) && (fieldCount != 12)) + return false; + parseTime(fields[0]); + parseFix(fields[1]); // parse out both latitude and direction, then go to next field, or fail - if (parseCoord(p, &latitudeDegrees, &latitude, &latitude_fixed, &lat)) + if (parseCoord(fields[2], &latitudeDegrees, &latitude, &latitude_fixed, &lat)) newDataValue(NMEA_LAT, latitudeDegrees); - p = strchr(p, ',') + 1; - p = strchr(p, ',') + 1; // parse out both longitude and direction, then go to next field, or fail - if (parseCoord(p, &longitudeDegrees, &longitude, &longitude_fixed, &lon)) + if (parseCoord(fields[4], &longitudeDegrees, &longitude, &longitude_fixed, &lon)) newDataValue(NMEA_LON, longitudeDegrees); - p = strchr(p, ',') + 1; - p = strchr(p, ',') + 1; - if (!isEmpty(p)) - newDataValue(NMEA_SOG, speed = atof(p)); - p = strchr(p, ',') + 1; - if (!isEmpty(p)) - newDataValue(NMEA_COG, angle = atof(p)); - p = strchr(p, ',') + 1; - if (!isEmpty(p)) { - uint32_t fulldate = atof(p); + if (!isEmpty(fields[6])) + newDataValue(NMEA_SOG, speed = atof(fields[6])); + if (!isEmpty(fields[7])) + newDataValue(NMEA_COG, angle = atof(fields[7])); + if (!isEmpty(fields[8])) { + uint32_t fulldate = atof(fields[8]); day = fulldate / 10000; month = (fulldate % 10000) / 100; year = (fulldate % 100); @@ -131,39 +132,34 @@ bool Adafruit_GPS::parse(char *nmea) { } else if (!strcmp(thisSentence, "GLL")) { //*****************************GLL // in Adafruit from Actisense NGW-1 from SH CP150C + // GLL sentences have 6 or 7 fields (FAA mode indicator in NMEA 2.3 and later) + // If the number is otherwise, fail + if ((fieldCount != 6) && (fieldCount != 7)) + return false; // parse out both latitude and direction, then go to next field, or fail - if (parseCoord(p, &latitudeDegrees, &latitude, &latitude_fixed, &lat)) + if (parseCoord(fields[0], &latitudeDegrees, &latitude, &latitude_fixed, &lat)) newDataValue(NMEA_LAT, latitudeDegrees); - p = strchr(p, ',') + 1; - p = strchr(p, ',') + 1; // parse out both longitude and direction, then go to next field, or fail - if (parseCoord(p, &longitudeDegrees, &longitude, &longitude_fixed, &lon)) + if (parseCoord(fields[2], &longitudeDegrees, &longitude, &longitude_fixed, &lon)) newDataValue(NMEA_LON, longitudeDegrees); - p = strchr(p, ',') + 1; - p = strchr(p, ',') + 1; - parseTime(p); - p = strchr(p, ',') + 1; - parseFix(p); // skip the rest + parseTime(fields[4]); + parseFix(fields[5]); // skip the rest } else if (!strcmp(thisSentence, "GSA")) { //*****************************GSA // in Adafruit from Actisense NGW-1 - p = strchr(p, ',') + 1; // skip selection mode - if (!isEmpty(p)) - fixquality_3d = atoi(p); - p = strchr(p, ',') + 1; - // skip 12 Satellite PDNs without interpreting them - for (int i = 0; i < 12; i++) - p = strchr(p, ',') + 1; - if (!isEmpty(p)) - PDOP = atof(p); - p = strchr(p, ',') + 1; + // GSA sentences have 17 fields. If the number is otherwise, fail + if (fieldCount != 17) + return false; + if (!isEmpty(fields[1])) + fixquality_3d = atoi(fields[1]); + if (!isEmpty(fields[14])) + PDOP = atof(fields[14]); // parse out HDOP, we also parse this from the GGA sentence. Chipset should // report the same for both - if (!isEmpty(p)) - newDataValue(NMEA_HDOP, HDOP = atof(p)); - p = strchr(p, ',') + 1; - if (!isEmpty(p)) - VDOP = atof(p); // last before checksum + if (!isEmpty(fields[15])) + newDataValue(NMEA_HDOP, HDOP = atof(fields[15])); + if (!isEmpty(fields[16])) + VDOP = atof(fields[16]); // last before checksum } else if (!strcmp(thisSentence, "TOP")) { //*****************************TOP // See: @@ -171,8 +167,11 @@ bool Adafruit_GPS::parse(char *nmea) { // There is an output sentence that will tell you the status of the // antenna. $PGTOP,11,x where x is the status number. If x is 3 that means // it is using the external antenna. If x is 2 it's using the internal - p = strchr(p, ',') + 1; - parseAntenna(p); + // + // TOP sentences have 2 fields. If the number is otherwise, fail + if (fieldCount != 2) + return false; + parseAntenna(fields[1]); } #ifdef NMEA_EXTENSIONS // Sentences not required for basic GPS functionality @@ -184,18 +183,18 @@ bool Adafruit_GPS::parse(char *nmea) { // from Actisense NGW-1 // feet, metres, fathoms below transducer coerced to water depth from // surface in metres - if (!isEmpty(p)) + // + // DBT sentences have 6 fields. If the number is otherwise, fail + if (fieldCount != 6) + return false; + if (!isEmpty(fields[0])) newDataValue(NMEA_DEPTH, - (nmea_float_t)atof(p) * 0.3048f + depthToTransducer); - p = strchr(p, ',') + 1; - p = strchr(p, ',') + 1; - if (!isEmpty(p)) - newDataValue(NMEA_DEPTH, (nmea_float_t)atof(p) + depthToTransducer); - p = strchr(p, ',') + 1; - p = strchr(p, ',') + 1; - if (!isEmpty(p)) + (nmea_float_t)atof(fields[0]) * 0.3048f + depthToTransducer); + if (!isEmpty(fields[2])) + newDataValue(NMEA_DEPTH, (nmea_float_t)atof(fields[2]) + depthToTransducer); + if (!isEmpty(fields[4])) newDataValue(NMEA_DEPTH, - (nmea_float_t)atof(p) * 6 * 0.3048f + depthToTransducer); + (nmea_float_t)atof(fields[4]) * 6 * 0.3048f + depthToTransducer); } else if (!strcmp(thisSentence, "DPT")) { //*****************************DPT // from Actisense NGW-1 @@ -210,31 +209,34 @@ bool Adafruit_GPS::parse(char *nmea) { return false; } else if (!strcmp(thisSentence, "HDM")) { //*****************************HDM - if (!isEmpty(p)) - newDataValue(NMEA_HDG, atof(p)); // skip the rest + // HDM sentences have 2 fields. If the number is otherwise, fail + if (fieldCount != 2) + return false; + if (!isEmpty(fields[0])) + newDataValue(NMEA_HDG, atof(fields[0])); // skip the rest } else if (!strcmp(thisSentence, "HDT")) { //*****************************HDT - if (!isEmpty(p)) - newDataValue(NMEA_HDT, atof(p)); // skip the rest + // HDT sentences have 2 fields. If the number is otherwise, fail + if (fieldCount != 2) + return false; + if (!isEmpty(fields[0])) + newDataValue(NMEA_HDT, atof(fields[0])); // skip the rest } else if (!strcmp(thisSentence, "MDA")) { //*****************************MDA // from Actisense NGW-1 - if (!isEmpty(p)) - newDataValue(NMEA_BAROMETER, atof(p) * 3386.39); - p = strchr(p, ',') + 1; - p = strchr(p, ',') + 1; - if (!isEmpty(p)) - newDataValue(NMEA_BAROMETER, atof(p) * 100000); - p = strchr(p, ',') + 1; - p = strchr(p, ',') + 1; + // MDA sentences have 15 fields. If the number is otherwise, fail + if (fieldCount != 15) + return false; + if (!isEmpty(fields[0])) + newDataValue(NMEA_BAROMETER, atof(fields[0]) * 3386.39); + if (!isEmpty(fields[2])) + newDataValue(NMEA_BAROMETER, atof(fields[2]) * 100000); nmea_float_t T = 100000.; char u = 'C'; - if (!isEmpty(p)) - T = atof(p); - p = strchr(p, ',') + 1; - if (!isEmpty(p)) - u = *p; - p = strchr(p, ',') + 1; + if (!isEmpty(fields[4])) + T = atof(fields[4]); + if (!isEmpty(fields[5])) + u = *(fields[5]); if (u != 'C') { T = (T - 32) / 1.8f; u = 'C'; @@ -243,29 +245,29 @@ bool Adafruit_GPS::parse(char *nmea) { newDataValue(NMEA_TEMPERATURE_AIR, T); T = 100000.; u = 'C'; - if (!isEmpty(p)) - T = atof(p); - p = strchr(p, ',') + 1; - if (!isEmpty(p)) - u = *p; - p = strchr(p, ',') + 1; + if (!isEmpty(fields[7])) + T = atof(fields[7]); + if (!isEmpty(fields[8])) + u = *(fields[8]); if (u != 'C') { T = (T - 32) / 1.8f; u = 'C'; } if (T < 1000) newDataValue(NMEA_TEMPERATURE_WATER, T); - if (!isEmpty(p)) - newDataValue(NMEA_HUMIDITY, atof(p)); // skip the rest + if (!isEmpty(fields[9])) + newDataValue(NMEA_HUMIDITY, atof(fields[9])); // skip the rest } else if (!strcmp(thisSentence, "MTW")) { //*****************************MTW + // MTW sentences have 2 fields. If the number is otherwise, fail + if (fieldCount != 2) + return false; nmea_float_t T = 100000.; char u = 'C'; - if (!isEmpty(p)) - T = atof(p); - p = strchr(p, ',') + 1; - if (!isEmpty(p)) - u = *p; // last before checksum + if (!isEmpty(fields[0])) + T = atof(fields[0]); + if (!isEmpty(fields[1])) + u = *(fields[1]); // last before checksum if (u != 'C') { T = (T - 32) / 1.8f; u = 'C'; @@ -279,25 +281,24 @@ bool Adafruit_GPS::parse(char *nmea) { } else if (!strcmp(thisSentence, "MWV")) { //*****************************MWV // from Actisense NGW-1 + // MWV sentences have 5 fields. If the number is otherwise, fail + if (fieldCount != 5) + return false; nmea_float_t ang = 100000.; char ref = 'T'; - if (!isEmpty(p)) - ang = atof(p); - p = strchr(p, ',') + 1; - if (!isEmpty(p)) - ref = *p; - p = strchr(p, ',') + 1; + if (!isEmpty(fields[0])) + ang = atof(fields[0]); + if (!isEmpty(fields[1])) + ref = *(fields[1]); nmea_float_t spd = 100000.; - if (!isEmpty(p)) - spd = atof(p); - p = strchr(p, ',') + 1; + if (!isEmpty(fields[2])) + spd = atof(fields[2]); char units = 'N'; - if (!isEmpty(p)) - units = *p; - p = strchr(p, ',') + 1; + if (!isEmpty(fields[3])) + units = *(fields[3]); char stat = 'A'; - if (!isEmpty(p)) - stat = *p; // last before checksum + if (!isEmpty(fields[4])) + stat = *(fields[4]); // last before checksum if (units == 'K') { spd /= 1.6f; units = 'M'; @@ -337,26 +338,28 @@ bool Adafruit_GPS::parse(char *nmea) { // 11) Bearing to destination in degrees True // 12) Destination closing velocity in knots // 13) Arrival Status, A = Arrival Circle Entered 14) Checksum - p = strchr(p, ',') + 1; // skip status + // + // RMB sentences have 13 or 14 fields (FAA mode indicator in NMEA 2.3 and later) + // If the number is otherwise, fail + if ((fieldCount != 13) && (fieldCount != 14)) + return false; + + // skip status - fields[0] nmea_float_t xte = 100000.; char xteDir = 'X'; - if (!isEmpty(p)) - xte = atof(p); - p = strchr(p, ',') + 1; - if (!isEmpty(p)) - xteDir = *p; - p = strchr(p, ',') + 1; + if (!isEmpty(fields[1])) + xte = atof(fields[1]); + if (!isEmpty(fields[2])) + xteDir = *(fields[2]); if (xte < 10000.0f && xteDir != 'X') { if (xteDir == 'L') xte *= -1.0f; newDataValue(NMEA_XTE, xte); } - if (!isEmpty(p)) - parseStr(toID, p, NMEA_MAX_WP_ID); - p = strchr(p, ',') + 1; - if (!isEmpty(p)) - parseStr(fromID, p, NMEA_MAX_WP_ID); - p = strchr(p, ',') + 1; + if (!isEmpty(fields[3])) + parseStr(toID, fields[3], NMEA_MAX_WP_ID); + if (!isEmpty(fields[4])) + parseStr(fromID, fields[4], NMEA_MAX_WP_ID); nmea_float_t latitudeWP = 0; nmea_float_t longitudeWP = 0; int32_t latitude_fixedWP = 0; @@ -368,34 +371,28 @@ bool Adafruit_GPS::parse(char *nmea) { // parse out both latitude and direction for WayPoint, then go to next // field, or fail - if (!isEmpty(p)) { - if (!parseCoord(p, &latitudeDegreesWP, &latitudeWP, &latitude_fixedWP, + if (!isEmpty(fields[5])) { + if (!parseCoord(fields[5], &latitudeDegreesWP, &latitudeWP, &latitude_fixedWP, &latWP)) return false; else newDataValue(NMEA_LATWP, latitudeDegreesWP); } - p = strchr(p, ',') + 1; - p = strchr(p, ',') + 1; // parse out both longitude and direction for WayPoint, then go to next // field, or fail - if (!isEmpty(p)) { - if (!parseCoord(p, &longitudeDegreesWP, &longitudeWP, &longitude_fixedWP, + if (!isEmpty(fields[7])) { + if (!parseCoord(fields[7], &longitudeDegreesWP, &longitudeWP, &longitude_fixedWP, &lonWP)) return false; else newDataValue(NMEA_LONWP, longitudeDegreesWP); } - p = strchr(p, ',') + 1; - p = strchr(p, ',') + 1; - if (!isEmpty(p)) - newDataValue(NMEA_DISTWP, atof(p)); - p = strchr(p, ',') + 1; - if (!isEmpty(p)) - newDataValue(NMEA_COGWP, atof(p)); - p = strchr(p, ',') + 1; - if (!isEmpty(p)) - newDataValue(NMEA_VMGWP, atof(p)); // skip arrival flag + if (!isEmpty(fields[9])) + newDataValue(NMEA_DISTWP, atof(fields[9])); + if (!isEmpty(fields[10])) + newDataValue(NMEA_COGWP, atof(fields[10])); + if (!isEmpty(fields[11])) + newDataValue(NMEA_VMGWP, atof(fields[11])); // skip arrival flag } else if (!strcmp(thisSentence, "ROT")) { //*****************************ROT return false; @@ -408,17 +405,17 @@ bool Adafruit_GPS::parse(char *nmea) { return false; } else if (!strcmp(thisSentence, "TXT")) { //*****************************TXT - if (!isEmpty(p)) - txtTot = atoi(p); - p = strchr(p, ',') + 1; - if (!isEmpty(p)) - txtN = atoi(p); - p = strchr(p, ',') + 1; - if (!isEmpty(p)) - txtID = atoi(p); - p = strchr(p, ',') + 1; - if (!isEmpty(p)) - parseStr(txtTXT, p, 61); // copy the text to NMEA TXT max of 61 characters + // TXT sentences have 4 fields. If the number is otherwise, fail + if (fieldCount != 4) + return false; + if (!isEmpty(fields[0])) + txtTot = atoi(fields[0]); + if (!isEmpty(fields[1])) + txtN = atoi(fields[1]); + if (!isEmpty(fields[2])) + txtID = atoi(fields[2]); + if (!isEmpty(fields[3])) + parseStr(txtTXT, fields[3], 61); // copy the text to NMEA TXT max of 61 characters } else if (!strcmp(thisSentence, "VDR")) { //*****************************VDR // from Actisense NGW-1 @@ -426,35 +423,36 @@ bool Adafruit_GPS::parse(char *nmea) { } else if (!strcmp(thisSentence, "VHW")) { //*****************************VHW // from Actisense NGW-1 - if (!isEmpty(p)) - newDataValue(NMEA_HDT, atof(p)); - p = strchr(p, ',') + 1; - p = strchr(p, ',') + 1; - if (!isEmpty(p)) - newDataValue(NMEA_HDG, atof(p)); - p = strchr(p, ',') + 1; - p = strchr(p, ',') + 1; - if (!isEmpty(p)) - newDataValue(NMEA_VTW, atof(p)); // skip the other units + // VHW sentences have 8 fields. If the number is otherwise, fail + if (fieldCount != 8) + return false; + if (!isEmpty(fields[0])) + newDataValue(NMEA_HDT, atof(fields[0])); + if (!isEmpty(fields[2])) + newDataValue(NMEA_HDG, atof(fields[2])); + if (!isEmpty(fields[4])) + newDataValue(NMEA_VTW, atof(fields[4])); // skip the other units } else if (!strcmp(thisSentence, "VLW")) { //*****************************VLW // from Actisense NGW-1 - if (!isEmpty(p)) - newDataValue(NMEA_LOG, atof(p)); - p = strchr(p, ',') + 1; - p = strchr(p, ',') + 1; - if (!isEmpty(p)) - newDataValue(NMEA_LOGR, atof(p)); // skip the other units + // VLW sentences have 4 fields. If the number is otherwise, fail + if (fieldCount != 4) + return false; + if (!isEmpty(fields[0])) + newDataValue(NMEA_LOG, atof(fields[0])); + if (!isEmpty(fields[2])) + newDataValue(NMEA_LOGR, atof(fields[2])); // skip the other units } else if (!strcmp(thisSentence, "VPW")) { //*****************************VPW + // VPW sentences have 4 fields. If the number is otherwise, fail + if (fieldCount != 4) + return false; // knots, metres/s coerced to knots nmea_float_t vmg = 100000.; - if (!isEmpty(p)) - vmg = atof(p); - p = strchr(p, ',') + 1; - p = strchr(p, ',') + 1; - if (!isEmpty(p)) - vmg = atof(p) * 0.3048 * 3600. / 6000.; // skip units + if (!isEmpty(fields[0])) + vmg = atof(fields[0]); + if (!isEmpty(fields[2])) + vmg = atof(fields[2]) * 0.3048 * 3600. / 6000.; // skip units if (vmg < 1000.0f) newDataValue(NMEA_VMG, vmg); } else if (!strcmp(thisSentence, "VTG")) { //*****************************VTG @@ -463,37 +461,33 @@ bool Adafruit_GPS::parse(char *nmea) { } else if (!strcmp(thisSentence, "VWR")) { //*****************************VWR // from Actisense NGW-1 + // VWR sentences have 8 fields. If the number is otherwise, fail + if (fieldCount != 8) + return false; nmea_float_t ang = 1000.; - if (!isEmpty(p)) - ang = atof(p); - p = strchr(p, ',') + 1; + if (!isEmpty(fields[0])) + ang = atof(fields[0]); char ref = ' '; - if (!isEmpty(p)) - ref = *p; - p = strchr(p, ',') + 1; + if (!isEmpty(fields[1])) + ref = *(fields[1]); if (ref == 'L') ang *= -1; if (ang < 1000.0f) newDataValue(NMEA_AWA, ang); nmea_float_t ws = 0.0; char units = 'X'; - if (!isEmpty(p)) - ws = atof(p); - p = strchr(p, ',') + 1; // knots - if (!isEmpty(p)) - units = *p; - p = strchr(p, ',') + 1; - if (!isEmpty(p)) - ws = atof(p); - p = strchr(p, ',') + 1; // meters / second - if (!isEmpty(p)) - units = *p; - p = strchr(p, ',') + 1; // M - if (!isEmpty(p)) - ws = atof(p); - p = strchr(p, ',') + 1; // kilometers / hour can be converted back to knots - if (!isEmpty(p)) - units = *p; // last before checksum + if (!isEmpty(fields[2])) + ws = atof(fields[2]); + if (!isEmpty(fields[3])) + units = *(fields[3]); + if (!isEmpty(fields[4])) + ws = atof(fields[4]); + if (!isEmpty(fields[5])) + units = *(fields[5]); + if (!isEmpty(fields[6])) + ws = atof(fields[6]); + if (!isEmpty(fields[7])) + units = *(fields[7]); // last before checksum if (units == 'M') { ws *= 3.6f; units = 'K'; @@ -511,21 +505,24 @@ bool Adafruit_GPS::parse(char *nmea) { } else if (!strcmp(thisSentence, "WCV")) { //*****************************WCV // from SH CP150C - if (!isEmpty(p)) - newDataValue(NMEA_VMGWP, atof(p)); // skip the rest + // WCV sentences have 3 fields. If the number is otherwise, fail + if (fieldCount != 3) + return false; + if (!isEmpty(fields[0])) + newDataValue(NMEA_VMGWP, atof(fields[0])); // skip the rest } else if (!strcmp(thisSentence, "XTE")) { //*****************************XTE // from Actisense NGW-1 from SH CP150C - p = strchr(p, ',') + 1; // skip status 1 - p = strchr(p, ',') + 1; // skip status 2 + // XTE sentences have 5 or 6 fields (FAA mode indicator in NMEA 2.3 and later) + // If the number is otherwise, fail + if ((fieldCount != 5) && (fieldCount != 6)) + return false; nmea_float_t xte = 100000.; char xteDir = 'X'; - if (!isEmpty(p)) - xte = atof(p); - p = strchr(p, ',') + 1; - if (!isEmpty(p)) - xteDir = *p; - p = strchr(p, ',') + 1; + if (!isEmpty(fields[2])) + xte = atof(fields[2]); + if (!isEmpty(fields[3])) + xteDir = *(fields[3]); if (xte < 10000.0f && xteDir != 'X') { if (xteDir == 'L') xte *= -1.0f; From 312107074e95ddf2d795c04c0e462ae235b3f6fe Mon Sep 17 00:00:00 2001 From: James Puderer Date: Sat, 9 Mar 2024 10:20:49 -0600 Subject: [PATCH 2/2] Check for null pointer before dereference. Implementation of isEmpty() was incorrect since it performed a null check after dereferencing the pointer --- src/NMEA_parse.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/NMEA_parse.cpp b/src/NMEA_parse.cpp index 88c2424..95dd4e1 100644 --- a/src/NMEA_parse.cpp +++ b/src/NMEA_parse.cpp @@ -845,7 +845,7 @@ bool Adafruit_GPS::parseAntenna(char *p) { */ /**************************************************************************/ bool Adafruit_GPS::isEmpty(char *pStart) { - if (',' != *pStart && '*' != *pStart && pStart != NULL) + if (pStart != NULL && ',' != *pStart && '*' != *pStart) return false; else return true;