[PATCH] location-gps-nmea: fix memory leaks

[Ben Chan]

On Aug 3, 2017 1:39 PM, "Dan Williams" <dcbw at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/modemmanager-devel/attachments/20170803/60ca8eab/attachment.html>