Parsing will cause NULL pointer exceptions when parsing malformed sentences.

[jpuderer]

https://github.com/adafruit/Adafruit_GPS/blob/4005211fd4db7090d24d7409910d3ab764d63d7d/src/NMEA_parse.cpp#L58

These strchr expressions are everywhere in the code, and will result in a null pointer exception if ever fed a malformed sentence that's missing a comma somewhere. If that's a limitation fine, but it makes the library pretty intolerant to line noise.

[jpuderer]

Correction, not quite a NULL pointer exception, but close enough. p will actually point to 0x00000001 which is also an invalid address.

[jpuderer]

It seems that this is somehow happening despite the passing the checksum. I'll have to dig into it. It's hard to catch, since it seems to happen only after running for a while. Adding print statements seems to make it occur much less frequently (a Heisenbug!).

[jpuderer]

As it turns out, I was getting a lot of invalid NMEA sentences that were getting silently discarded. However, an NMEA checksum is only a single byte, so eventually I win the lottery (1/256 chance) and try to parse an invalid NMEA sentence with a valid checksum.

This happens easily enough if you aren't calling the libraries read() function frequently enough, and miss some characters. This is a known issue.

You can't always call read() frequently enough either. If you are producing a lot of serial output for example, or other IO, it is easy to conceive of a situation in which you don't call read() before a character gets dropped. The correct solution to this of course, is to service the UART in a ISR, but that isn't the kind of thing that most people will know how to do (this is an Arduino library that attracts mostly learners and tinkerers...)

Considering all this, we should probably make the calls to strchr() a little more robust to detect when something is amiss. 1/256 is just too high a chance to accidentally pass the checksum with an invalid sentence that will then fatally crash the board.

[sellensr]

Last time I worked on the library, the main obstacle to improving the string validation was the limitation of being able to compile and run on the limited resources of an UNO. It might be time to split into two versions, something simple for the UNO and something more complex for boards with bigger processors.

Cheers,

Rick

On Jan 28, 2024, at 17:35, James Puderer @.***> wrote:

As it turns out, I was getting a lot of invalid NMEA sentences that were getting silently discarded. However, an NMEA checksum is only a single byte, so eventually I win the lottery (1/256 chance) and try to parse an invalid NMEA sentence with a valid checksum.

This happens easily enough if you aren't calling the libraries read() function frequently enough, and miss some characters. This is a known issue.

You can't always call read() frequently enough either. If you are producing a lot of serial output for example, or other IO, it is easy to conceive of a situation in which you don't call read() before a character gets dropped. The correct solution to this of course, is to service the UART in a ISR, but that isn't the kind of thing that most people will know how to do (this is an Arduino library that attracts mostly learners and tinkerers...)

Considering all this, we should probably make the calls to strchr() a little more robust to detect when something is amiss. 1/256 is just too high a chance to accidentally pass the checksum with an invalid sentence that will then fatally crash the board.

— Reply to this email directly, view it on GitHub https://github.com/adafruit/Adafruit_GPS/issues/154#issuecomment-1913744053, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAU44SMBAT2OYAHWQINBX4TYQ3HCJAVCNFSM6AAAAABCON63BWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJTG42DIMBVGM. You are receiving this because you are subscribed to this thread.

[svdrummer]

As the UNO is sooooo old and has been replaced with so many, much more powerful options, is it worth someone spending time writing a library for such an old device.

[sellensr]

No, but you could fork this one and produce newer versions that explicitly don’t support UNO, leaving the current version, or even one of the older, smaller versions as the UNO legacy support.

Cheers,

Rick

On Jan 28, 2024, at 23:54, svdrummer @.***> wrote:

As the UNO is sooooo old and has been replaced with so many, much more powerful options, is it worth someone spending time writing a library for such an old device.

— Reply to this email directly, view it on GitHub https://github.com/adafruit/Adafruit_GPS/issues/154#issuecomment-1913959261, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAU44SLIVWE4EQSLNTOIV33YQ4TOXAVCNFSM6AAAAABCON63BWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJTHE2TSMRWGE. You are receiving this because you commented.

[jpuderer]

Can someone look at my pull request? It works, is tested, and uses less memory.

[jpuderer]

Hello?