<div dir="auto"><div><br><div class="gmail_extra"><br><div class="gmail_quote">On Aug 3, 2017 1:39 PM, "Dan Williams" <<a href="mailto:dcbw@redhat.com">dcbw@redhat.com</a>> wrote:<br type="attribution"><blockquote class="quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="elided-text">On Thu, 2017-08-03 at 13:25 -0700, Ben Chan wrote:<br>
> This patch fixes the following potential memory leaks in<br>
> MMLocationGpsNmea:<br>
><br>
> - When the `trace' string provided to location_gps_nmea_take_trace()<br>
> isn't<br>
>   added to the hash table, its ownership is still considered<br>
> transferred.<br>
>   It should thus be freed. Similarly, the `trace_type' string isn't<br>
>   added the hash table and should thus be freed.<br>
><br>
> - mm_location_gps_nmea_add_<wbr>trace() duplicates a given trace string<br>
> and<br>
>   then passes the trace copy to location_gps_nmea_take_trace()<wbr>. When<br>
>   location_gps_nmea_take_trace() returns FALSE, the ownership of the<br>
>   copy isn't transferred. mm_location_gps_nmea_add_<wbr>trace() should<br>
> thus<br>
>   free the copy.<br>
> ---<br>
>  libmm-glib/mm-location-gps-<wbr>nmea.c | 14 ++++++++++++--<br>
>  1 file changed, 12 insertions(+), 2 deletions(-)<br>
><br>
> diff --git a/libmm-glib/mm-location-gps-<wbr>nmea.c b/libmm-glib/mm-<br>
> location-gps-nmea.c<br>
> index 08ec97ff..577a5d90 100644<br>
> --- a/libmm-glib/mm-location-gps-<wbr>nmea.c<br>
> +++ b/libmm-glib/mm-location-gps-<wbr>nmea.c<br>
> @@ -96,8 +96,11 @@ location_gps_nmea_take_trace (MMLocationGpsNmea<br>
> *self,<br>
>              gchar *sequence;<br>
>  <br>
>              /* Skip the trace if we already have it there */<br>
> -            if (strstr (previous, trace))<br>
> +            if (strstr (previous, trace)) {<br>
> +                g_free (trace_type);<br>
> +                g_free (trace);<br>
>                  return TRUE;<br>
> +            }<br>
<br>
</div>This looks fine to me.<br>
<div class="quoted-text"> <br>
>              sequence = g_strdup_printf ("%s%s%s",<br>
>                               <wbr>           previous,<br>
> @@ -118,7 +121,14 @@ gboolean<br>
>  mm_location_gps_nmea_add_<wbr>trace (MMLocationGpsNmea *self,<br>
>                               <wbr>   const gchar *trace)<br>
>  {<br>
> -    return location_gps_nmea_take_trace (self, g_strdup (trace));<br>
> +    char *trace_copy;<br>
> +<br>
> +    trace_copy = g_strdup (trace);<br>
> +    if (location_gps_nmea_take_trace (self, trace_copy))<br>
> +        return TRUE;<br>
> +<br>
> +    g_free (trace_copy);<br>
> +    return FALSE;<br>
>  }<br>
<br>
</div>Since take_trace() is an internal function, and the current callers<br>
don't do anything useful with a FALSE return, maybe we should just<br>
update take_trace() to consume the input regardless of the return?<br>
Otherwise callers will have some strings consumed, and others not<br>
consumed, and the caller has to track that.  Seems kinda pointless.<br></blockquote></div></div></div><div dir="auto"><br></div><div dir="auto">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 ;)</div><div dir="auto"><br></div><div dir="auto">Aleksander, WDYT?</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<font color="#888888"><br>
Dan<br>
</font></blockquote></div><br></div></div></div>