[Libguestfs] [PATCH] drives: fix deletion of servers on error

[Pino Toscano]

On Monday 18 August 2014 17:30:27 Richard W.M. Jones wrote:
> On Mon, Aug 18, 2014 at 02:56:08PM +0200, Pino Toscano wrote:
> > Make sure to not skip any of the created server, and to always free
> > the "server" array.
> > diff --git a/src/drives.c b/src/drives.c
> > index 4bd8328..85c1495 100644
> > --- a/src/drives.c
> > +++ b/src/drives.c
> > @@ -743,8 +743,7 @@ parse_servers (guestfs_h *g, char *const *strs,
> > 
> >    for (i = 0; i < n; ++i) {
> >    
> >      if (parse_one_server (g, strs[i], &servers[i]) == -1) {
> > 
> > -      if (i > 0)
> > -        free_drive_servers (servers, i-1);
> > +      free_drive_servers (servers, i);
> > 
> >        return -1;
> >      
> >      }
> >    
> >    }
> > 
> > ---
> > 
> >  src/drives.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/src/drives.c b/src/drives.c
> > index 4bd8328..85c1495 100644
> > --- a/src/drives.c
> > +++ b/src/drives.c
> > @@ -743,8 +743,7 @@ parse_servers (guestfs_h *g, char *const *strs,
> > 
> >    for (i = 0; i < n; ++i) {
> >    
> >      if (parse_one_server (g, strs[i], &servers[i]) == -1) {
> > 
> > -      if (i > 0)
> > -        free_drive_servers (servers, i-1);
> > +      free_drive_servers (servers, i);
> > 
> >        return -1;
> >      
> >      }
> >    
> >    }
> 
> The original code is attempting to avoid a double-free of
> servers[i].u.hostname.  I think this change would mean the double-free
> would happen again.

It shouldn't.

> I still don't understand the motivation for this change.

Basically in case of errors when parsing a server, we are leaking 
possibly one or two things, depending on the situation.

Taking the original code:

  for (i = 0; i < n; ++i) {
    if (parse_one_server (g, strs[i], &servers[i]) == -1) {
      if (i > 0)
        free_drive_servers (servers, i-1);
      return -1;
    }
  }

1) if the first element (i == 0) cannot be parsed, then the "servers" 
array is leaked; for example with:
  guestfish add-drive /disk server:"invalid_host"

2) if the faulty element is second and beyond, then there will be i 
elements before the faulty i-th, while the code frees i-1 elements 
(leaking the (i-1)-th one); for example with:
  guestfish add-drive /disk server:"tcp:example.com invalid_host"

-- 
Pino Toscano