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

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;
     }
   }
-- 
1.9.3

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;
>      }
>    }
> -- 
> 1.9.3
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.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

Or maybe not; but I still don't understand the motivation for this
change.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

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