[PATCH] mbimcli: use g_strfreev instead of g_free to free g_strsplit results

[Ben Chan]

On Mon, Feb 24, 2014 at 5:00 AM, Aleksander Morgado <
aleksander at aleksander.es> wrote:

> Hey Ben,
>
> On Mon, Feb 24, 2014 at 6:27 AM, Ben Chan <benchan at chromium.org> wrote:
> > ---
>
> That's wrong actually. g_strsplit () does create an array of strings
> in heap, but we're returning some of the contents of the array by
> reference. E.g. in the first chunk split[0] is returned as 'pin' and
> split[1] is returned as 'new_pin'. The problem here is that not every
> item in the split array may be returned, and that is why you may be
> seen memleaks. So, either we g_strdup() the returned strings and
> g_strfreev() the whole array afterwards, or we make sure the
> non-returned strings are g_free()-ed independently and leave the last
> g_free() for the array itself. See more comments below.
>
>
> >  src/mbimcli/mbimcli-basic-connect.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/mbimcli/mbimcli-basic-connect.c
> b/src/mbimcli/mbimcli-basic-connect.c
> > index 97368aa..9208d71 100644
> > --- a/src/mbimcli/mbimcli-basic-connect.c
> > +++ b/src/mbimcli/mbimcli-basic-connect.c
> > @@ -642,7 +642,7 @@ set_pin_input_parse (guint         n_expected,
> >      *pin = split[0];
> >      *new_pin = split[1] ? split[1] : NULL;
> >
> > -    g_free (split);
> > +    g_strfreev (split);
>
> There shouldn't be any leak here, both split[0] and split[1] get
> returned, and we end up freeing the split array (not its contents).
>
> >      return TRUE;
> >  }
> >
> > @@ -784,7 +784,7 @@ set_connect_activate_parse (const gchar       *str,
> >          }
> >      }
> >
> > -    g_free (split);
> > +    g_strfreev (split);
>
> In this case, I believe that only split[1] is being leaked. A simple
> g_free(split[1]) would be enough, right?
>
> >      return TRUE;
> >  }
> >
> > --
> > 1.9.0.rc1.175.g0b1dcb5
> >
>
>
Aleksander,

How about g_strdup the strings being returned, and then g_strfreev the
string array? That's less efficient but makes the ownership transfer of the
allocated strings more obvious. Also, we don't need to assume how
g_strsplit actually allocates the string array as the glib API only
suggests using g_strfreev.  How do you think?

Thanks,
Ben
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/libmbim-devel/attachments/20140224/b682f6f3/attachment-0001.html>