[PATCH] Coverity patches (tmem/xenstat) v1.

[PATCH] Coverity patches (tmem/xenstat) v1.

[Konrad Rzeszutek Wilk]

Please see some of the patches for tmem that Coverity has flagged.

And also the xenstat which I had fixed in the past but did not fix
it completely.


 tools/libxc/xc_domain_save.c           |  6 ++--
 tools/libxc/xc_tmem.c                  | 51 ++++++++++++++++++++--------------
 tools/libxc/xenctrl.h                  |  2 +-
 tools/xenstat/libxenstat/src/xenstat.c |  2 +-
 xen/common/tmem.c                      | 23 ++++++++++-----
 5 files changed, 52 insertions(+), 32 deletions(-)

Konrad Rzeszutek Wilk (5):
      xc/tmem: Free temporary buffer used during migration.
      xc/tmem: Unchecked return value.
      tmem: Check copy_to_user_* return value.
      tmem: Pointless check of NULL against an array
      xenstat: Fix buffer over-run with new_domains being negative (again).

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

[Konrad Rzeszutek Wilk]

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>
---
 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 61e1549..0918615 100644
--- a/tools/libxc/xc_tmem.c
+++ b/tools/libxc/xc_tmem.c
@@ -225,6 +225,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;
@@ -259,7 +260,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;
 
@@ -273,13 +273,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;
 
@@ -289,7 +289,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-- )
             {
@@ -300,21 +300,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;
+                    }
                     break;
                 }
             }
@@ -325,9 +326,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 */
@@ -396,6 +401,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 )
@@ -433,7 +439,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;
 
@@ -455,7 +460,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-- )
         {
@@ -463,20 +468,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 )
@@ -484,9 +489,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 */
-- 
1.8.3.1

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

[Andrew Cooper]

On 25/11/13 17:00, 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 61e1549..0918615 100644
> --- a/tools/libxc/xc_tmem.c
> +++ b/tools/libxc/xc_tmem.c
> @@ -225,6 +225,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;
> @@ -259,7 +260,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;
>  
> @@ -273,13 +273,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;
>  
> @@ -289,7 +289,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-- )
>              {
> @@ -300,21 +300,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;
> +                    }
>                      break;
>                  }
>              }
> @@ -325,9 +326,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 */
> @@ -396,6 +401,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 )
> @@ -433,7 +439,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;
>  
> @@ -455,7 +460,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-- )
>          {
> @@ -463,20 +468,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 )
> @@ -484,9 +489,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 */

[PATCH 2/5] xc/tmem: Unchecked return value.

[Konrad Rzeszutek Wilk]

CID 1055045 complains that the return value needs checking.

The tmem op is a flush call and there is no expectation
right now of any return besides zero. But just in case
this changes lets blow up.

CC: Bob Liu <bob.liu@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxc/xc_domain_save.c | 6 ++++--
 tools/libxc/xc_tmem.c        | 4 ++--
 tools/libxc/xenctrl.h        | 2 +-
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c
index 42c4752..c3f0f19 100644
--- a/tools/libxc/xc_domain_save.c
+++ b/tools/libxc/xc_domain_save.c
@@ -2095,8 +2095,10 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
         goto copypages;
     }
 
-    if ( tmem_saved != 0 && live )
-        xc_tmem_save_done(xch, dom);
+    if ( tmem_saved != 0 && live ) {
+        if ( xc_tmem_save_done(xch, dom) )
+            DPRINTF("Warning - failed to flush tmem on originating server.");
+    }
 
     if ( live )
     {
diff --git a/tools/libxc/xc_tmem.c b/tools/libxc/xc_tmem.c
index 0918615..df50ef5 100644
--- a/tools/libxc/xc_tmem.c
+++ b/tools/libxc/xc_tmem.c
@@ -366,9 +366,9 @@ int xc_tmem_save_extra(xc_interface *xch, int dom, int io_fd, int field_marker)
 }
 
 /* only called for live migration */
-void xc_tmem_save_done(xc_interface *xch, int dom)
+int xc_tmem_save_done(xc_interface *xch, int dom)
 {
-    xc_tmem_control(xch,0,TMEMC_SAVE_END,dom,0,0,0,NULL);
+    return xc_tmem_control(xch,0,TMEMC_SAVE_END,dom,0,0,0,NULL);
 }
 
 /* restore routines */
diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
index 4ac6b8a..18d4f78 100644
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -1996,7 +1996,7 @@ int xc_tmem_control(xc_interface *xch,
 int xc_tmem_auth(xc_interface *xch, int cli_id, char *uuid_str, int arg1);
 int xc_tmem_save(xc_interface *xch, int dom, int live, int fd, int field_marker);
 int xc_tmem_save_extra(xc_interface *xch, int dom, int fd, int field_marker);
-void xc_tmem_save_done(xc_interface *xch, int dom);
+int xc_tmem_save_done(xc_interface *xch, int dom);
 int xc_tmem_restore(xc_interface *xch, int dom, int fd);
 int xc_tmem_restore_extra(xc_interface *xch, int dom, int fd);
 
-- 
1.8.3.1

Re: [PATCH 2/5] xc/tmem: Unchecked return value.

[Andrew Cooper]

On 25/11/13 17:00, Konrad Rzeszutek Wilk wrote:
> CID 1055045 complains that the return value needs checking.
>
> The tmem op is a flush call and there is no expectation
> right now of any return besides zero. But just in case
> this changes lets blow up.
>
> CC: Bob Liu <bob.liu@oracle.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  tools/libxc/xc_domain_save.c | 6 ++++--
>  tools/libxc/xc_tmem.c        | 4 ++--
>  tools/libxc/xenctrl.h        | 2 +-
>  3 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c
> index 42c4752..c3f0f19 100644
> --- a/tools/libxc/xc_domain_save.c
> +++ b/tools/libxc/xc_domain_save.c
> @@ -2095,8 +2095,10 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
>          goto copypages;
>      }
>  
> -    if ( tmem_saved != 0 && live )
> -        xc_tmem_save_done(xch, dom);
> +    if ( tmem_saved != 0 && live ) {
> +        if ( xc_tmem_save_done(xch, dom) )

These could be combined into a single if statement.

> +            DPRINTF("Warning - failed to flush tmem on originating server.");

Does this need to bail at this point then?

> +    }
>  
>      if ( live )
>      {
> diff --git a/tools/libxc/xc_tmem.c b/tools/libxc/xc_tmem.c
> index 0918615..df50ef5 100644
> --- a/tools/libxc/xc_tmem.c
> +++ b/tools/libxc/xc_tmem.c
> @@ -366,9 +366,9 @@ int xc_tmem_save_extra(xc_interface *xch, int dom, int io_fd, int field_marker)
>  }
>  
>  /* only called for live migration */
> -void xc_tmem_save_done(xc_interface *xch, int dom)
> +int xc_tmem_save_done(xc_interface *xch, int dom)
>  {
> -    xc_tmem_control(xch,0,TMEMC_SAVE_END,dom,0,0,0,NULL);
> +    return xc_tmem_control(xch,0,TMEMC_SAVE_END,dom,0,0,0,NULL);

As you are tweaking this, could you apply some coding style with spaces
following commas?

~Andrew

>  }
>  
>  /* restore routines */
> diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
> index 4ac6b8a..18d4f78 100644
> --- a/tools/libxc/xenctrl.h
> +++ b/tools/libxc/xenctrl.h
> @@ -1996,7 +1996,7 @@ int xc_tmem_control(xc_interface *xch,
>  int xc_tmem_auth(xc_interface *xch, int cli_id, char *uuid_str, int arg1);
>  int xc_tmem_save(xc_interface *xch, int dom, int live, int fd, int field_marker);
>  int xc_tmem_save_extra(xc_interface *xch, int dom, int fd, int field_marker);
> -void xc_tmem_save_done(xc_interface *xch, int dom);
> +int xc_tmem_save_done(xc_interface *xch, int dom);
>  int xc_tmem_restore(xc_interface *xch, int dom, int fd);
>  int xc_tmem_restore_extra(xc_interface *xch, int dom, int fd);
>  

Re: [PATCH 2/5] xc/tmem: Unchecked return value.

[Konrad Rzeszutek Wilk]

On Mon, Nov 25, 2013 at 05:11:31PM +0000, Andrew Cooper wrote:
> On 25/11/13 17:00, Konrad Rzeszutek Wilk wrote:
> > CID 1055045 complains that the return value needs checking.
> >
> > The tmem op is a flush call and there is no expectation
> > right now of any return besides zero. But just in case
> > this changes lets blow up.
> >
> > CC: Bob Liu <bob.liu@oracle.com>
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  tools/libxc/xc_domain_save.c | 6 ++++--
> >  tools/libxc/xc_tmem.c        | 4 ++--
> >  tools/libxc/xenctrl.h        | 2 +-
> >  3 files changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c
> > index 42c4752..c3f0f19 100644
> > --- a/tools/libxc/xc_domain_save.c
> > +++ b/tools/libxc/xc_domain_save.c
> > @@ -2095,8 +2095,10 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
> >          goto copypages;
> >      }
> >  
> > -    if ( tmem_saved != 0 && live )
> > -        xc_tmem_save_done(xch, dom);
> > +    if ( tmem_saved != 0 && live ) {
> > +        if ( xc_tmem_save_done(xch, dom) )
> 
> These could be combined into a single if statement.
> 
> > +            DPRINTF("Warning - failed to flush tmem on originating server.");
> 
> Does this need to bail at this point then?

No. This failing means that the host hypervisor has some pages it couldn't free.
It should never happen (ha!) - but in case it does it is OK to forget about them
and just leave the 'zombie' guest behind.

> 
> > +    }
> >  
> >      if ( live )
> >      {
> > diff --git a/tools/libxc/xc_tmem.c b/tools/libxc/xc_tmem.c
> > index 0918615..df50ef5 100644
> > --- a/tools/libxc/xc_tmem.c
> > +++ b/tools/libxc/xc_tmem.c
> > @@ -366,9 +366,9 @@ int xc_tmem_save_extra(xc_interface *xch, int dom, int io_fd, int field_marker)
> >  }
> >  
> >  /* only called for live migration */
> > -void xc_tmem_save_done(xc_interface *xch, int dom)
> > +int xc_tmem_save_done(xc_interface *xch, int dom)
> >  {
> > -    xc_tmem_control(xch,0,TMEMC_SAVE_END,dom,0,0,0,NULL);
> > +    return xc_tmem_control(xch,0,TMEMC_SAVE_END,dom,0,0,0,NULL);
> 
> As you are tweaking this, could you apply some coding style with spaces
> following commas?

Of course. I thought it was frowend to mix coding style changes with
bug-fixes? Or was it more of an follow-up patch that would clean this
mess up?

> 
> ~Andrew
> 
> >  }
> >  
> >  /* restore routines */
> > diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
> > index 4ac6b8a..18d4f78 100644
> > --- a/tools/libxc/xenctrl.h
> > +++ b/tools/libxc/xenctrl.h
> > @@ -1996,7 +1996,7 @@ int xc_tmem_control(xc_interface *xch,
> >  int xc_tmem_auth(xc_interface *xch, int cli_id, char *uuid_str, int arg1);
> >  int xc_tmem_save(xc_interface *xch, int dom, int live, int fd, int field_marker);
> >  int xc_tmem_save_extra(xc_interface *xch, int dom, int fd, int field_marker);
> > -void xc_tmem_save_done(xc_interface *xch, int dom);
> > +int xc_tmem_save_done(xc_interface *xch, int dom);
> >  int xc_tmem_restore(xc_interface *xch, int dom, int fd);
> >  int xc_tmem_restore_extra(xc_interface *xch, int dom, int fd);
> >  
> 

[PATCH 3/5] tmem: Check copy_to_user_* return value.

[Konrad Rzeszutek Wilk]

We weren't checking whether that operation fails and
return the proper error.

This fixes CID 1055125, 105512, 1055127, 1055128, 1055129,
1055130.

CC: Bob Liu <bob.liu@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/common/tmem.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/xen/common/tmem.c b/xen/common/tmem.c
index 081772e..3bc35fd 100644
--- a/xen/common/tmem.c
+++ b/xen/common/tmem.c
@@ -2146,8 +2146,12 @@ static int tmemc_list(domid_t cli_id, tmem_cli_va_param_t buf, uint32_t len,
     if ( cli_id == TMEM_CLI_ID_NULL ) {
         off = tmemc_list_global(buf,0,len,use_long);
         off += tmemc_list_shared(buf,off,len-off,use_long);
-        list_for_each_entry(client,&global_client_list,client_list)
-            off += tmemc_list_client(client, buf, off, len-off, use_long);
+        list_for_each_entry(client,&global_client_list,client_list) {
+            int ret = tmemc_list_client(client, buf, off, len-off, use_long);
+            if ( ret < 0 )
+                return ret;
+            off += ret;
+        }
         off += tmemc_list_global_perf(buf,off,len-off,use_long);
     }
     else if ( (client = tmem_client_from_cli_id(cli_id)) == NULL)
@@ -2155,6 +2159,8 @@ static int tmemc_list(domid_t cli_id, tmem_cli_va_param_t buf, uint32_t len,
     else
         off = tmemc_list_client(client, buf, 0, len, use_long);
 
+    if ( off < 0 )
+        return off;
     return 0;
 }
 
@@ -2319,8 +2325,9 @@ static int tmemc_save_subop(int cli_id, uint32_t pool_id,
     case TMEMC_SAVE_GET_POOL_UUID:
          if ( pool == NULL )
              break;
-        tmem_copy_to_client_buf(buf, pool->uuid, 2);
         rc = 0;
+        if ( tmem_copy_to_client_buf(buf, pool->uuid, 2) )
+            rc = -EFAULT;
         break;
     case TMEMC_SAVE_END:
         if ( client == NULL )
@@ -2383,7 +2390,10 @@ static int tmemc_save_get_next_page(int cli_id, uint32_t pool_id,
     BUILD_BUG_ON(sizeof(h.oid) != sizeof(oid));
     memcpy(h.oid, oid.oid, sizeof(h.oid));
     h.index = pgp->index;
-    tmem_copy_to_client_buf(buf, &h, 1);
+    if ( tmem_copy_to_client_buf(buf, &h, 1) ) {
+        ret = -EFAULT;
+        goto out;
+    }
     tmem_client_buf_add(buf, sizeof(h));
     ret = do_tmem_get(pool, &oid, pgp->index, 0, 0, 0, pagesize, buf);
 
@@ -2427,8 +2437,9 @@ static int tmemc_save_get_next_inv(int cli_id, tmem_cli_va_param_t buf,
     BUILD_BUG_ON(sizeof(h.oid) != sizeof(pgp->inv_oid));
     memcpy(h.oid, pgp->inv_oid.oid, sizeof(h.oid));
     h.index = pgp->index;
-    tmem_copy_to_client_buf(buf, &h, 1);
     ret = 1;
+    if ( tmem_copy_to_client_buf(buf, &h, 1) )
+        ret = -EFAULT;
 out:
     tmem_spin_unlock(&pers_lists_spinlock);
     return ret;
-- 
1.8.3.1

Re: [PATCH 3/5] tmem: Check copy_to_user_* return value.

[Andrew Cooper]

On 25/11/13 17:00, Konrad Rzeszutek Wilk wrote:
> We weren't checking whether that operation fails and
> return the proper error.
>
> This fixes CID 1055125, 105512, 1055127, 1055128, 1055129,
> 1055130.
>
> CC: Bob Liu <bob.liu@oracle.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  xen/common/tmem.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/xen/common/tmem.c b/xen/common/tmem.c
> index 081772e..3bc35fd 100644
> --- a/xen/common/tmem.c
> +++ b/xen/common/tmem.c
> @@ -2146,8 +2146,12 @@ static int tmemc_list(domid_t cli_id, tmem_cli_va_param_t buf, uint32_t len,
>      if ( cli_id == TMEM_CLI_ID_NULL ) {
>          off = tmemc_list_global(buf,0,len,use_long);
>          off += tmemc_list_shared(buf,off,len-off,use_long);
> -        list_for_each_entry(client,&global_client_list,client_list)
> -            off += tmemc_list_client(client, buf, off, len-off, use_long);
> +        list_for_each_entry(client,&global_client_list,client_list) {

Spaces and commas.

> +            int ret = tmemc_list_client(client, buf, off, len-off, use_long);
> +            if ( ret < 0 )
> +                return ret;
> +            off += ret;
> +        }
>          off += tmemc_list_global_perf(buf,off,len-off,use_long);
>      }
>      else if ( (client = tmem_client_from_cli_id(cli_id)) == NULL)
> @@ -2155,6 +2159,8 @@ static int tmemc_list(domid_t cli_id, tmem_cli_va_param_t buf, uint32_t len,
>      else
>          off = tmemc_list_client(client, buf, 0, len, use_long);
>  
> +    if ( off < 0 )
> +        return off;

This looks to check for an overflow of 'off', but it is too late. 
Overflow needs to be checked each time you possibly add more to it.

~Andrew

>      return 0;
>  }
>  
> @@ -2319,8 +2325,9 @@ static int tmemc_save_subop(int cli_id, uint32_t pool_id,
>      case TMEMC_SAVE_GET_POOL_UUID:
>           if ( pool == NULL )
>               break;
> -        tmem_copy_to_client_buf(buf, pool->uuid, 2);
>          rc = 0;
> +        if ( tmem_copy_to_client_buf(buf, pool->uuid, 2) )
> +            rc = -EFAULT;
>          break;
>      case TMEMC_SAVE_END:
>          if ( client == NULL )
> @@ -2383,7 +2390,10 @@ static int tmemc_save_get_next_page(int cli_id, uint32_t pool_id,
>      BUILD_BUG_ON(sizeof(h.oid) != sizeof(oid));
>      memcpy(h.oid, oid.oid, sizeof(h.oid));
>      h.index = pgp->index;
> -    tmem_copy_to_client_buf(buf, &h, 1);
> +    if ( tmem_copy_to_client_buf(buf, &h, 1) ) {
> +        ret = -EFAULT;
> +        goto out;
> +    }
>      tmem_client_buf_add(buf, sizeof(h));
>      ret = do_tmem_get(pool, &oid, pgp->index, 0, 0, 0, pagesize, buf);
>  
> @@ -2427,8 +2437,9 @@ static int tmemc_save_get_next_inv(int cli_id, tmem_cli_va_param_t buf,
>      BUILD_BUG_ON(sizeof(h.oid) != sizeof(pgp->inv_oid));
>      memcpy(h.oid, pgp->inv_oid.oid, sizeof(h.oid));
>      h.index = pgp->index;
> -    tmem_copy_to_client_buf(buf, &h, 1);
>      ret = 1;
> +    if ( tmem_copy_to_client_buf(buf, &h, 1) )
> +        ret = -EFAULT;
>  out:
>      tmem_spin_unlock(&pers_lists_spinlock);
>      return ret;

Re: [PATCH 3/5] tmem: Check copy_to_user_* return value.

[Jan Beulich]

>>> On 25.11.13 at 18:00, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:

First of all, the title is wrong: You really mean copy_to_guest*().

> We weren't checking whether that operation fails and
> return the proper error.
> 
> This fixes CID 1055125, 105512, 1055127, 1055128, 1055129,
> 1055130.

But if you're doing something like this, you should fix all instances,
not just some. Which would e.g. require
tmem_copy_to_client_buf_offset() to propagate the return value
of copy_to_guest_offset() (it's odd anyway that this one is an
inline function, while tmem_copy_to_client_buf() is a macro).

But then again I'm wondering what baseline your patch uses:

> --- a/xen/common/tmem.c
> +++ b/xen/common/tmem.c
> @@ -2146,8 +2146,12 @@ static int tmemc_list(domid_t cli_id, tmem_cli_va_param_t buf, uint32_t len,
>      if ( cli_id == TMEM_CLI_ID_NULL ) {
>          off = tmemc_list_global(buf,0,len,use_long);
>          off += tmemc_list_shared(buf,off,len-off,use_long);

This isn't on line 2146 of today's staging tree, but on line 2239.

Hence looking at the individual changes may not make much sense,
as it's not clear whether there are other dependencies on earlier
changes here.

Jan

Re: [PATCH 3/5] tmem: Check copy_to_user_* return value.

[Konrad Rzeszutek Wilk]

On Tue, Nov 26, 2013 at 08:21:09AM +0000, Jan Beulich wrote:
> >>> On 25.11.13 at 18:00, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> 
> First of all, the title is wrong: You really mean copy_to_guest*().
> 
> > We weren't checking whether that operation fails and
> > return the proper error.
> > 
> > This fixes CID 1055125, 105512, 1055127, 1055128, 1055129,
> > 1055130.
> 
> But if you're doing something like this, you should fix all instances,
> not just some. Which would e.g. require
> tmem_copy_to_client_buf_offset() to propagate the return value
> of copy_to_guest_offset() (it's odd anyway that this one is an
> inline function, while tmem_copy_to_client_buf() is a macro).

No need. One of the Bobs patches fixes that. (the ones he posted
a couple of days ago that - not the ones you pulled).

> 
> But then again I'm wondering what baseline your patch uses:

It was on top of the earlier tmem patches. Which is staging, so this
should apply nicely on top of that. But I also realize that it has
the first of his patches that he had posted this week. That is the
    tmem: cleanup: drop some debug code

patch.

Sorry about that - I should have mentioned that in the cover letter and
I completely forgot.

> 
> > --- a/xen/common/tmem.c
> > +++ b/xen/common/tmem.c
> > @@ -2146,8 +2146,12 @@ static int tmemc_list(domid_t cli_id, tmem_cli_va_param_t buf, uint32_t len,
> >      if ( cli_id == TMEM_CLI_ID_NULL ) {
> >          off = tmemc_list_global(buf,0,len,use_long);
> >          off += tmemc_list_shared(buf,off,len-off,use_long);
> 
> This isn't on line 2146 of today's staging tree, but on line 2239.
> 
> Hence looking at the individual changes may not make much sense,
> as it's not clear whether there are other dependencies on earlier
> changes here.

<nods>
> 
> Jan
> 

[PATCH 4/5] tmem: Pointless check of NULL against an array

[Konrad Rzeszutek Wilk]

CID 1055632 tells us that we are comparing an array to a NULL
which is indeed silly. Lets not do it.

CC: Bob Liu <bob.liu@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/common/tmem.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/xen/common/tmem.c b/xen/common/tmem.c
index 3bc35fd..1ebed8a 100644
--- a/xen/common/tmem.c
+++ b/xen/common/tmem.c
@@ -1798,8 +1798,6 @@ static int do_tmem_destroy_pool(uint32_t pool_id)
     struct client *client = tmem_client_from_current();
     struct tmem_pool *pool;
 
-    if ( client->pools == NULL )
-        return 0;
     if ( pool_id >= MAX_POOLS_PER_DOMAIN )
         return 0;
     if ( (pool = client->pools[pool_id]) == NULL )
-- 
1.8.3.1

Re: [PATCH 4/5] tmem: Pointless check of NULL against an array

[Andrew Cooper]

On 25/11/13 17:00, Konrad Rzeszutek Wilk wrote:
> CID 1055632 tells us that we are comparing an array to a NULL
> which is indeed silly. Lets not do it.
>
> CC: Bob Liu <bob.liu@oracle.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  xen/common/tmem.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/xen/common/tmem.c b/xen/common/tmem.c
> index 3bc35fd..1ebed8a 100644
> --- a/xen/common/tmem.c
> +++ b/xen/common/tmem.c
> @@ -1798,8 +1798,6 @@ static int do_tmem_destroy_pool(uint32_t pool_id)
>      struct client *client = tmem_client_from_current();
>      struct tmem_pool *pool;
>  
> -    if ( client->pools == NULL )
> -        return 0;

client->pools certainly can't be null, but client certainly can.  This
should be "if ( !client )"

~Andrew

>      if ( pool_id >= MAX_POOLS_PER_DOMAIN )
>          return 0;
>      if ( (pool = client->pools[pool_id]) == NULL )

Re: [PATCH 4/5] tmem: Pointless check of NULL against an array

[Konrad Rzeszutek Wilk]

On Mon, Nov 25, 2013 at 05:19:32PM +0000, Andrew Cooper wrote:
> On 25/11/13 17:00, Konrad Rzeszutek Wilk wrote:
> > CID 1055632 tells us that we are comparing an array to a NULL
> > which is indeed silly. Lets not do it.
> >
> > CC: Bob Liu <bob.liu@oracle.com>
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  xen/common/tmem.c | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/xen/common/tmem.c b/xen/common/tmem.c
> > index 3bc35fd..1ebed8a 100644
> > --- a/xen/common/tmem.c
> > +++ b/xen/common/tmem.c
> > @@ -1798,8 +1798,6 @@ static int do_tmem_destroy_pool(uint32_t pool_id)
> >      struct client *client = tmem_client_from_current();
> >      struct tmem_pool *pool;
> >  
> > -    if ( client->pools == NULL )
> > -        return 0;
> 
> client->pools certainly can't be null, but client certainly can.  This
> should be "if ( !client )"

No need. The caller of this function does that already and if there
does not exist one - it will create one.

I can add an ASSERT here?
> 
> ~Andrew
> 
> >      if ( pool_id >= MAX_POOLS_PER_DOMAIN )
> >          return 0;
> >      if ( (pool = client->pools[pool_id]) == NULL )
> 

Re: [PATCH 4/5] tmem: Pointless check of NULL against an array

[Jan Beulich]

>>> On 25.11.13 at 20:54, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> On Mon, Nov 25, 2013 at 05:19:32PM +0000, Andrew Cooper wrote:
>> On 25/11/13 17:00, Konrad Rzeszutek Wilk wrote:
>> > CID 1055632 tells us that we are comparing an array to a NULL
>> > which is indeed silly. Lets not do it.
>> >
>> > CC: Bob Liu <bob.liu@oracle.com>
>> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> > ---
>> >  xen/common/tmem.c | 2 --
>> >  1 file changed, 2 deletions(-)
>> >
>> > diff --git a/xen/common/tmem.c b/xen/common/tmem.c
>> > index 3bc35fd..1ebed8a 100644
>> > --- a/xen/common/tmem.c
>> > +++ b/xen/common/tmem.c
>> > @@ -1798,8 +1798,6 @@ static int do_tmem_destroy_pool(uint32_t pool_id)
>> >      struct client *client = tmem_client_from_current();
>> >      struct tmem_pool *pool;
>> >  
>> > -    if ( client->pools == NULL )
>> > -        return 0;
>> 
>> client->pools certainly can't be null, but client certainly can.  This
>> should be "if ( !client )"
> 
> No need. The caller of this function does that already and if there
> does not exist one - it will create one.
> 
> I can add an ASSERT here?

Yes. Or just pass the already validated "client" into the function.

Jan

[PATCH 5/5] xenstat: Fix buffer over-run with new_domains being negative (again).

[Konrad Rzeszutek Wilk]

The git commit 1438d36f96e90d1116bebc6b3013634ca21c49c8
"xenstat: Fix buffer over-run with new_domains being negative."
fixed part of the problem, but it failed to do one more check.

That is the if we don't exit out of the loop if the
xc_domain_getinfolist returns -1. This makes the check
by casting the unsigned int value to int as otherwise
the
	if (new_domains < 0)

would never get executed (as unsigned int won't be negative).

Fixes CID 1055740.

CC: andrew.cooper3@citrix.com
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/xenstat/libxenstat/src/xenstat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/xenstat/libxenstat/src/xenstat.c b/tools/xenstat/libxenstat/src/xenstat.c
index e5facb8..27d8e22 100644
--- a/tools/xenstat/libxenstat/src/xenstat.c
+++ b/tools/xenstat/libxenstat/src/xenstat.c
@@ -208,7 +208,7 @@ xenstat_node *xenstat_get_node(xenstat_handle * handle, unsigned int flags)
 						    node->num_domains, 
 						    DOMAIN_CHUNK_SIZE, 
 						    domaininfo);
-		if (new_domains < 0)
+		if ((int)new_domains < 0)
 			goto err;
 
 		tmp = realloc(node->domains,
-- 
1.8.3.1

Re: [PATCH 5/5] xenstat: Fix buffer over-run with new_domains being negative (again).

[Ian Campbell]

On Mon, 2013-11-25 at 12:00 -0500, Konrad Rzeszutek Wilk wrote:
> The git commit 1438d36f96e90d1116bebc6b3013634ca21c49c8
> "xenstat: Fix buffer over-run with new_domains being negative."
> fixed part of the problem, but it failed to do one more check.
> 
> That is the if we don't exit out of the loop if the
> xc_domain_getinfolist returns -1. This makes the check
> by casting the unsigned int value to int as otherwise
> the
> 	if (new_domains < 0)
> 
> would never get executed (as unsigned int won't be negative).
> 
> Fixes CID 1055740.
> 
> CC: andrew.cooper3@citrix.com
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  tools/xenstat/libxenstat/src/xenstat.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/xenstat/libxenstat/src/xenstat.c b/tools/xenstat/libxenstat/src/xenstat.c
> index e5facb8..27d8e22 100644
> --- a/tools/xenstat/libxenstat/src/xenstat.c
> +++ b/tools/xenstat/libxenstat/src/xenstat.c
> @@ -208,7 +208,7 @@ xenstat_node *xenstat_get_node(xenstat_handle * handle, unsigned int flags)
>  						    node->num_domains, 
>  						    DOMAIN_CHUNK_SIZE, 
>  						    domaininfo);
> -		if (new_domains < 0)
> +		if ((int)new_domains < 0)

I expect that the right fix is to make new_domains the correct (signed)
type, no?

Ian.

Re: [PATCH 5/5] xenstat: Fix buffer over-run with new_domains being negative (again).

[Andrew Cooper]

On 25/11/13 17:06, Ian Campbell wrote:
> On Mon, 2013-11-25 at 12:00 -0500, Konrad Rzeszutek Wilk wrote:
>> The git commit 1438d36f96e90d1116bebc6b3013634ca21c49c8
>> "xenstat: Fix buffer over-run with new_domains being negative."
>> fixed part of the problem, but it failed to do one more check.
>>
>> That is the if we don't exit out of the loop if the
>> xc_domain_getinfolist returns -1. This makes the check
>> by casting the unsigned int value to int as otherwise
>> the
>> 	if (new_domains < 0)
>>
>> would never get executed (as unsigned int won't be negative).
>>
>> Fixes CID 1055740.
>>
>> CC: andrew.cooper3@citrix.com
>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> ---
>>  tools/xenstat/libxenstat/src/xenstat.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/xenstat/libxenstat/src/xenstat.c b/tools/xenstat/libxenstat/src/xenstat.c
>> index e5facb8..27d8e22 100644
>> --- a/tools/xenstat/libxenstat/src/xenstat.c
>> +++ b/tools/xenstat/libxenstat/src/xenstat.c
>> @@ -208,7 +208,7 @@ xenstat_node *xenstat_get_node(xenstat_handle * handle, unsigned int flags)
>>  						    node->num_domains, 
>>  						    DOMAIN_CHUNK_SIZE, 
>>  						    domaininfo);
>> -		if (new_domains < 0)
>> +		if ((int)new_domains < 0)
> I expect that the right fix is to make new_domains the correct (signed)
> type, no?
>
> Ian.
>

xc_domain_getinfolist() is just as crazy as its partner in crime,
xc_domain_getinfo(), and takes an unsigned int "max_doms" and returns a
signed number of domains gotten, or -1 for certain failed hypercalls.

I might be inclined to introduce a signed rc to hold the return value
xc_domain_getinfolist(), check for a negative return before assigning to
new_domains and also check for an overflow of node->num_domains +
new_domains for the realloc.

~Andrew

Re: [PATCH 5/5] xenstat: Fix buffer over-run with new_domains being negative (again).

[Konrad Rzeszutek Wilk]

On Mon, Nov 25, 2013 at 05:36:29PM +0000, Andrew Cooper wrote:
> On 25/11/13 17:06, Ian Campbell wrote:
> > On Mon, 2013-11-25 at 12:00 -0500, Konrad Rzeszutek Wilk wrote:
> >> The git commit 1438d36f96e90d1116bebc6b3013634ca21c49c8
> >> "xenstat: Fix buffer over-run with new_domains being negative."
> >> fixed part of the problem, but it failed to do one more check.
> >>
> >> That is the if we don't exit out of the loop if the
> >> xc_domain_getinfolist returns -1. This makes the check
> >> by casting the unsigned int value to int as otherwise
> >> the
> >> 	if (new_domains < 0)
> >>
> >> would never get executed (as unsigned int won't be negative).
> >>
> >> Fixes CID 1055740.
> >>
> >> CC: andrew.cooper3@citrix.com
> >> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >> ---
> >>  tools/xenstat/libxenstat/src/xenstat.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/tools/xenstat/libxenstat/src/xenstat.c b/tools/xenstat/libxenstat/src/xenstat.c
> >> index e5facb8..27d8e22 100644
> >> --- a/tools/xenstat/libxenstat/src/xenstat.c
> >> +++ b/tools/xenstat/libxenstat/src/xenstat.c
> >> @@ -208,7 +208,7 @@ xenstat_node *xenstat_get_node(xenstat_handle * handle, unsigned int flags)
> >>  						    node->num_domains, 
> >>  						    DOMAIN_CHUNK_SIZE, 
> >>  						    domaininfo);
> >> -		if (new_domains < 0)
> >> +		if ((int)new_domains < 0)
> > I expect that the right fix is to make new_domains the correct (signed)
> > type, no?

That can be done too.
> >
> > Ian.
> >
> 
> xc_domain_getinfolist() is just as crazy as its partner in crime,
> xc_domain_getinfo(), and takes an unsigned int "max_doms" and returns a
> signed number of domains gotten, or -1 for certain failed hypercalls.
> 
> I might be inclined to introduce a signed rc to hold the return value
> xc_domain_getinfolist(), check for a negative return before assigning to
> new_domains and also check for an overflow of node->num_domains +
> new_domains for the realloc.

Oh, hadn't thought about the overflow.

Re: [PATCH 5/5] xenstat: Fix buffer over-run with new_domains being negative (again).

[Ian Campbell]

On Mon, 2013-11-25 at 17:36 +0000, Andrew Cooper wrote:
> xc_domain_getinfolist() is just as crazy as its partner in crime,
> xc_domain_getinfo(), and takes an unsigned int "max_doms" and returns a
> signed number of domains gotten, or -1 for certain failed hypercalls.

Perhaps we should be changing those two to take a signed int max? The
domid_t typedef might be a suitable type.

For evtchns we have evtchn_port_or_error_t to cover the -1..2^16 space,
we could do something similar here for the return type, or just leave it
as is.