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
