GPSBabel / gpsbabel

GPSBabel: convert, manipulate, and transfer data from GPS programs or GPS receivers. Open Source and supported on MacOS, Windows, Linux, and more. Pointy clicky GUI or a command line version...
https://www.gpsbabel.org
GNU General Public License v2.0
470 stars 125 forks source link

GPS serial memory leak? #1221

Closed tsteven4 closed 9 months ago

tsteven4 commented 9 months ago

GPS_Serial_On allocates memory for the device header on both windows https://github.com/GPSBabel/gpsbabel/blob/ad23c5767a149e71381503810677ba75df95c471/jeeps/gpsserial.cc#L94-L100 and posix https://github.com/GPSBabel/gpsbabel/blob/ad23c5767a149e71381503810677ba75df95c471/jeeps/gpsserial.cc#L573-L575

GPS_Serial_Off frees the memory on windows https://github.com/GPSBabel/gpsbabel/blob/ad23c5767a149e71381503810677ba75df95c471/jeeps/gpsserial.cc#L164-L169 but not posix. https://github.com/GPSBabel/gpsbabel/blob/ad23c5767a149e71381503810677ba75df95c471/jeeps/gpsserial.cc#L599-L608

robertlipe commented 9 months ago

Back at home where I can review on something bigger than a phone...

It's probable that the obvious one-line fix is the correct one.

It's possible that it'll corrupt memory in some weird way. The serial/USB paths have not been subject to the valgrind/memtest rigor that has tormented the rest of the code. In addition to being a pain to run via automation, the reality is that the runs are necessarily shorter (I think even now that the biggest garmin won't take more than 1,000 wpts through the USB protocol. Maybe it's 5,000.) and are less likely to be tied up in a long-running daemon process than the rest of the code. Code touches the device, moves data through, exits. A leak just isn't a big deal.

I think I brought on the devh stuff as part of the USB work in '04 or so. It was definitely retrofitted in with a big hammer, even then.

If you want to go ahead and just add the "obvious" xfree(dh); go for it. If there's a problem, assign this to me and I'll find and attach a serial and/or USB device and track down any exposed issue.

I'd respect you no less for just backing https://media2.giphy.com/media/COYGe9rZvfiaQ/giphy.gif?cid=ecf05e47afm9dzbthsmbzrymgmtckb4sovwcwiu0zuh55wil&ep=v1_gifs_search&rid=giphy.gif&ct=g away.

RJL

On Wed, Nov 15, 2023 at 6:07 PM tsteven4 @.***> wrote:

GPS_Serial_On allocates memory for the device header on both windows

https://github.com/GPSBabel/gpsbabel/blob/ad23c5767a149e71381503810677ba75df95c471/jeeps/gpsserial.cc#L94-L100 and posix

https://github.com/GPSBabel/gpsbabel/blob/ad23c5767a149e71381503810677ba75df95c471/jeeps/gpsserial.cc#L573-L575

GPS_Serial_Off frees the memory on windows

https://github.com/GPSBabel/gpsbabel/blob/ad23c5767a149e71381503810677ba75df95c471/jeeps/gpsserial.cc#L164-L169 but not posix.

https://github.com/GPSBabel/gpsbabel/blob/ad23c5767a149e71381503810677ba75df95c471/jeeps/gpsserial.cc#L599-L608

— Reply to this email directly, view it on GitHub https://github.com/GPSBabel/gpsbabel/issues/1221, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCSD35EXCKSWZPPHU4VJK3YEVKN7AVCNFSM6AAAAAA7NJOFCGVHI2DSMVQWIX3LMV43ASLTON2WKOZRHE4TKNZYHAZDAMI . You are receiving this because you are subscribed to this thread.Message ID: @.***>

tsteven4 commented 9 months ago

Thanks for your reply. The leak is confirmed. image