Re: [Gpsbabel-code] [BUG/PATCH] Garmin packet length checking, filter behaviour
Brought to you by:
robertl
Menu
▾
▴
Re: [Gpsbabel-code] [BUG/PATCH] Garmin packet length checking, filter behaviour
|
From: Robert L. <rob...@gp...> - 2010-03-26 03:13:40
|
On Thu, Mar 4, 2010 at 5:23 AM, Martin Buck <
mb-...@gr...> wrote:
> Hi again,
>
> while implementing Garmin course upload support I noticed two more issues
> in
> gpsbabel. I can offer a patch for one of them:
> * When reading a packet from serial or USB, its payload length isn't
> checked, so gpsbabel will corrupt memory if more than 1024 bytes of
> payload data are received (e.g. if ETX is lost on serial or we receive a
> bougs payload legnth field on USB). I saw this once on USB: Depending on
> which USB port it is connected to, my Forerunner 305 sometimes seems to
> drop the first byte in a USB packet, causing all the following bytes to be
> shifted. This resulted in a huge or negative payload length field which
> caused a buffer overflow and core dump. Patch attached that tries to print
> a meaningful error message instead of dumping core. Needs review: Is
> returning 0 from GPS_Packet_Read() in case of error OK or should the
> return value be negative? I noticed a bit of inconsistency when checking
> its return value...
>
USB is supposed to be detected/corrected at lower levels than the stack.
In general, the error recovery in the jeeps code is not great. Your checks
help so I've applied them.
> * If a filter drops some track points, the track point count in trk_waypts
> doesn't seem to get adjusted. When uploading the resulting track to a
> Garmin device, you get bogus track points since garmin.c sends as many
> track points as specified in trk_waypts/track_waypt_count() even though
> there are fewer valid ones after filtering. Should the filters adjust
> trk_waypts or is it a bad idea to rely on the result of
> track_waypt_count()?
>
>
Probably track_del_wpt needs a trk_waypts-- with corresponding edit on the
route side. Can you confirm that fixes what you're seeing?
RJL
> Thanks,
> Martin
>
>
>
>
> diff -ur gpsbabel-cvs-20091119-packet-len-checking.orig/jeeps/gpsread.c
> gpsbabel-cvs-20091119-packet-len-checking/jeeps/gpsread.c
> --- gpsbabel-cvs-20091119-packet-len-checking.orig/jeeps/gpsread.c
> 2010-02-11 07:18:22.000000000 +0100
> +++ gpsbabel-cvs-20091119-packet-len-checking/jeeps/gpsread.c 2010-02-25
> 18:10:44.000000000 +0100
> @@ -172,6 +172,12 @@
> return (*packet)->n;
> }
>
> + if (p - (*packet)->data >= MAX_GPS_PACKET_SIZE)
> + {
> + GPS_Error("GPS_Serial_Packet_Read: Bad payload size/no ETX
> found");
> + gps_errno = FRAMING_ERROR;
> + return 0;
> + }
> *p++ = u;
> }
> }
> diff -ur gpsbabel-cvs-20091119-packet-len-checking.orig/jeeps/gpsusbread.c
> gpsbabel-cvs-20091119-packet-len-checking/jeeps/gpsusbread.c
> --- gpsbabel-cvs-20091119-packet-len-checking.orig/jeeps/gpsusbread.c
> 2010-02-11 07:09:39.000000000 +0100
> +++ gpsbabel-cvs-20091119-packet-len-checking/jeeps/gpsusbread.c
> 2010-02-25 18:04:36.000000000 +0100
> @@ -71,6 +71,12 @@
> */
> (*packet)->type = le_read16(&pkt.gusb_pkt.pkt_id);
> payload_size = le_read32(&pkt.gusb_pkt.datasz);
> + if (payload_size<0 || payload_size>MAX_GPS_PACKET_SIZE)
> + {
> + GPS_Error("GPS_Packet_Read_usb: Bad payload size %d",
> payload_size);
> + gps_errno = FRAMING_ERROR;
> + return 0;
> + }
> (*packet)->n = payload_size;
> memcpy((*packet)->data, &pkt.gusb_pkt.databuf, payload_size);
>
>
>
> ------------------------------------------------------------------------------
> Download Intel® Parallel Studio Eval
> Try the new software tools for yourself. Speed compiling, find bugs
> proactively, and fine-tune applications for parallel performance.
> See why Intel Parallel Studio got high marks during beta.
> http://p.sf.net/sfu/intel-sw-dev
> _______________________________________________
> Gpsbabel-code mailing list http://www.gpsbabel.org
> Gps...@li...
> https://lists.sourceforge.net/lists/listinfo/gpsbabel-code
>
|