From ca781d1d35b44769448e16f7b4aaf6760d813c80 Mon Sep 17 00:00:00 2001 From: Alexander Entinger Date: Wed, 20 Jan 2021 15:17:40 +0100 Subject: [PATCH 1/7] Eliminate synced/synching state machine by always flushing the parser buffer when the start of a valid message is detected. --- src/ArduinoNmeaParser.cpp | 14 +++++--------- src/ArduinoNmeaParser.h | 6 ------ 2 files changed, 5 insertions(+), 15 deletions(-) diff --git a/src/ArduinoNmeaParser.cpp b/src/ArduinoNmeaParser.cpp index 3bc8436..90c41a0 100644 --- a/src/ArduinoNmeaParser.cpp +++ b/src/ArduinoNmeaParser.cpp @@ -26,7 +26,6 @@ ArduinoNmeaParser::ArduinoNmeaParser(OnRmcUpdateFunc on_rmc_update, OnGgaUpdateFunc on_gga_update) : _error{Error::None} -, _parser_state{ParserState::Synching} , _parser_buf{{0}, 0} , _rmc{nmea::INVALID_RMC} , _gga{nmea::INVALID_GGA} @@ -42,15 +41,12 @@ ArduinoNmeaParser::ArduinoNmeaParser(OnRmcUpdateFunc on_rmc_update, void ArduinoNmeaParser::encode(char const c) { - /* Wait for the first '$' to be received which - * indicates the start of a NMEA message. + /* Flash the whole parser buffer everytime we encounter + * a '$' sign. This way the parser buffer always starts + * with a valid NMEA message. */ - if (_parser_state == ParserState::Synching) { - if (c == '$') - _parser_state = ParserState::Synced; - else - return; - } + if (c == '$') + flushParserBuffer(); if (!isParseBufferFull()) addToParserBuffer(c); diff --git a/src/ArduinoNmeaParser.h b/src/ArduinoNmeaParser.h index aa920b5..3f4cc79 100644 --- a/src/ArduinoNmeaParser.h +++ b/src/ArduinoNmeaParser.h @@ -64,13 +64,7 @@ class ArduinoNmeaParser size_t elems_in_buf; } ParserBuffer; - enum class ParserState - { - Synching, Synced - }; - Error _error; - ParserState _parser_state; ParserBuffer _parser_buf; nmea::RmcData _rmc; nmea::GgaData _gga; From 062a724f468c9012db1a16b17e3ae1477b6c1ca2 Mon Sep 17 00:00:00 2001 From: Alexander Entinger Date: Wed, 20 Jan 2021 15:19:53 +0100 Subject: [PATCH 2/7] Simplifying logic since we can assume that any message within the buffer starts with '$' --- src/ArduinoNmeaParser.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/ArduinoNmeaParser.cpp b/src/ArduinoNmeaParser.cpp index 90c41a0..0a9ba01 100644 --- a/src/ArduinoNmeaParser.cpp +++ b/src/ArduinoNmeaParser.cpp @@ -105,9 +105,6 @@ void ArduinoNmeaParser::flushParserBuffer() bool ArduinoNmeaParser::isCompleteNmeaMessageInParserBuffer() { - if (_parser_buf.elems_in_buf < 8) /* $GPxxx\r\n = 8 */ - return false; - char const prev_last = _parser_buf.buf[_parser_buf.elems_in_buf - 2]; char const last = _parser_buf.buf[_parser_buf.elems_in_buf - 1]; From 4524dada563100f6dbe19357b96e911240564854 Mon Sep 17 00:00:00 2001 From: Alexander Entinger Date: Wed, 20 Jan 2021 15:21:54 +0100 Subject: [PATCH 3/7] Fixing spelling mistake of 'everytime' -> 'every time' --- src/ArduinoNmeaParser.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ArduinoNmeaParser.cpp b/src/ArduinoNmeaParser.cpp index 0a9ba01..de88f89 100644 --- a/src/ArduinoNmeaParser.cpp +++ b/src/ArduinoNmeaParser.cpp @@ -41,7 +41,7 @@ ArduinoNmeaParser::ArduinoNmeaParser(OnRmcUpdateFunc on_rmc_update, void ArduinoNmeaParser::encode(char const c) { - /* Flash the whole parser buffer everytime we encounter + /* Flash the whole parser buffer every time we encounter * a '$' sign. This way the parser buffer always starts * with a valid NMEA message. */ From cd962a03d6aa3febca416ab0a627f7148e1e7836 Mon Sep 17 00:00:00 2001 From: Alexander Entinger Date: Wed, 20 Jan 2021 15:52:39 +0100 Subject: [PATCH 4/7] Simplify checking for valgrind and also readability of code by dispensing struct ParserBuffer. --- src/ArduinoNmeaParser.cpp | 25 +++++++++++++------------ src/ArduinoNmeaParser.h | 9 ++------- 2 files changed, 15 insertions(+), 19 deletions(-) diff --git a/src/ArduinoNmeaParser.cpp b/src/ArduinoNmeaParser.cpp index de88f89..e25eceb 100644 --- a/src/ArduinoNmeaParser.cpp +++ b/src/ArduinoNmeaParser.cpp @@ -26,7 +26,8 @@ ArduinoNmeaParser::ArduinoNmeaParser(OnRmcUpdateFunc on_rmc_update, OnGgaUpdateFunc on_gga_update) : _error{Error::None} -, _parser_buf{{0}, 0} +, _parser_buf{0} +, _parser_buf_elems{0} , _rmc{nmea::INVALID_RMC} , _gga{nmea::INVALID_GGA} , _on_rmc_update{on_rmc_update} @@ -66,15 +67,15 @@ void ArduinoNmeaParser::encode(char const c) terminateParserBuffer(); /* Verify if the checksum of the NMEA message is correct. */ - if (!nmea::util::isChecksumOk(_parser_buf.buf)) { + if (!nmea::util::isChecksumOk(_parser_buf)) { _error = Error::Checksum; flushParserBuffer(); return; } /* Parse the various NMEA messages. */ - if (nmea::util::rmc_isGxRMC(_parser_buf.buf)) parseGxRMC(); - else if (nmea::util::rmc_isGxGGA(_parser_buf.buf)) parseGxGGA(); + if (nmea::util::rmc_isGxRMC(_parser_buf)) parseGxRMC(); + else if (nmea::util::rmc_isGxGGA(_parser_buf)) parseGxGGA(); /* The NMEA message has been fully processed and all * values updates so its time to flush the parser @@ -89,24 +90,24 @@ void ArduinoNmeaParser::encode(char const c) bool ArduinoNmeaParser::isParseBufferFull() { - return (_parser_buf.elems_in_buf >= (NMEA_PARSE_BUFFER_SIZE - 1)); + return (_parser_buf_elems >= (NMEA_PARSE_BUFFER_SIZE - 1)); } void ArduinoNmeaParser::addToParserBuffer(char const c) { - _parser_buf.buf[_parser_buf.elems_in_buf] = c; - _parser_buf.elems_in_buf++; + _parser_buf[_parser_buf_elems] = c; + _parser_buf_elems++; } void ArduinoNmeaParser::flushParserBuffer() { - _parser_buf.elems_in_buf = 0; + _parser_buf_elems = 0; } bool ArduinoNmeaParser::isCompleteNmeaMessageInParserBuffer() { - char const prev_last = _parser_buf.buf[_parser_buf.elems_in_buf - 2]; - char const last = _parser_buf.buf[_parser_buf.elems_in_buf - 1]; + char const prev_last = _parser_buf[_parser_buf_elems - 2]; + char const last = _parser_buf[_parser_buf_elems - 1]; return ((prev_last == '\r') && (last == '\n')); } @@ -118,7 +119,7 @@ void ArduinoNmeaParser::terminateParserBuffer() void ArduinoNmeaParser::parseGxRMC() { - nmea::GxRMC::parse(_parser_buf.buf, _rmc); + nmea::GxRMC::parse(_parser_buf, _rmc); if (_on_rmc_update) _on_rmc_update(_rmc); @@ -126,7 +127,7 @@ void ArduinoNmeaParser::parseGxRMC() void ArduinoNmeaParser::parseGxGGA() { - nmea::GxGGA::parse(_parser_buf.buf, _gga); + nmea::GxGGA::parse(_parser_buf, _gga); if (_on_gga_update) _on_gga_update(_gga); diff --git a/src/ArduinoNmeaParser.h b/src/ArduinoNmeaParser.h index 3f4cc79..f856362 100644 --- a/src/ArduinoNmeaParser.h +++ b/src/ArduinoNmeaParser.h @@ -58,14 +58,9 @@ class ArduinoNmeaParser static size_t constexpr NMEA_PARSE_BUFFER_SIZE = 82 + 1; /* Leave space for the '\0' terminator */ - typedef struct - { - char buf[NMEA_PARSE_BUFFER_SIZE]; - size_t elems_in_buf; - } ParserBuffer; - Error _error; - ParserBuffer _parser_buf; + char _parser_buf[NMEA_PARSE_BUFFER_SIZE]; + size_t _parser_buf_elems; nmea::RmcData _rmc; nmea::GgaData _gga; OnRmcUpdateFunc _on_rmc_update; From 4293c36cb91229a2b18308f48d778919e2fe6ddc Mon Sep 17 00:00:00 2001 From: Alexander Entinger Date: Wed, 20 Jan 2021 17:57:08 +0100 Subject: [PATCH 5/7] Prevent array-out-of-bounds access. --- src/ArduinoNmeaParser.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/ArduinoNmeaParser.cpp b/src/ArduinoNmeaParser.cpp index e25eceb..b9b1cd0 100644 --- a/src/ArduinoNmeaParser.cpp +++ b/src/ArduinoNmeaParser.cpp @@ -106,6 +106,9 @@ void ArduinoNmeaParser::flushParserBuffer() bool ArduinoNmeaParser::isCompleteNmeaMessageInParserBuffer() { + if (_parser_buf_elems < 2) + return false; + char const prev_last = _parser_buf[_parser_buf_elems - 2]; char const last = _parser_buf[_parser_buf_elems - 1]; From edafe97d7ac33203c4d81386f0d9a7b6249766db Mon Sep 17 00:00:00 2001 From: Alexander Entinger Date: Thu, 21 Jan 2021 07:11:46 +0100 Subject: [PATCH 6/7] Adding test code to cause segmentation violation. --- extras/test/src/test_ArduinoNmeaParser.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/extras/test/src/test_ArduinoNmeaParser.cpp b/extras/test/src/test_ArduinoNmeaParser.cpp index 2a8fa27..9074a7c 100644 --- a/extras/test/src/test_ArduinoNmeaParser.cpp +++ b/extras/test/src/test_ArduinoNmeaParser.cpp @@ -238,3 +238,10 @@ TEST_CASE("Multiple NMEA messages received", "[Parser-06]") REQUIRE(parser.error() == ArduinoNmeaParser::Error::None); } + +TEST_CASE("NMEA message with no checksum received", "[Parser-07]") +{ + ArduinoNmeaParser parser(nullptr, nullptr); + std::string const GPRMC = "79\r\n"; /* This should not lead to a segmentation violation. */ + encode(parser, GPRMC); +} From 51732341958bca7e35b5e47df40249eabb33cf95 Mon Sep 17 00:00:00 2001 From: Alexander Entinger Date: Wed, 20 Jan 2021 15:06:44 +0100 Subject: [PATCH 7/7] Bugfix: The old implementation of 'isCompleteMessageInParserBuffer' has suffered from the fact that it did not check for the occurence of both $ and \r\n within the parser buffer. Consequently - when during a power down an incomplete message arrives garbage is fed to the decoder which leads to all sorts of trouble down the line. This commit is fixing this issue. --- src/ArduinoNmeaParser.cpp | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/src/ArduinoNmeaParser.cpp b/src/ArduinoNmeaParser.cpp index b9b1cd0..aaa5a81 100644 --- a/src/ArduinoNmeaParser.cpp +++ b/src/ArduinoNmeaParser.cpp @@ -106,13 +106,34 @@ void ArduinoNmeaParser::flushParserBuffer() bool ArduinoNmeaParser::isCompleteNmeaMessageInParserBuffer() { - if (_parser_buf_elems < 2) + /* Temporarily terminate string with a '\0' terminator in + * order to be able to use string library functions. + */ + _parser_buf[_parser_buf_elems] = '\0'; + + /* Determine whether or not there exists a '$' marking + * the start of a NMEA message. + */ + char const * nmea_start = strchr(_parser_buf, '$'); + if (!nmea_start) + return false; + + /* Now that we've got a '$' marking the start of a NMEA + * message it is time to check if there's also an end + * to it. + */ + char const * nmea_stop_cr = strchr(nmea_start, '\r'); + if (!nmea_stop_cr) return false; - char const prev_last = _parser_buf[_parser_buf_elems - 2]; - char const last = _parser_buf[_parser_buf_elems - 1]; + char const * nmea_stop_lf = strchr(nmea_start, '\n'); + if (!nmea_stop_lf) + return false; - return ((prev_last == '\r') && (last == '\n')); + /* Lastly: check if the LF is really directly following + * the CR within the NMEA message. + */ + return ((nmea_stop_cr + 1) == nmea_stop_lf); } void ArduinoNmeaParser::terminateParserBuffer()