On Aug 3, 2017 1:39 PM, "Dan Williams" <d...@redhat.com> wrote:
On Thu, 2017-08-03 at 13:25 -0700, Ben Chan wrote: > This patch fixes the following potential memory leaks in > MMLocationGpsNmea: > > - When the `trace' string provided to location_gps_nmea_take_trace() > isn't > added to the hash table, its ownership is still considered > transferred. > It should thus be freed. Similarly, the `trace_type' string isn't > added the hash table and should thus be freed. > > - mm_location_gps_nmea_add_trace() duplicates a given trace string > and > then passes the trace copy to location_gps_nmea_take_trace(). When > location_gps_nmea_take_trace() returns FALSE, the ownership of the > copy isn't transferred. mm_location_gps_nmea_add_trace() should > thus > free the copy. > --- > libmm-glib/mm-location-gps-nmea.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/libmm-glib/mm-location-gps-nmea.c b/libmm-glib/mm- > location-gps-nmea.c > index 08ec97ff..577a5d90 100644 > --- a/libmm-glib/mm-location-gps-nmea.c > +++ b/libmm-glib/mm-location-gps-nmea.c > @@ -96,8 +96,11 @@ location_gps_nmea_take_trace (MMLocationGpsNmea > *self, > gchar *sequence; > > /* Skip the trace if we already have it there */ > - if (strstr (previous, trace)) > + if (strstr (previous, trace)) { > + g_free (trace_type); > + g_free (trace); > return TRUE; > + } This looks fine to me. > sequence = g_strdup_printf ("%s%s%s", > previous, > @@ -118,7 +121,14 @@ gboolean > mm_location_gps_nmea_add_trace (MMLocationGpsNmea *self, > const gchar *trace) > { > - return location_gps_nmea_take_trace (self, g_strdup (trace)); > + char *trace_copy; > + > + trace_copy = g_strdup (trace); > + if (location_gps_nmea_take_trace (self, trace_copy)) > + return TRUE; > + > + g_free (trace_copy); > + return FALSE; > } Since take_trace() is an internal function, and the current callers don't do anything useful with a FALSE return, maybe we should just update take_trace() to consume the input regardless of the return? Otherwise callers will have some strings consumed, and others not consumed, and the caller has to track that. Seems kinda pointless. I was wondering the same thing, but not sure what's the typical semantic or convention for something called *_take_*(). I would prefer a simpler semantic, like you said, that it always take the ownership of the argument. But if there is an established semantic or convention, I would be hesitated to change it but that wouldn't prevent us from naming the function differently ;) Aleksander, WDYT? Dan
_______________________________________________ ModemManager-devel mailing list ModemManager-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel