[Libguestfs] [PATCH] daemon: readdir: fix invalid memory access on error

[Pino Toscano]

On Wednesday 21 January 2015 10:28:33 Hu Tao wrote:
> On Tue, Jan 20, 2015 at 04:28:39PM +0100, Pino Toscano wrote:
> > If "strdup (d->d_name)" fails with "i" > 0, then both "p" and
> > "ret->guestfs_int_dirent_list_val" are non-null pointers, but the latter
> > is no more valid (since "p" is the new realloc'ed buffer). Hence, trying
> > to free both will access to invalid memory.
> > 
> > Make sure to free only one of them, "p" if not null or
> > "ret->guestfs_int_dirent_list_val" otherwise.
> > ---
> >  daemon/readdir.c | 18 ++++++++++++++++--
> >  1 file changed, 16 insertions(+), 2 deletions(-)
> > 
> > diff --git a/daemon/readdir.c b/daemon/readdir.c
> > index f0ddd21..e488f93 100644
> > --- a/daemon/readdir.c
> > +++ b/daemon/readdir.c
> > @@ -27,6 +27,17 @@
> >  #include "daemon.h"
> >  #include "actions.h"
> >  
> > +static void
> > +free_int_dirent_list (guestfs_int_dirent *p, size_t len)
> > +{
> > +  size_t i;
> > +
> > +  for (i = 0; i < len; ++i) {
> > +    free (p[i].name);
> > +  }
> > +  free (p);
> > +}
> > +
> >  guestfs_int_dirent_list *
> >  do_readdir (const char *path)
> >  {
> > @@ -64,8 +75,11 @@ do_readdir (const char *path)
> >      v.name = strdup (d->d_name);
> >      if (!p || !v.name) {
> >        reply_with_perror ("allocate");
> > -      free (ret->guestfs_int_dirent_list_val);
> > -      free (p);
> > +      if (p) {
> > +        free_int_dirent_list (p, i);
> 
> I think free() is the correct way to free memory here, since the memory
> is allocated by realloc().

free_int_dirent_list in the end calls free on the whole array, but each
element has a char* member which need to be freed too.

-- 
Pino Toscano