Re: [PATCH 1/4] xc/tmem: Free temporary buffer used during migration.

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 30/04/2014 21:29, Konrad Rzeszutek Wilk wrote:
> CID 1090388.
>
> Within the loop reading the pool_id we set the buf:
>
>  if ( (buf = realloc(buf,bufsize)) == NULL )
>
> and then continue on using it without freeing. Worst yet
> there are multiple 'if (..) return -1' which do not free
> the buffer.
>
> As such insert a 'fail' goto label to free the buffer
> and also add on the OK path a mechanism to free the buffer.
>
> Replace all of the 'return -1' with a jump to the failed
> label.
>
> CC: Bob Liu <bob.liu@oracle.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
>  tools/libxc/xc_tmem.c | 47 ++++++++++++++++++++++++++++-------------------
>  1 file changed, 28 insertions(+), 19 deletions(-)
>
> diff --git a/tools/libxc/xc_tmem.c b/tools/libxc/xc_tmem.c
> index 3261e10..12fa105 100644
> --- a/tools/libxc/xc_tmem.c
> +++ b/tools/libxc/xc_tmem.c
> @@ -215,6 +215,7 @@ int xc_tmem_save(xc_interface *xch,
>      uint32_t pool_id;
>      uint32_t minusone = -1;
>      struct tmem_handle *h;
> +    char *buf = NULL;
>  
>      if ( xc_tmem_control(xch,0,TMEMC_SAVE_BEGIN,dom,live,0,0,NULL) <= 0 )
>          return 0;
> @@ -249,7 +250,6 @@ int xc_tmem_save(xc_interface *xch,
>          uint64_t uuid[2];
>          uint32_t n_pages;
>          uint32_t pagesize;
> -        char *buf = NULL;
>          int bufsize = 0;
>          int checksum = 0;
>  
> @@ -263,13 +263,13 @@ int xc_tmem_save(xc_interface *xch,
>                  n_pages = 0;
>              (void)xc_tmem_control(xch,i,TMEMC_SAVE_GET_POOL_UUID,dom,sizeof(uuid),0,0,&uuid);
>              if ( write_exact(io_fd, &pool_id, sizeof(pool_id)) )
> -                return -1;
> +                goto failed;
>              if ( write_exact(io_fd, &flags, sizeof(flags)) )
> -                return -1;
> +                goto failed;
>              if ( write_exact(io_fd, &n_pages, sizeof(n_pages)) )
> -                return -1;
> +                goto failed;
>              if ( write_exact(io_fd, &uuid, sizeof(uuid)) )
> -                return -1;
> +                goto failed;
>              if ( n_pages == 0 )
>                  continue;
>  
> @@ -279,7 +279,7 @@ int xc_tmem_save(xc_interface *xch,
>              {
>                  bufsize = pagesize + sizeof(struct tmem_handle);
>                  if ( (buf = realloc(buf,bufsize)) == NULL )
> -                    return -1;
> +                    goto failed;
>              }
>              for ( j = n_pages; j > 0; j-- )
>              {
> @@ -290,21 +290,22 @@ int xc_tmem_save(xc_interface *xch,
>                  {
>                      h = (struct tmem_handle *)buf;
>                      if ( write_exact(io_fd, &h->oid, sizeof(h->oid)) )
> -                        return -1;
> +                        goto failed;
>                      if ( write_exact(io_fd, &h->index, sizeof(h->index)) )
> -                        return -1;
> +                        goto failed;
>                      h++;
>                      checksum += *(char *)h;
>                      if ( write_exact(io_fd, h, pagesize) )
> -                        return -1;
> +                        goto failed;
>                  } else if ( ret == 0 ) {
>                      continue;
>                  } else {
>                      /* page list terminator */
>                      h = (struct tmem_handle *)buf;
>                      h->oid[0] = h->oid[1] = h->oid[2] = -1L;
> -                    if ( write_exact(io_fd, &h->oid, sizeof(h->oid)) )
> -                        return -1;
> +                    if ( write_exact(io_fd, &h->oid, sizeof(h->oid)) ) {
> +                        goto failed;
> +                    }

Superfluous braces.

Otherwise, still fine.

~Andrew

>                      break;
>                  }
>              }
> @@ -315,9 +316,13 @@ int xc_tmem_save(xc_interface *xch,
>      /* pool list terminator */
>      minusone = -1;
>      if ( write_exact(io_fd, &minusone, sizeof(minusone)) )
> -        return -1;
> +        goto failed;
>  
> +    free(buf);
>      return 1;
> +failed:
> +    free(buf);
> +    return -1;
>  }
>  
>  /* only called for live migration */
> @@ -386,6 +391,7 @@ int xc_tmem_restore(xc_interface *xch, int dom, int io_fd)
>      uint32_t minusone;
>      uint32_t weight, cap, flags;
>      int checksum = 0;
> +    char *buf = NULL;
>  
>      save_version = xc_tmem_control(xch,0,TMEMC_SAVE_GET_VERSION,dom,0,0,0,NULL);
>      if ( save_version == -1 )
> @@ -423,7 +429,6 @@ int xc_tmem_restore(xc_interface *xch, int dom, int io_fd)
>      {
>          uint64_t uuid[2];
>          uint32_t n_pages;
> -        char *buf = NULL;
>          int bufsize = 0, pagesize;
>          int j;
>  
> @@ -445,7 +450,7 @@ int xc_tmem_restore(xc_interface *xch, int dom, int io_fd)
>          {
>              bufsize = pagesize;
>              if ( (buf = realloc(buf,bufsize)) == NULL )
> -                return -1;
> +                goto failed;
>          }
>          for ( j = n_pages; j > 0; j-- )
>          {
> @@ -453,20 +458,20 @@ int xc_tmem_restore(xc_interface *xch, int dom, int io_fd)
>              uint32_t index;
>              int rc;
>              if ( read_exact(io_fd, &oid, sizeof(oid)) )
> -                return -1;
> +                goto failed;
>              if ( oid.oid[0] == -1L && oid.oid[1] == -1L && oid.oid[2] == -1L )
>                  break;
>              if ( read_exact(io_fd, &index, sizeof(index)) )
> -                return -1;
> +                goto failed;
>              if ( read_exact(io_fd, buf, pagesize) )
> -                return -1;
> +                goto failed;
>              checksum += *buf;
>              if ( (rc = xc_tmem_control_oid(xch, pool_id,
>                                             TMEMC_RESTORE_PUT_PAGE, dom,
>                                             bufsize, index, oid, buf)) <= 0 )
>              {
>                  DPRINTF("xc_tmem_restore: putting page failed, rc=%d\n",rc);
> -                return -1;
> +                goto failed;
>              }
>          }
>          if ( n_pages )
> @@ -474,9 +479,13 @@ int xc_tmem_restore(xc_interface *xch, int dom, int io_fd)
>                      n_pages-j,dom,pool_id,checksum);
>      }
>      if ( pool_id != -1 )
> -        return -1;
> +        goto failed;
>  
> +    free(buf);
>      return 0;
> +failed:
> +    free(buf);
> +    return -1;
>  }
>  
>  /* only called for live migration, must be called after suspend */