search

ArduPilot / ardupilot

ArduPlane, ArduCopter, ArduRover, ArduSub source
http://ardupilot.org/
GNU General Public License v3.0
11k stars 17.54k forks source link

NMEA Parser - potential lockup #961

Closed Crashpilot1000 closed 9 years ago

Crashpilot1000 commented 10 years ago

Scooping through the nmea parser I think this codeline

https://github.com/diydrones/ardupilot/blob/master/libraries/AP_GPS/AP_GPS_NMEA.cpp#L224

frac_min += (q++ - '0') \ frac_scale;

would be nicer/more stringent this way: frac_min += DIGIT_TO_VAL(q++) \ frac_scale;

Since DIGIT_TO_VAL to val is already used for DEG and Minutes. So just a suggestion for the readers eyes..

Crashpilot1000 commented 10 years ago

Forgot something. Maybe this line https://github.com/diydrones/ardupilot/blob/master/libraries/AP_GPS/AP_GPS_NMEA.cpp#L201 would be better off if the for loop had another termination (than isdigit alone) to prevent running out of bounds?

Crashpilot1000 commented 10 years ago

You know that wrong NMEA datapackages WILL FREEZE the flightcontrol then? May God be with you.

tridge commented 10 years ago

I'm reopening this as it should indeed be fixed. We don't guarantee that _term is terminated. I don't think it could actually cause a freeze, but it would be worthwhile putting in some sanity checks, or just guaranteeing a nul on end of of all parse buffers. I've assigned this issue to myself, but I may not get to it soon, sorry!

squilter commented 10 years ago

@Crashpilot1000 I'm cleaning through the issue tracker, closing non-issues. Hopefully you can understand why the title "NMEA Parser - no real issue" caught my eye. I'm going to go ahead and rename this issue.

Crashpilot1000 commented 10 years ago

Sorry for my misleading title. I choose it while looking at the gps drivers before I found that issue. And thanks for reconsidering it. Since multiwii uses a nearly 1:1 copy of your driver the problem popped up there and was fixed in its derivates: http://www.multiwii.com/forum/viewtopic.php?f=8&t=4930 https://github.com/multiwii/baseflight/blob/master/src/gps.c#L979 Since I messed up my repo I forgot to add it there so I did that today: https://github.com/Crashpilot1000/TestCode3/blob/b883b6110c9f1fe4c2024ce8268f8e750ed214d7/src/drv_gps.c#L485 So actually it was good for me to discuss that again. I hope this is helpful and somehow a little payback. Cheers Rob

tridge commented 9 years ago

i've pushed fixes for these - thanks very much for noticing this!