Xen devel - Dec 2013 - [PATCH 00/13] Coverity fixes for libxl

Matthew Daley (13):
  libxl: fix unsigned less-than-0 comparison in e820_sanitize
  libxl: check for xc_domain_setmaxmem failure in libxl__build_pre
  libxl: correct file open success check in libxl__device_pci_reset
  libxl: don''t leak p in libxl__wait_for_backend
  libxl: remove unsigned less-than-0 comparison
  libxl: actually abort if initializing a ctx''s lock fails
  libxl: don''t leak output vcpu info on error in libxl_list_vcpu
  libxl: don''t leak ptr in libxl_list_vm error case
  libxl: don''t leak pcidevs in libxl_pcidev_assignable
  libxl: don''t try to fclose file twice on error in
    libxl_userdata_store
  libxl: use pipe instead of temporary file for VNC viewer --autopass
  libxl: don''t leak buf in libxl_xen_console_read_start error handling
  libxl: replace for loop with more idiomatic do-while loop

 tools/libxl/libxl.c        |   64 +++++++++++++++++++++-----------------------
 tools/libxl/libxl_cpuid.c  |    2 +-
 tools/libxl/libxl_device.c |    3 +++
 tools/libxl/libxl_dom.c    |   42 ++++++++++++++---------------
 tools/libxl/libxl_pci.c    |    9 +++----
 tools/libxl/libxl_x86.c    |    2 +-
 6 files changed, 59 insertions(+), 63 deletions(-)

-- 
1.7.10.4

Both src[i].size and delta are unsigned, so checking their difference
for being less than 0 doesn''t work.

Coverity-ID: 1055615
Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
---
 tools/libxl/libxl_x86.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
index e1c183f..b11d036 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -125,7 +125,7 @@ static int e820_sanitize(libxl_ctx *ctx, struct e820entry
src[],
             src[i].type = E820_UNUSABLE;
             delta = ram_end - src[i].addr;
             /* The end < ram_end should weed this out */
-            if (src[i].size - delta < 0)
+            if (src[i].size < delta)
                 src[i].type = 0;
             else {
                 src[i].size -= delta;
-- 
1.7.10.4

Coverity-ID: 1087115
Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
---
 tools/libxl/libxl_dom.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 72489f8..e9ac8f3 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -238,7 +238,12 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
     libxl_domain_set_nodeaffinity(ctx, domid, &info->nodemap);
     libxl_set_vcpuaffinity_all(ctx, domid, info->max_vcpus,
&info->cpumap);
 
-    xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb +
LIBXL_MAXMEM_CONSTANT);
+    rc = xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb +
LIBXL_MAXMEM_CONSTANT);
+    if (rc != 0) {
+        LOG(ERROR, "Couldn''t set max memory");
+        return rc;
+    }
+
     xs_domid = xs_read(ctx->xsh, XBT_NULL,
"/tool/xenstored/domid", NULL);
     state->store_domid = xs_domid ? atoi(xs_domid) : 0;
     free(xs_domid);
-- 
1.7.10.4

It could, even if only in theory, be fd 0.

(This is not the same as commit 4b62556b57!)

Coverity-ID: 1055895
Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
---
 tools/libxl/libxl_pci.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index c8eceb4..095d564 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -979,7 +979,7 @@ static int libxl__device_pci_reset(libxl__gc *gc, unsigned
int domain, unsigned
         LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Failed to access pciback
path %s", reset);
     reset = libxl__sprintf(gc, "%s/"PCI_BDF"/reset",
SYSFS_PCI_DEV, domain, bus, dev, func);
     fd = open(reset, O_WRONLY);
-    if (fd > 0) {
+    if (fd >= 0) {
         rc = write(fd, "1", 1);
         if (rc < 0)
             LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "write to %s returned
%d", reset, rc);
-- 
1.7.10.4

Coverity-ID: 1055891
Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
---
 tools/libxl/libxl_device.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index d995c83..072b5e3 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -1219,6 +1219,7 @@ int libxl__wait_for_backend(libxl__gc *gc, char *be_path,
char *state)
             goto out;
         } else {
             if (!strcmp(p, state)) {
+                free(p);
                 rc = 0;
                 goto out;
             } else {
@@ -1226,6 +1227,8 @@ int libxl__wait_for_backend(libxl__gc *gc, char *be_path,
char *state)
                 watchdog--;
             }
         }
+
+        free(p);
     }
     LOG(ERROR, "Backend %s not ready", be_path);
 out:
-- 
1.7.10.4

...from libxl_cpuid_parse_config_xend. value is unsigned so this
doesn''t
work, and either way the following comparison on it being bigger than 3
does what was intended here anyway.

Coverity-ID: 1055614
Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
---
 tools/libxl/libxl_cpuid.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_cpuid.c b/tools/libxl/libxl_cpuid.c
index e1c406c..dd21b78 100644
--- a/tools/libxl/libxl_cpuid.c
+++ b/tools/libxl/libxl_cpuid.c
@@ -298,7 +298,7 @@ int libxl_cpuid_parse_config_xend(libxl_cpuid_policy_list
*cpuid,
         }
         value = str[1] - ''a'';
         endptr = strchr(str, ''='');
-        if (value < 0 || value > 3 || endptr == NULL) {
+        if (value > 3 || endptr == NULL) {
             return 4;
         }
         str = endptr + 1;
-- 
1.7.10.4

If initializing the ctx''s lock fails, don''t keep going, but
instead
error out.

Coverity-ID: 1055289
Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
---
 tools/libxl/libxl.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 2b847ef..26eaee4 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -74,6 +74,8 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
         LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Failed to initialize
mutex");
         free(ctx);
         ctx = 0;
+        rc = ERROR_FAIL;
+        goto out;
     }
 
     /* Now ctx is safe for ctx_free; failures simply set rc and "goto
out" */
-- 
1.7.10.4

Coverity-ID: 1055887
Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
---
 tools/libxl/libxl.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 26eaee4..a57d571 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4544,15 +4544,15 @@ libxl_vcpuinfo *libxl_list_vcpu(libxl_ctx *ctx, uint32_t
domid,
     for (*nb_vcpu = 0; *nb_vcpu <= domaininfo.max_vcpu_id; ++*nb_vcpu,
++ptr) {
         if (libxl_cpu_bitmap_alloc(ctx, &ptr->cpumap, 0)) {
             LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "allocating
cpumap");
-            return NULL;
+            goto err;
         }
         if (xc_vcpu_getinfo(ctx->xch, domid, *nb_vcpu, &vcpuinfo) == -1)
{
             LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting vcpu
info");
-            return NULL;
+            goto err;
         }
         if (xc_vcpu_getaffinity(ctx->xch, domid, *nb_vcpu,
ptr->cpumap.map) == -1) {
             LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting vcpu
affinity");
-            return NULL;
+            goto err;
         }
         ptr->vcpuid = *nb_vcpu;
         ptr->cpu = vcpuinfo.cpu;
@@ -4562,6 +4562,10 @@ libxl_vcpuinfo *libxl_list_vcpu(libxl_ctx *ctx, uint32_t
domid,
         ptr->vcpu_time = vcpuinfo.cpu_time;
     }
     return ret;
+
+err:
+    free(ret);
+    return NULL;
 }
 
 int libxl_set_vcpuaffinity(libxl_ctx *ctx, uint32_t domid, uint32_t vcpuid,
-- 
1.7.10.4

While at it, tidy up the function; there''s no point in allocating more
than the amount of domains actually returned by xc_domain_getinfolist.

Coverity-ID: 1055888
Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
---
 tools/libxl/libxl.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index a57d571..ca4c2cd 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -674,17 +674,17 @@ libxl_vminfo * libxl_list_vm(libxl_ctx *ctx, int
*nb_vm_out)
     libxl_vminfo *ptr;
     int idx, i, ret;
     xc_domaininfo_t info[1024];
-    int size = 1024;
 
-    ptr = calloc(size, sizeof(libxl_vminfo));
-    if (!ptr)
+    ret = xc_domain_getinfolist(ctx->xch, 1, ARRAY_SIZE(info), info);
+    if (ret < 0) {
+        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting domain info
list");
         return NULL;
+    }
 
-    ret = xc_domain_getinfolist(ctx->xch, 1, 1024, info);
-    if (ret<0) {
-        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "geting domain info
list");
+    ptr = calloc(ret, sizeof(libxl_vminfo));
+    if (!ptr)
         return NULL;
-    }
+
     for (idx = i = 0; i < ret; i++) {
         if (libxl_is_stubdom(ctx, info[i].domain, NULL))
             continue;
-- 
1.7.10.4

Coverity-ID: 1055896
Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
---
 tools/libxl/libxl_pci.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 095d564..2e52470 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -1021,11 +1021,10 @@ static int libxl_pcidev_assignable(libxl_ctx *ctx,
libxl_device_pci *pcidev)
             pcidevs[i].bus == pcidev->bus &&
             pcidevs[i].dev == pcidev->dev &&
             pcidevs[i].func == pcidev->func)
-        {
-            return 1;
-        }
+            break;
     }
-    return 0;
+    free(pcidevs);
+    return i != num;
 }
 
 int libxl__device_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci
*pcidev, int starting)
-- 
1.7.10.4

Do this by changing the function to not use stdio file operations, but
just use write(2) directly.

While at it, tidy up the function''s style issues.

Coverity-ID: 1056195
Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
---
 tools/libxl/libxl_dom.c |   35 ++++++++++++++---------------------
 1 file changed, 14 insertions(+), 21 deletions(-)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index e9ac8f3..0fe0a77 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1604,8 +1604,6 @@ int libxl_userdata_store(libxl_ctx *ctx, uint32_t domid,
     const char *newfilename;
     int e, rc;
     int fd = -1;
-    FILE *f = NULL;
-    size_t rs;
 
     filename = userdata_path(gc, domid, userdata_userid, "d");
     if (!filename) {
@@ -1626,38 +1624,33 @@ int libxl_userdata_store(libxl_ctx *ctx, uint32_t domid,
 
     rc = ERROR_FAIL;
 
-    fd= open(newfilename, O_RDWR|O_CREAT|O_TRUNC, 0600);
-    if (fd<0)
+    fd = open(newfilename, O_RDWR | O_CREAT | O_TRUNC, 0600);
+    if (fd < 0)
         goto err;
 
-    f= fdopen(fd, "wb");
-    if (!f)
+    if (write(fd, data, datalen) != datalen)
         goto err;
-    fd = -1;
 
-    rs = fwrite(data, 1, datalen, f);
-    if (rs != datalen) {
-        assert(ferror(f));
+    if (close(fd) < 0) {
+        fd = -1;
         goto err;
     }
+    fd = -1;
 
-    if (fclose(f))
-        goto err;
-    f = 0;
-
-    if (rename(newfilename,filename))
+    if (rename(newfilename, filename))
         goto err;
 
     rc = 0;
 
 err:
-    e = errno;
-    if (f) fclose(f);
-    if (fd>=0) close(fd);
+    if (fd >= 0) {
+        e = errno;
+        close(fd);
+        errno = e;
+    }
 
-    errno = e;
-    if ( rc )
-        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "cannot write %s for
%s",
+    if (rc)
+        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "cannot write/rename %s
for %s",
                  newfilename, filename);
 out:
     GC_FREE;
-- 
1.7.10.4

Coverity was complaining about the permissions implicitly set on the
temporary file used to pass the VNC password to the viewer when using
the --autopass feature. By replacing the use of the temporary file
with a pipe, we fix the problem (well, quiesce Coverity at least), tidy
the code and remove the buildup of temporary file cruft all at once.

Tested with TightVNC.

Coverity-ID: 1055958
Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
---
 tools/libxl/libxl.c |   30 +++++++++++-------------------
 1 file changed, 11 insertions(+), 19 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index ca4c2cd..41b8f60 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1623,7 +1623,7 @@ int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid,
int autopass)
     GC_INIT(ctx);
     const char *vnc_port;
     const char *vnc_listen = NULL, *vnc_pass = NULL;
-    int port = 0, autopass_fd = -1;
+    int port = 0, autopass_fds[2] = {-1, -1};
     char *vnc_bin, *args[] = {
         "vncviewer",
         NULL, /* hostname:display */
@@ -1655,38 +1655,30 @@ int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid,
int autopass)
     args[1] = libxl__sprintf(gc, "%s:%d", vnc_listen, port);
 
     if ( vnc_pass ) {
-        char tmpname[] = "/tmp/vncautopass.XXXXXX";
-        autopass_fd = mkstemp(tmpname);
-        if ( autopass_fd < 0 ) {
-            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
-                             "mkstemp %s failed", tmpname);
-            goto x_fail;
-        }
-
-        if ( unlink(tmpname) ) {
-            /* should never happen */
-            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
-                             "unlink %s failed", tmpname);
+        if ( pipe(autopass_fds) < 0 ) {
+            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "pipe failed");
             goto x_fail;
         }
 
-        if ( libxl_write_exactly(ctx, autopass_fd, vnc_pass, strlen(vnc_pass),
-                                    tmpname, "vnc password") )
+        if ( libxl_write_exactly(ctx, autopass_fds[1], vnc_pass,
strlen(vnc_pass),
+                                    "(pipe)", "vnc
password") )
             goto x_fail;
 
-        if ( lseek(autopass_fd, SEEK_SET, 0) ) {
-            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
-                             "rewind %s (autopass) failed", tmpname);
+        if ( close(autopass_fds[1]) < 0 ) {
+            autopass_fds[1] = -1;
             goto x_fail;
         }
+        autopass_fds[1] = -1;
 
         args[2] = "-autopass";
     }
 
-    libxl__exec(gc, autopass_fd, -1, -1, args[0], args, NULL);
+    libxl__exec(gc, autopass_fds[0], -1, -1, args[0], args, NULL);
     abort();
 
  x_fail:
+    if ( autopass_fds[0] >= 0 ) close(autopass_fds[0]);
+    if ( autopass_fds[1] >= 0 ) close(autopass_fds[1]);
     GC_FREE;
     return ERROR_FAIL;
 }
-- 
1.7.10.4

Coverity-ID: 1055889
Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
---
 tools/libxl/libxl.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 41b8f60..4cd54a8 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5116,6 +5116,7 @@ libxl_xen_console_reader *
     cr = malloc(sizeof(libxl_xen_console_reader));
     if (!cr) {
         LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "cannot malloc
libxl_xen_console_reader");
+        free(buf);
         return NULL;
     }
 
-- 
1.7.10.4

Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
---
 tools/libxl/libxl.c |    7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 4cd54a8..293e14a 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5413,14 +5413,11 @@ int libxl_cpupool_destroy(libxl_ctx *ctx, uint32_t
poolid)
         goto out1;
     }
 
-    for (;;) {
+    do {
         t = xs_transaction_start(ctx->xsh);
 
         xs_rm(ctx->xsh, XBT_NULL, libxl__sprintf(gc,
"/local/pool/%d", poolid));
-
-        if (xs_transaction_end(ctx->xsh, t, 0) || (errno != EAGAIN))
-            break;
-    }
+    } while (!xs_transaction_end(ctx->xsh, t, 0) && errno ==
EAGAIN);
 
     rc = 0;
 
-- 
1.7.10.4

On 01/12/2013 10:14, Matthew Daley wrote:
> Coverity-ID: 1055891
> Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
As with my " tools/libxl: Fix libxl__device_nic_from_xs_be()" patch, I
feel the suggestion will be to use libxl__xs_read_checked() instead,
given all the libxl GC machinery already around.

~Andrew
> ---
>  tools/libxl/libxl_device.c |    3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index d995c83..072b5e3 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -1219,6 +1219,7 @@ int libxl__wait_for_backend(libxl__gc *gc, char
*be_path, char *state)
>              goto out;
>          } else {
>              if (!strcmp(p, state)) {
> +                free(p);
>                  rc = 0;
>                  goto out;
>              } else {
> @@ -1226,6 +1227,8 @@ int libxl__wait_for_backend(libxl__gc *gc, char
*be_path, char *state)
>                  watchdog--;
>              }
>          }
> +
> +        free(p);
>      }
>      LOG(ERROR, "Backend %s not ready", be_path);
>  out:

On 01/12/2013 10:15, Matthew Daley wrote:
> While at it, tidy up the function; there''s no point in allocating
more
> than the amount of domains actually returned by xc_domain_getinfolist.
>
> Coverity-ID: 1055888
> Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
> ---
>  tools/libxl/libxl.c |   14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index a57d571..ca4c2cd 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -674,17 +674,17 @@ libxl_vminfo * libxl_list_vm(libxl_ctx *ctx, int
*nb_vm_out)
>      libxl_vminfo *ptr;
>      int idx, i, ret;
>      xc_domaininfo_t info[1024];
> -    int size = 1024;
>  
> -    ptr = calloc(size, sizeof(libxl_vminfo));
> -    if (!ptr)
> +    ret = xc_domain_getinfolist(ctx->xch, 1, ARRAY_SIZE(info), info);
> +    if (ret < 0) {
> +        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting domain info
list");
>          return NULL;
> +    }
>  
> -    ret = xc_domain_getinfolist(ctx->xch, 1, 1024, info);
> -    if (ret<0) {
> -        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "geting domain info
list");
> +    ptr = calloc(ret, sizeof(libxl_vminfo));
We now have a possible case of calling calloc(0, sizeof(libxl_vminfo));

The implementation is free to return NULL which will cause this function
to fail in the eyes of its callers.

Doing a calloc(min(1,ret), sizeof(libxl_vminfo)); will suffice, as the
callers already have to correctly deal with 0 domains but some allocated
memory as a result of this function.

~Andrew
> +    if (!ptr)
>          return NULL;
> -    }
> +
>      for (idx = i = 0; i < ret; i++) {
>          if (libxl_is_stubdom(ctx, info[i].domain, NULL))
>              continue;

On 01/12/2013 10:14, Matthew Daley wrote:

All except for patches 4 and 8 (which have followups),

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Matthew Daley (13):
>   libxl: fix unsigned less-than-0 comparison in e820_sanitize
>   libxl: check for xc_domain_setmaxmem failure in libxl__build_pre
>   libxl: correct file open success check in libxl__device_pci_reset
>   libxl: don''t leak p in libxl__wait_for_backend
>   libxl: remove unsigned less-than-0 comparison
>   libxl: actually abort if initializing a ctx''s lock fails
>   libxl: don''t leak output vcpu info on error in libxl_list_vcpu
>   libxl: don''t leak ptr in libxl_list_vm error case
>   libxl: don''t leak pcidevs in libxl_pcidev_assignable
>   libxl: don''t try to fclose file twice on error in
>     libxl_userdata_store
>   libxl: use pipe instead of temporary file for VNC viewer --autopass
>   libxl: don''t leak buf in libxl_xen_console_read_start error
handling
>   libxl: replace for loop with more idiomatic do-while loop
>
>  tools/libxl/libxl.c        |   64
+++++++++++++++++++++-----------------------
>  tools/libxl/libxl_cpuid.c  |    2 +-
>  tools/libxl/libxl_device.c |    3 +++
>  tools/libxl/libxl_dom.c    |   42 ++++++++++++++---------------
>  tools/libxl/libxl_pci.c    |    9 +++----
>  tools/libxl/libxl_x86.c    |    2 +-
>  6 files changed, 59 insertions(+), 63 deletions(-)
>

On Mon, Dec 2, 2013 at 12:53 AM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 01/12/2013 10:14, Matthew Daley wrote:
>> Coverity-ID: 1055891
>> Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
>
> As with my " tools/libxl: Fix libxl__device_nic_from_xs_be()"
patch, I
> feel the suggestion will be to use libxl__xs_read_checked() instead,
> given all the libxl GC machinery already around.
Good point, I didn''t notice it was allocated by xs_read in the first
place. I was holding back on such cases with the intent to produce one
big patch to convert all xs_reads in libxl to their GC''d counterparts,
but since I still haven''t done this, and since it makes sense to
individually patch the ones that are leaking their results right now,
I''ll make a v2.

- Matthew

Use libxl__xs_read_checked instead of xs_read. While at it, tidy up the
function as well.

Coverity-ID: 1055891
Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
---
v2: Use libxl__xs_read_checked instead of xs_read. Tidy up the function.

 tools/libxl/libxl_device.c   |   41 +++++++++++++++++------------------------
 tools/libxl/libxl_internal.h |    3 ++-
 2 files changed, 19 insertions(+), 25 deletions(-)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index d995c83..ba7d100 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -1199,37 +1199,30 @@ int libxl__wait_for_device_model_deprecated(libxl__gc
*gc,
                                      check_callback, check_callback_userdata);
 }
 
-int libxl__wait_for_backend(libxl__gc *gc, char *be_path, char *state)
+int libxl__wait_for_backend(libxl__gc *gc, const char *be_path,
+                            const char *state)
 {
-    libxl_ctx *ctx = libxl__gc_owner(gc);
     int watchdog = 100;
-    unsigned int len;
-    char *p;
-    char *path = GCSPRINTF("%s/state", be_path);
-    int rc = -1;
+    const char *p, *path = GCSPRINTF("%s/state", be_path);
+    int rc;
+
+    while (watchdog-- > 0) {
+        rc = libxl__xs_read_checked(gc, XBT_NULL, path, &p);
+        if (rc) return rc;
 
-    while (watchdog > 0) {
-        p = xs_read(ctx->xsh, XBT_NULL, path, &len);
         if (p == NULL) {
-            if (errno == ENOENT) {
-                LOG(ERROR, "Backend %s does not exist", be_path);
-            } else {
-                LOGE(ERROR, "Failed to access backend %s", be_path);
-            }
-            goto out;
-        } else {
-            if (!strcmp(p, state)) {
-                rc = 0;
-                goto out;
-            } else {
-                usleep(100000);
-                watchdog--;
-            }
+            LOG(ERROR, "Backend %s does not exist", be_path);
+            return ERROR_FAIL;
         }
+
+        if (!strcmp(p, state))
+            return 0;
+
+        usleep(100000);
     }
+
     LOG(ERROR, "Backend %s not ready", be_path);
-out:
-    return rc;
+    return ERROR_FAIL;
 }
 
 /*
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index a2d8247..1bd23ff 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -944,7 +944,8 @@ _hidden char *libxl__device_frontend_path(libxl__gc *gc,
libxl__device *device);
 _hidden int libxl__parse_backend_path(libxl__gc *gc, const char *path,
                                       libxl__device *dev);
 _hidden int libxl__device_destroy(libxl__gc *gc, libxl__device *dev);
-_hidden int libxl__wait_for_backend(libxl__gc *gc, char *be_path, char *state);
+_hidden int libxl__wait_for_backend(libxl__gc *gc, const char *be_path,
+                                    const char *state);
 _hidden int libxl__nic_type(libxl__gc *gc, libxl__device *dev,
                             libxl_nic_type *nictype);
 
-- 
1.7.10.4

On Mon, Dec 2, 2013 at 1:20 AM, Andrew Cooper <andrew.cooper3@citrix.com>
wrote:
> On 01/12/2013 10:15, Matthew Daley wrote:
>> While at it, tidy up the function; there''s no point in
allocating more
>> than the amount of domains actually returned by xc_domain_getinfolist.
>>
>> Coverity-ID: 1055888
>> Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
>> ---
>>  tools/libxl/libxl.c |   14 +++++++-------
>>  1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index a57d571..ca4c2cd 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
>> @@ -674,17 +674,17 @@ libxl_vminfo * libxl_list_vm(libxl_ctx *ctx, int
*nb_vm_out)
>>      libxl_vminfo *ptr;
>>      int idx, i, ret;
>>      xc_domaininfo_t info[1024];
>> -    int size = 1024;
>>
>> -    ptr = calloc(size, sizeof(libxl_vminfo));
>> -    if (!ptr)
>> +    ret = xc_domain_getinfolist(ctx->xch, 1, ARRAY_SIZE(info),
info);
>> +    if (ret < 0) {
>> +        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting domain
info list");
>>          return NULL;
>> +    }
>>
>> -    ret = xc_domain_getinfolist(ctx->xch, 1, 1024, info);
>> -    if (ret<0) {
>> -        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "geting domain
info list");
>> +    ptr = calloc(ret, sizeof(libxl_vminfo));
>
> We now have a possible case of calling calloc(0, sizeof(libxl_vminfo));
>
> The implementation is free to return NULL which will cause this function
> to fail in the eyes of its callers.
Good catch.
>
> Doing a calloc(min(1,ret), sizeof(libxl_vminfo)); will suffice, as the
> callers already have to correctly deal with 0 domains but some allocated
> memory as a result of this function.
v2 on its way...

- Matthew

While at it, tidy up the function; there''s no point in allocating more
than the amount of domains actually returned by xc_domain_getinfolist
(unless 0 domains are returned, in which case we should still allocate
one libxl_vminfo struct so we can return a non-NULL result and not
appear to have failed from the caller''s perspective.)

Coverity-ID: 1055888
Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
---
v2: Always allocate at least one libxl_vminfo struct for the reason given in
the commit description.

 tools/libxl/libxl.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index b112294..944194b 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -674,17 +674,17 @@ libxl_vminfo * libxl_list_vm(libxl_ctx *ctx, int
*nb_vm_out)
     libxl_vminfo *ptr;
     int idx, i, ret;
     xc_domaininfo_t info[1024];
-    int size = 1024;
 
-    ptr = calloc(size, sizeof(libxl_vminfo));
-    if (!ptr)
+    ret = xc_domain_getinfolist(ctx->xch, 1, ARRAY_SIZE(info), info);
+    if (ret < 0) {
+        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting domain info
list");
         return NULL;
+    }
 
-    ret = xc_domain_getinfolist(ctx->xch, 1, 1024, info);
-    if (ret<0) {
-        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "geting domain info
list");
+    ptr = calloc(min(1, ret), sizeof(libxl_vminfo));
+    if (!ptr)
         return NULL;
-    }
+
     for (idx = i = 0; i < ret; i++) {
         if (libxl_is_stubdom(ctx, info[i].domain, NULL))
             continue;
-- 
1.7.10.4

On 02/12/2013 00:37, Matthew Daley wrote:
> While at it, tidy up the function; there''s no point in allocating
more
> than the amount of domains actually returned by xc_domain_getinfolist
> (unless 0 domains are returned, in which case we should still allocate
> one libxl_vminfo struct so we can return a non-NULL result and not
> appear to have failed from the caller''s perspective.)
>
> Coverity-ID: 1055888
> Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> v2: Always allocate at least one libxl_vminfo struct for the reason given
in
> the commit description.
>
>  tools/libxl/libxl.c |   14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index b112294..944194b 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -674,17 +674,17 @@ libxl_vminfo * libxl_list_vm(libxl_ctx *ctx, int
*nb_vm_out)
>      libxl_vminfo *ptr;
>      int idx, i, ret;
>      xc_domaininfo_t info[1024];
> -    int size = 1024;
>  
> -    ptr = calloc(size, sizeof(libxl_vminfo));
> -    if (!ptr)
> +    ret = xc_domain_getinfolist(ctx->xch, 1, ARRAY_SIZE(info), info);
> +    if (ret < 0) {
> +        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting domain info
list");
>          return NULL;
> +    }
>  
> -    ret = xc_domain_getinfolist(ctx->xch, 1, 1024, info);
> -    if (ret<0) {
> -        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "geting domain info
list");
> +    ptr = calloc(min(1, ret), sizeof(libxl_vminfo));
> +    if (!ptr)
>          return NULL;
> -    }
> +
>      for (idx = i = 0; i < ret; i++) {
>          if (libxl_is_stubdom(ctx, info[i].domain, NULL))
>              continue;

On 02/12/2013 00:27, Matthew Daley wrote:
> Use libxl__xs_read_checked instead of xs_read. While at it, tidy up the
> function as well.
>
> Coverity-ID: 1055891
> Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
> ---
> v2: Use libxl__xs_read_checked instead of xs_read. Tidy up the function.
>
>  tools/libxl/libxl_device.c   |   41
+++++++++++++++++------------------------
>  tools/libxl/libxl_internal.h |    3 ++-
>  2 files changed, 19 insertions(+), 25 deletions(-)
>
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index d995c83..ba7d100 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -1199,37 +1199,30 @@ int
libxl__wait_for_device_model_deprecated(libxl__gc *gc,
>                                       check_callback,
check_callback_userdata);
>  }
>  
> -int libxl__wait_for_backend(libxl__gc *gc, char *be_path, char *state)
> +int libxl__wait_for_backend(libxl__gc *gc, const char *be_path,
> +                            const char *state)
>  {
> -    libxl_ctx *ctx = libxl__gc_owner(gc);
>      int watchdog = 100;
> -    unsigned int len;
> -    char *p;
> -    char *path = GCSPRINTF("%s/state", be_path);
> -    int rc = -1;
> +    const char *p, *path = GCSPRINTF("%s/state", be_path);
> +    int rc;
> +
> +    while (watchdog-- > 0) {
> +        rc = libxl__xs_read_checked(gc, XBT_NULL, path, &p);
libxl__xs_read_checked() can return 0, with p set to NULL in the case of
an ENOENT from xenstore.

I think you need a NULL check before strcmp.

~Andrew
> +        if (rc) return rc;
>  
> -    while (watchdog > 0) {
> -        p = xs_read(ctx->xsh, XBT_NULL, path, &len);
>          if (p == NULL) {
> -            if (errno == ENOENT) {
> -                LOG(ERROR, "Backend %s does not exist",
be_path);
> -            } else {
> -                LOGE(ERROR, "Failed to access backend %s",
be_path);
> -            }
> -            goto out;
> -        } else {
> -            if (!strcmp(p, state)) {
> -                rc = 0;
> -                goto out;
> -            } else {
> -                usleep(100000);
> -                watchdog--;
> -            }
> +            LOG(ERROR, "Backend %s does not exist", be_path);
> +            return ERROR_FAIL;
>          }
> +
> +        if (!strcmp(p, state))
> +            return 0;
> +
> +        usleep(100000);
>      }
> +
>      LOG(ERROR, "Backend %s not ready", be_path);
> -out:
> -    return rc;
> +    return ERROR_FAIL;
>  }
>  
>  /*
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index a2d8247..1bd23ff 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -944,7 +944,8 @@ _hidden char *libxl__device_frontend_path(libxl__gc
*gc, libxl__device *device);
>  _hidden int libxl__parse_backend_path(libxl__gc *gc, const char *path,
>                                        libxl__device *dev);
>  _hidden int libxl__device_destroy(libxl__gc *gc, libxl__device *dev);
> -_hidden int libxl__wait_for_backend(libxl__gc *gc, char *be_path, char
*state);
> +_hidden int libxl__wait_for_backend(libxl__gc *gc, const char *be_path,
> +                                    const char *state);
>  _hidden int libxl__nic_type(libxl__gc *gc, libxl__device *dev,
>                              libxl_nic_type *nictype);
>

On Mon, Dec 2, 2013 at 1:42 PM, Andrew Cooper <andrew.cooper3@citrix.com>
wrote:
> On 02/12/2013 00:27, Matthew Daley wrote:
>> Use libxl__xs_read_checked instead of xs_read. While at it, tidy up the
>> function as well.
>>
>> Coverity-ID: 1055891
>> Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
>> ---
>> v2: Use libxl__xs_read_checked instead of xs_read. Tidy up the
function.
>>
>>  tools/libxl/libxl_device.c   |   41
+++++++++++++++++------------------------
>>  tools/libxl/libxl_internal.h |    3 ++-
>>  2 files changed, 19 insertions(+), 25 deletions(-)
>>
>> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
>> index d995c83..ba7d100 100644
>> --- a/tools/libxl/libxl_device.c
>> +++ b/tools/libxl/libxl_device.c
>> @@ -1199,37 +1199,30 @@ int
libxl__wait_for_device_model_deprecated(libxl__gc *gc,
>>                                       check_callback,
check_callback_userdata);
>>  }
>>
>> -int libxl__wait_for_backend(libxl__gc *gc, char *be_path, char *state)
>> +int libxl__wait_for_backend(libxl__gc *gc, const char *be_path,
>> +                            const char *state)
>>  {
>> -    libxl_ctx *ctx = libxl__gc_owner(gc);
>>      int watchdog = 100;
>> -    unsigned int len;
>> -    char *p;
>> -    char *path = GCSPRINTF("%s/state", be_path);
>> -    int rc = -1;
>> +    const char *p, *path = GCSPRINTF("%s/state", be_path);
>> +    int rc;
>> +
>> +    while (watchdog-- > 0) {
>> +        rc = libxl__xs_read_checked(gc, XBT_NULL, path, &p);
>
> libxl__xs_read_checked() can return 0, with p set to NULL in the case of
> an ENOENT from xenstore.
>
> I think you need a NULL check before strcmp.
>
> ~Andrew
>
>> +        if (rc) return rc;
>>
>> -    while (watchdog > 0) {
>> -        p = xs_read(ctx->xsh, XBT_NULL, path, &len);
>>          if (p == NULL) {
^ That''s checked here, isn''t it? (The diff has sneakily left
it here
inbetween a bunch of removals)

- Matthew
>> -            if (errno == ENOENT) {
>> -                LOG(ERROR, "Backend %s does not exist",
be_path);
>> -            } else {
>> -                LOGE(ERROR, "Failed to access backend %s",
be_path);
>> -            }
>> -            goto out;
>> -        } else {
>> -            if (!strcmp(p, state)) {
>> -                rc = 0;
>> -                goto out;
>> -            } else {
>> -                usleep(100000);
>> -                watchdog--;
>> -            }
>> +            LOG(ERROR, "Backend %s does not exist",
be_path);
>> +            return ERROR_FAIL;
>>          }
>> +
>> +        if (!strcmp(p, state))
>> +            return 0;
>> +
>> +        usleep(100000);
>>      }
>> +
>>      LOG(ERROR, "Backend %s not ready", be_path);
>> -out:
>> -    return rc;
>> +    return ERROR_FAIL;
>>  }
>>
>>  /*
>> diff --git a/tools/libxl/libxl_internal.h
b/tools/libxl/libxl_internal.h
>> index a2d8247..1bd23ff 100644
>> --- a/tools/libxl/libxl_internal.h
>> +++ b/tools/libxl/libxl_internal.h
>> @@ -944,7 +944,8 @@ _hidden char *libxl__device_frontend_path(libxl__gc
*gc, libxl__device *device);
>>  _hidden int libxl__parse_backend_path(libxl__gc *gc, const char *path,
>>                                        libxl__device *dev);
>>  _hidden int libxl__device_destroy(libxl__gc *gc, libxl__device *dev);
>> -_hidden int libxl__wait_for_backend(libxl__gc *gc, char *be_path, char
*state);
>> +_hidden int libxl__wait_for_backend(libxl__gc *gc, const char
*be_path,
>> +                                    const char *state);
>>  _hidden int libxl__nic_type(libxl__gc *gc, libxl__device *dev,
>>                              libxl_nic_type *nictype);
>>
>

On 02/12/2013 00:46, Matthew Daley wrote:
> On Mon, Dec 2, 2013 at 1:42 PM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>> On 02/12/2013 00:27, Matthew Daley wrote:
>>> Use libxl__xs_read_checked instead of xs_read. While at it, tidy up
the
>>> function as well.
>>>
>>> Coverity-ID: 1055891
>>> Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
>>> ---
>>> v2: Use libxl__xs_read_checked instead of xs_read. Tidy up the
function.
>>>
>>>  tools/libxl/libxl_device.c   |   41
+++++++++++++++++------------------------
>>>  tools/libxl/libxl_internal.h |    3 ++-
>>>  2 files changed, 19 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/tools/libxl/libxl_device.c
b/tools/libxl/libxl_device.c
>>> index d995c83..ba7d100 100644
>>> --- a/tools/libxl/libxl_device.c
>>> +++ b/tools/libxl/libxl_device.c
>>> @@ -1199,37 +1199,30 @@ int
libxl__wait_for_device_model_deprecated(libxl__gc *gc,
>>>                                       check_callback,
check_callback_userdata);
>>>  }
>>>
>>> -int libxl__wait_for_backend(libxl__gc *gc, char *be_path, char
*state)
>>> +int libxl__wait_for_backend(libxl__gc *gc, const char *be_path,
>>> +                            const char *state)
>>>  {
>>> -    libxl_ctx *ctx = libxl__gc_owner(gc);
>>>      int watchdog = 100;
>>> -    unsigned int len;
>>> -    char *p;
>>> -    char *path = GCSPRINTF("%s/state", be_path);
>>> -    int rc = -1;
>>> +    const char *p, *path = GCSPRINTF("%s/state",
be_path);
>>> +    int rc;
>>> +
>>> +    while (watchdog-- > 0) {
>>> +        rc = libxl__xs_read_checked(gc, XBT_NULL, path, &p);
>> libxl__xs_read_checked() can return 0, with p set to NULL in the case
of
>> an ENOENT from xenstore.
>>
>> I think you need a NULL check before strcmp.
>>
>> ~Andrew
>>
>>> +        if (rc) return rc;
>>>
>>> -    while (watchdog > 0) {
>>> -        p = xs_read(ctx->xsh, XBT_NULL, path, &len);
>>>          if (p == NULL) {
> ^ That''s checked here, isn''t it? (The diff has sneakily
left it here
> inbetween a bunch of removals)
>
> - Matthew
So it is.  That was sneaky.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
>>> -            if (errno == ENOENT) {
>>> -                LOG(ERROR, "Backend %s does not exist",
be_path);
>>> -            } else {
>>> -                LOGE(ERROR, "Failed to access backend
%s", be_path);
>>> -            }
>>> -            goto out;
>>> -        } else {
>>> -            if (!strcmp(p, state)) {
>>> -                rc = 0;
>>> -                goto out;
>>> -            } else {
>>> -                usleep(100000);
>>> -                watchdog--;
>>> -            }
>>> +            LOG(ERROR, "Backend %s does not exist",
be_path);
>>> +            return ERROR_FAIL;
>>>          }
>>> +
>>> +        if (!strcmp(p, state))
>>> +            return 0;
>>> +
>>> +        usleep(100000);
>>>      }
>>> +
>>>      LOG(ERROR, "Backend %s not ready", be_path);
>>> -out:
>>> -    return rc;
>>> +    return ERROR_FAIL;
>>>  }
>>>
>>>  /*
>>> diff --git a/tools/libxl/libxl_internal.h
b/tools/libxl/libxl_internal.h
>>> index a2d8247..1bd23ff 100644
>>> --- a/tools/libxl/libxl_internal.h
>>> +++ b/tools/libxl/libxl_internal.h
>>> @@ -944,7 +944,8 @@ _hidden char
*libxl__device_frontend_path(libxl__gc *gc, libxl__device *device);
>>>  _hidden int libxl__parse_backend_path(libxl__gc *gc, const char
*path,
>>>                                        libxl__device *dev);
>>>  _hidden int libxl__device_destroy(libxl__gc *gc, libxl__device
*dev);
>>> -_hidden int libxl__wait_for_backend(libxl__gc *gc, char *be_path,
char *state);
>>> +_hidden int libxl__wait_for_backend(libxl__gc *gc, const char
*be_path,
>>> +                                    const char *state);
>>>  _hidden int libxl__nic_type(libxl__gc *gc, libxl__device *dev,
>>>                              libxl_nic_type *nictype);
>>>

While at it, tidy up the function; there''s no point in allocating more
than the amount of domains actually returned by xc_domain_getinfolist
(unless 0 domains are returned, in which case we should still allocate
one libxl_vminfo struct so we can return a non-NULL result and not
appear to have failed from the caller''s perspective.)

Coverity-ID: 1055888
Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
---
v3: Oops, min() isn''t defined or used anywhere in libxl. Just use a
ternary
expression instead.

v2: Always allocate at least one libxl_vminfo struct for the reason given in
the commit description.

 tools/libxl/libxl.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index b112294..1a2462c 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -674,17 +674,17 @@ libxl_vminfo * libxl_list_vm(libxl_ctx *ctx, int
*nb_vm_out)
     libxl_vminfo *ptr;
     int idx, i, ret;
     xc_domaininfo_t info[1024];
-    int size = 1024;
 
-    ptr = calloc(size, sizeof(libxl_vminfo));
-    if (!ptr)
+    ret = xc_domain_getinfolist(ctx->xch, 1, ARRAY_SIZE(info), info);
+    if (ret < 0) {
+        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting domain info
list");
         return NULL;
+    }
 
-    ret = xc_domain_getinfolist(ctx->xch, 1, 1024, info);
-    if (ret<0) {
-        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "geting domain info
list");
+    ptr = calloc(ret ? ret : 1, sizeof(libxl_vminfo));
+    if (!ptr)
         return NULL;
-    }
+
     for (idx = i = 0; i < ret; i++) {
         if (libxl_is_stubdom(ctx, info[i].domain, NULL))
             continue;
-- 
1.7.10.4

On 02/12/13 02:58, Matthew Daley wrote:
> While at it, tidy up the function; there''s no point in allocating
more
> than the amount of domains actually returned by xc_domain_getinfolist
> (unless 0 domains are returned, in which case we should still allocate
> one libxl_vminfo struct so we can return a non-NULL result and not
> appear to have failed from the caller''s perspective.)
>
> Coverity-ID: 1055888
> Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
> ---
> v3: Oops, min() isn''t defined or used anywhere in libxl. Just use
a ternary
> expression instead.
>
> v2: Always allocate at least one libxl_vminfo struct for the reason given
in
> the commit description.
>
>  tools/libxl/libxl.c |   14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index b112294..1a2462c 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -674,17 +674,17 @@ libxl_vminfo * libxl_list_vm(libxl_ctx *ctx, int
*nb_vm_out)
>      libxl_vminfo *ptr;
>      int idx, i, ret;
>      xc_domaininfo_t info[1024];
> -    int size = 1024;
>  
> -    ptr = calloc(size, sizeof(libxl_vminfo));
> -    if (!ptr)
> +    ret = xc_domain_getinfolist(ctx->xch, 1, ARRAY_SIZE(info), info);
> +    if (ret < 0) {
> +        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting domain info
list");
>          return NULL;
> +    }
>  
> -    ret = xc_domain_getinfolist(ctx->xch, 1, 1024, info);
> -    if (ret<0) {
> -        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "geting domain info
list");
> +    ptr = calloc(ret ? ret : 1, sizeof(libxl_vminfo));
Now I reconsider this, a comment in the code wouldn’t go amis, given the
obscurity.

Also, given -std=gnu99, you can use the gcc-ism of "ret ?: 1"
> +    if (!ptr)
>          return NULL;
> -    }
> +
>      for (idx = i = 0; i < ret; i++) {
>          if (libxl_is_stubdom(ctx, info[i].domain, NULL))
>              continue;

On Mon, Dec 2, 2013 at 11:35 PM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 02/12/13 02:58, Matthew Daley wrote:
>> While at it, tidy up the function; there''s no point in
allocating more
>> than the amount of domains actually returned by xc_domain_getinfolist
>> (unless 0 domains are returned, in which case we should still allocate
>> one libxl_vminfo struct so we can return a non-NULL result and not
>> appear to have failed from the caller''s perspective.)
>>
>> Coverity-ID: 1055888
>> Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
>> ---
>> v3: Oops, min() isn''t defined or used anywhere in libxl. Just
use a ternary
>> expression instead.
>>
>> v2: Always allocate at least one libxl_vminfo struct for the reason
given in
>> the commit description.
>>
>>  tools/libxl/libxl.c |   14 +++++++-------
>>  1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index b112294..1a2462c 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
>> @@ -674,17 +674,17 @@ libxl_vminfo * libxl_list_vm(libxl_ctx *ctx, int
*nb_vm_out)
>>      libxl_vminfo *ptr;
>>      int idx, i, ret;
>>      xc_domaininfo_t info[1024];
>> -    int size = 1024;
>>
>> -    ptr = calloc(size, sizeof(libxl_vminfo));
>> -    if (!ptr)
>> +    ret = xc_domain_getinfolist(ctx->xch, 1, ARRAY_SIZE(info),
info);
>> +    if (ret < 0) {
>> +        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting domain
info list");
>>          return NULL;
>> +    }
>>
>> -    ret = xc_domain_getinfolist(ctx->xch, 1, 1024, info);
>> -    if (ret<0) {
>> -        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "geting domain
info list");
>> +    ptr = calloc(ret ? ret : 1, sizeof(libxl_vminfo));
>
> Now I reconsider this, a comment in the code wouldn’t go amis, given the
> obscurity.
Agreed.
>
> Also, given -std=gnu99, you can use the gcc-ism of "ret ?: 1"
Eww, but OK.

- Matthew
>
>> +    if (!ptr)
>>          return NULL;
>> -    }
>> +
>>      for (idx = i = 0; i < ret; i++) {
>>          if (libxl_is_stubdom(ctx, info[i].domain, NULL))
>>              continue;
>

On Mon, 2013-12-02 at 23:47 +1300, Matthew Daley wrote:
> > Also, given -std=gnu99, you can use the gcc-ism of "ret ?:
1"
> 
> Eww, but OK.
"Can" but not "must" ;-)

While at it, tidy up the function; there''s no point in allocating more
than the amount of domains actually returned by xc_domain_getinfolist
(unless 0 domains are returned, in which case we should still allocate
one libxl_vminfo struct so we can return a non-NULL result and not
appear to have failed from the caller''s perspective.)

Coverity-ID: 1055888
Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
---
v4: Add a comment describing the calloc malarkey

 tools/libxl/libxl.c |   19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index b112294..7308d44 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -674,17 +674,22 @@ libxl_vminfo * libxl_list_vm(libxl_ctx *ctx, int
*nb_vm_out)
     libxl_vminfo *ptr;
     int idx, i, ret;
     xc_domaininfo_t info[1024];
-    int size = 1024;
 
-    ptr = calloc(size, sizeof(libxl_vminfo));
-    if (!ptr)
+    ret = xc_domain_getinfolist(ctx->xch, 1, ARRAY_SIZE(info), info);
+    if (ret < 0) {
+        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting domain info
list");
         return NULL;
+    }
 
-    ret = xc_domain_getinfolist(ctx->xch, 1, 1024, info);
-    if (ret<0) {
-        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "geting domain info
list");
+    /*
+     * Always make sure to allocate at least one element; if we don''t
and we
+     * request zero, we (might) get back a null pointer, which if returned
+     * to our caller will make them think we''ve failed
+     */
+    ptr = calloc(ret ? ret : 1, sizeof(libxl_vminfo));
+    if (!ptr)
         return NULL;
-    }
+
     for (idx = i = 0; i < ret; i++) {
         if (libxl_is_stubdom(ctx, info[i].domain, NULL))
             continue;
-- 
1.7.10.4

On 02/12/13 11:05, Matthew Daley wrote:
> While at it, tidy up the function; there''s no point in allocating
more
> than the amount of domains actually returned by xc_domain_getinfolist
> (unless 0 domains are returned, in which case we should still allocate
> one libxl_vminfo struct so we can return a non-NULL result and not
> appear to have failed from the caller''s perspective.)
>
> Coverity-ID: 1055888
> Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> v4: Add a comment describing the calloc malarkey
>
>  tools/libxl/libxl.c |   19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index b112294..7308d44 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -674,17 +674,22 @@ libxl_vminfo * libxl_list_vm(libxl_ctx *ctx, int
*nb_vm_out)
>      libxl_vminfo *ptr;
>      int idx, i, ret;
>      xc_domaininfo_t info[1024];
> -    int size = 1024;
>  
> -    ptr = calloc(size, sizeof(libxl_vminfo));
> -    if (!ptr)
> +    ret = xc_domain_getinfolist(ctx->xch, 1, ARRAY_SIZE(info), info);
> +    if (ret < 0) {
> +        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting domain info
list");
>          return NULL;
> +    }
>  
> -    ret = xc_domain_getinfolist(ctx->xch, 1, 1024, info);
> -    if (ret<0) {
> -        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "geting domain info
list");
> +    /*
> +     * Always make sure to allocate at least one element; if we
don''t and we
> +     * request zero, we (might) get back a null pointer, which if returned
> +     * to our caller will make them think we''ve failed
> +     */
> +    ptr = calloc(ret ? ret : 1, sizeof(libxl_vminfo));
> +    if (!ptr)
>          return NULL;
> -    }
> +
>      for (idx = i = 0; i < ret; i++) {
>          if (libxl_is_stubdom(ctx, info[i].domain, NULL))
>              continue;

Matthew Daley writes ("[PATCH 02/13] libxl: check for xc_domain_setmaxmem
failure in libxl__build_pre"):
> Coverity-ID: 1087115
...
> -    xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb +
LIBXL_MAXMEM_CONSTANT);
> +    rc = xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb +
LIBXL_MAXMEM_CONSTANT);
> +    if (rc != 0) {
> +        LOG(ERROR, "Couldn''t set max memory");
> +        return rc;
> +    }
xc_domain_setmaxmem returns -1 on error, setting errno.  rc is
supposed to be a libxl error number.

There are a lot of wrong (and also deviant) error handling patterns in
libxl_dom.c.  I would recommend using the idiom found in
libxl_domain_unpause in libxl.c.

Ian.

Matthew Daley writes ("[PATCH 03/13] libxl: correct file open success check
in libxl__device_pci_reset"):
> It could, even if only in theory, be fd 0.
> 
> (This is not the same as commit 4b62556b57!)
> 
> Coverity-ID: 1055895
> Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

Thanks,
Ian.

Andrew Cooper writes ("Re: [PATCH 04/13 v2] libxl: don''t leak p in
libxl__wait_for_backend"):
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Thanks, both.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

Matthew Daley writes ("[PATCH 05/13] libxl: remove unsigned less-than-0
comparison"):
> ...from libxl_cpuid_parse_config_xend. value is unsigned so this
doesn''t
> work, and either way the following comparison on it being bigger than 3
> does what was intended here anyway.
> 
> Coverity-ID: 1055614
> Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

Matthew Daley writes ("[PATCH 06/13] libxl: actually abort if initializing
a ctx''s lock fails"):
> If initializing the ctx''s lock fails, don''t keep going,
but instead
> error out.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

Matthew Daley writes ("[PATCH 07/13] libxl: don''t leak output vcpu
info on error in libxl_list_vcpu"):
> Coverity-ID: 1055887
> Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

Matthew Daley writes ("[PATCH 08/13 v4] libxl: don''t leak ptr in
libxl_list_vm error case"):
> While at it, tidy up the function; there''s no point in allocating
more
> than the amount of domains actually returned by xc_domain_getinfolist
> (unless 0 domains are returned, in which case we should still allocate
> one libxl_vminfo struct so we can return a non-NULL result and not
> appear to have failed from the caller''s perspective.)
...
> +    ptr = calloc(ret ? ret : 1, sizeof(libxl_vminfo));
I think this should be a call to libxl__calloc, which cannot fail.

You still need to prat about with the ?: though I''m afraid.

Ian.

Coverity-ID: 1087115
Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
---
v2: Use LIBXL__LOG_ERRNO instead of LOG and return ERROR_FAIL instead of -1

 tools/libxl/libxl_dom.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index e95eff8..e696fee 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -224,7 +224,6 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
      * whatever that turns out to be.
      */
     if (libxl_defbool_val(info->numa_placement)) {
-
         if (!libxl_bitmap_is_full(&info->cpumap)) {
             LOG(ERROR, "Can run NUMA placement only if no vcpu "
                        "affinity is specified");
@@ -238,7 +237,12 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
     libxl_domain_set_nodeaffinity(ctx, domid, &info->nodemap);
     libxl_set_vcpuaffinity_all(ctx, domid, info->max_vcpus,
&info->cpumap);
 
-    xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb +
LIBXL_MAXMEM_CONSTANT);
+    if (xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb +
+        LIBXL_MAXMEM_CONSTANT) < 0) {
+        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Couldn''t set max
memory");
+        return ERROR_FAIL;
+    }
+
     xs_domid = xs_read(ctx->xsh, XBT_NULL,
"/tool/xenstored/domid", NULL);
     state->store_domid = xs_domid ? atoi(xs_domid) : 0;
     free(xs_domid);
-- 
1.7.10.4

Matthew Daley writes ("[PATCH 10/13] libxl: don''t try to fclose
file twice on error in libxl_userdata_store"):
> Do this by changing the function to not use stdio file operations, but
> just use write(2) directly.
This is not correct.  If you use write(2) directly you have to call it
in a loop, in case of partial writes.  This is quite tiresome.  I
think it would be better to simply fix the function not to "retry" the
fclose if it fails the first time.
> While at it, tidy up the function''s style issues.
This makes the result very hard to review.  It is better to split
nonfunctional changes out into their own patch.

Ian.

Matthew Daley writes ("[PATCH 09/13] libxl: don''t leak pcidevs in
libxl_pcidev_assignable"):
> Coverity-ID: 1055896
> Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

On Tue, Dec 3, 2013 at 1:08 AM, Ian Jackson <Ian.Jackson@eu.citrix.com>
wrote:
> Matthew Daley writes ("[PATCH 08/13 v4] libxl: don''t leak ptr
in libxl_list_vm error case"):
>> While at it, tidy up the function; there''s no point in
allocating more
>> than the amount of domains actually returned by xc_domain_getinfolist
>> (unless 0 domains are returned, in which case we should still allocate
>> one libxl_vminfo struct so we can return a non-NULL result and not
>> appear to have failed from the caller''s perspective.)
> ...
>> +    ptr = calloc(ret ? ret : 1, sizeof(libxl_vminfo));
>
> I think this should be a call to libxl__calloc, which cannot fail.
That adds the returned pointer to the GC, and since libxl_list_vm is
part of the public libxl interface, I don''t think that would be an
acceptable change?
>
> You still need to prat about with the ?: though I''m afraid.
You mean, you require the use of "a ?: b" over "a ? a : b"?

- Matthew

Matthew Daley writes ("[PATCH 11/13] libxl: use pipe instead of temporary
file for VNC viewer --autopass"):
> Coverity was complaining about the permissions implicitly set on the
> temporary file used to pass the VNC password to the viewer when using
> the --autopass feature. By replacing the use of the temporary file
> with a pipe, we fix the problem (well, quiesce Coverity at least), tidy
> the code and remove the buildup of temporary file cruft all at once.
I don''t think this is a good idea.  The VNC client may want to read
the file more than once.
> Coverity-ID: 1055958
I think this is a false positive.

From mkstemp(3) on Debian wheezy:

       The file is created with permissions 0600, that is, read plus write for
       owner only.  (In glibc versions 2.06 and earlier, the file  is  created
       with  permissions  0666,  that  is, read and write for all users.)  The
       returned file descriptor provides both read and  write  access  to  the
       file.   The  file  is opened with the open(2) O_EXCL flag, guaranteeing
       that the caller is the process that creates the file.

From mkstemp(3) on FreeBSD 9.2-RELEASE:

     The mkstemp() function makes the same replacement to the template and
     creates the template file, mode 0600, returning a file descriptor opened
     for reading and writing.  [...]

I was going to say that if the standard spec doesn''t have this
behaviour, it''s a bug.  But SuS agrees too.  From
http://pubs.opengroup.org/onlinepubs/9699919799/functions/mkstemp.html:

    The mkstemp() function shall create the file, and obtain a file
    descriptor for it, as if by a call to:

    open(pathname, O_RDWR|O_CREAT|O_EXCL, S_IRUSR|S_IWUSR)

So I think Coverity is just wrong about this.  Perhaps it thinks you
are using an old version of glibc with an (IMO outrageous) security
bug.

Ian.

On Tue, Dec 3, 2013 at 1:14 AM, Ian Jackson <Ian.Jackson@eu.citrix.com>
wrote:
> Matthew Daley writes ("[PATCH 10/13] libxl: don''t try to
fclose file twice on error in libxl_userdata_store"):
>> Do this by changing the function to not use stdio file operations, but
>> just use write(2) directly.
>
> This is not correct.  If you use write(2) directly you have to call it
> in a loop, in case of partial writes.  This is quite tiresome.  I
> think it would be better to simply fix the function not to
"retry" the
> fclose if it fails the first time.
Would using libxl_write_exactly be acceptable instead?
>
>> While at it, tidy up the function''s style issues.
>
> This makes the result very hard to review.  It is better to split
> nonfunctional changes out into their own patch.
Sorry, will do.

- Matthew

Matthew Daley writes ("[PATCH 12/13] libxl: don''t leak buf in
libxl_xen_console_read_start error handling"):
> Coverity-ID: 1055889
> Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
...
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 41b8f60..4cd54a8 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -5116,6 +5116,7 @@ libxl_xen_console_reader *
>      cr = malloc(sizeof(libxl_xen_console_reader));
>      if (!cr) {
>          LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "cannot malloc
libxl_xen_console_reader");
> +        free(buf);
>          return NULL;
>      }
In general we prefer to use the initialise everything and "goto out"
style of error handling all the time.

But in this case the only errors are allocation failures which we have
deemed fatal: so the function should simply be changed to use
libxl__zalloc, which cannot fail.

Ian.

Matthew Daley writes ("[PATCH 13/13] libxl: replace for loop with more
idiomatic do-while loop"):
> Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
Personally I think the for loop is clearer.  I''m not a fan of do
loops.

Ian.

On Tue, Dec 3, 2013 at 1:22 AM, Ian Jackson <Ian.Jackson@eu.citrix.com>
wrote:
> Matthew Daley writes ("[PATCH 11/13] libxl: use pipe instead of
temporary file for VNC viewer --autopass"):
>> Coverity was complaining about the permissions implicitly set on the
>> temporary file used to pass the VNC password to the viewer when using
>> the --autopass feature. By replacing the use of the temporary file
>> with a pipe, we fix the problem (well, quiesce Coverity at least), tidy
>> the code and remove the buildup of temporary file cruft all at once.
>
> I don''t think this is a good idea.  The VNC client may want to
read
> the file more than once.
I know it''s a handwavey argument, but doesn''t passing the
password
through stdin (vs. giving an explicit filename) imply that the file
could very well be a pipe (or at least unseekable)?
>
>> Coverity-ID: 1055958
>
> I think this is a false positive.
>
> From mkstemp(3) on Debian wheezy:
>
>        The file is created with permissions 0600, that is, read plus write
for
>        owner only.  (In glibc versions 2.06 and earlier, the file  is 
created
>        with  permissions  0666,  that  is, read and write for all users.) 
The
>        returned file descriptor provides both read and  write  access  to 
the
>        file.   The  file  is opened with the open(2) O_EXCL flag,
guaranteeing
>        that the caller is the process that creates the file.
>
> From mkstemp(3) on FreeBSD 9.2-RELEASE:
>
>      The mkstemp() function makes the same replacement to the template and
>      creates the template file, mode 0600, returning a file descriptor
opened
>      for reading and writing.  [...]
>
> I was going to say that if the standard spec doesn''t have this
> behaviour, it''s a bug.  But SuS agrees too.  From
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/mkstemp.html:
>
>     The mkstemp() function shall create the file, and obtain a file
>     descriptor for it, as if by a call to:
>
>     open(pathname, O_RDWR|O_CREAT|O_EXCL, S_IRUSR|S_IWUSR)
>
> So I think Coverity is just wrong about this.  Perhaps it thinks you
> are using an old version of glibc with an (IMO outrageous) security
> bug.
My `man mkstemp` says: "In glibc versions 2.06 and earlier, the file
is created with permissions 0666, that is, read and write for all
users.". So, a long time ago, you could very well have been outraged.

That said, I''m happy to drop this patch and tell Coverity to hush if
that''s preferable.

- Matthew

On Tue, Dec 3, 2013 at 1:26 AM, Ian Jackson <Ian.Jackson@eu.citrix.com>
wrote:
> Matthew Daley writes ("[PATCH 13/13] libxl: replace for loop with more
idiomatic do-while loop"):
>> Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
>
> Personally I think the for loop is clearer.  I''m not a fan of do
> loops.
Hrmn, really? I guess I''ll go find another bikeshed then...

- Matthew

Matthew Daley writes ("Re: [PATCH 08/13 v4] libxl: don''t leak ptr
in libxl_list_vm error case"):
> On Tue, Dec 3, 2013 at 1:08 AM, Ian Jackson
<Ian.Jackson@eu.citrix.com> wrote:
> >> +    ptr = calloc(ret ? ret : 1, sizeof(libxl_vminfo));
> >
> > I think this should be a call to libxl__calloc, which cannot fail.
> 
> That adds the returned pointer to the GC, and since libxl_list_vm is
> part of the public libxl interface, I don''t think that would be an
> acceptable change?
That''s what NOGC is for.  You pass it to libxl__calloc et al, instead
of gc.
> > You still need to prat about with the ?: though I''m afraid.
> 
> You mean, you require the use of "a ?: b" over "a ? a :
b"?
I mean only that libxl__calloc has the same unfortunate semantics as
calloc, when the size is zero, so you need to keep your existing ? :.
I agree that use of "a ?: b" in this case is a bad thing so please
don''t change that.

Thanks,
Ian.

Matthew Daley writes ("Re: [PATCH 10/13] libxl: don''t try to
fclose file twice on error in
libxl_userdata_store"):
> On Tue, Dec 3, 2013 at 1:14 AM, Ian Jackson
<Ian.Jackson@eu.citrix.com> wrote:
> > This is not correct.  If you use write(2) directly you have to call it
> > in a loop, in case of partial writes.  This is quite tiresome.  I
> > think it would be better to simply fix the function not to
"retry" the
> > fclose if it fails the first time.
> 
> Would using libxl_write_exactly be acceptable instead?
Yes, I guess, if you prefer.

Ian.

Do this by changing the function to not use stdio file operations, but
just use write(2) directly.

While at it, tidy up the function''s style issues.

Coverity-ID: 1056195
Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
---
v2: Use libxl_write_exactly instead of write to ensure all the userdata is
written out

 tools/libxl/libxl_dom.c |   35 ++++++++++++++---------------------
 1 file changed, 14 insertions(+), 21 deletions(-)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index faba32f..55f74b2 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1603,8 +1603,6 @@ int libxl_userdata_store(libxl_ctx *ctx, uint32_t domid,
     const char *newfilename;
     int e, rc;
     int fd = -1;
-    FILE *f = NULL;
-    size_t rs;
 
     filename = userdata_path(gc, domid, userdata_userid, "d");
     if (!filename) {
@@ -1625,38 +1623,33 @@ int libxl_userdata_store(libxl_ctx *ctx, uint32_t domid,
 
     rc = ERROR_FAIL;
 
-    fd= open(newfilename, O_RDWR|O_CREAT|O_TRUNC, 0600);
-    if (fd<0)
+    fd = open(newfilename, O_RDWR | O_CREAT | O_TRUNC, 0600);
+    if (fd < 0)
         goto err;
 
-    f= fdopen(fd, "wb");
-    if (!f)
+    if (libxl_write_exactly(ctx, fd, data, datalen, "userdata",
newfilename))
         goto err;
-    fd = -1;
 
-    rs = fwrite(data, 1, datalen, f);
-    if (rs != datalen) {
-        assert(ferror(f));
+    if (close(fd) < 0) {
+        fd = -1;
         goto err;
     }
+    fd = -1;
 
-    if (fclose(f))
-        goto err;
-    f = 0;
-
-    if (rename(newfilename,filename))
+    if (rename(newfilename, filename))
         goto err;
 
     rc = 0;
 
 err:
-    e = errno;
-    if (f) fclose(f);
-    if (fd>=0) close(fd);
+    if (fd >= 0) {
+        e = errno;
+        close(fd);
+        errno = e;
+    }
 
-    errno = e;
-    if ( rc )
-        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "cannot write %s for
%s",
+    if (rc)
+        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "cannot write/rename %s
for %s",
                  newfilename, filename);
 out:
     GC_FREE;
-- 
1.7.10.4

Do this by changing the function to not use stdio file operations, but
just use the fd directly with libxl_write_exactly.

While at it, tidy up the function''s style issues.

Coverity-ID: 1056195
Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
---
v3: Fix the commit message

v2: Use libxl_write_exactly instead of write to ensure all the userdata is
written out

 tools/libxl/libxl_dom.c |   35 ++++++++++++++---------------------
 1 file changed, 14 insertions(+), 21 deletions(-)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index faba32f..55f74b2 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1603,8 +1603,6 @@ int libxl_userdata_store(libxl_ctx *ctx, uint32_t domid,
     const char *newfilename;
     int e, rc;
     int fd = -1;
-    FILE *f = NULL;
-    size_t rs;
 
     filename = userdata_path(gc, domid, userdata_userid, "d");
     if (!filename) {
@@ -1625,38 +1623,33 @@ int libxl_userdata_store(libxl_ctx *ctx, uint32_t domid,
 
     rc = ERROR_FAIL;
 
-    fd= open(newfilename, O_RDWR|O_CREAT|O_TRUNC, 0600);
-    if (fd<0)
+    fd = open(newfilename, O_RDWR | O_CREAT | O_TRUNC, 0600);
+    if (fd < 0)
         goto err;
 
-    f= fdopen(fd, "wb");
-    if (!f)
+    if (libxl_write_exactly(ctx, fd, data, datalen, "userdata",
newfilename))
         goto err;
-    fd = -1;
 
-    rs = fwrite(data, 1, datalen, f);
-    if (rs != datalen) {
-        assert(ferror(f));
+    if (close(fd) < 0) {
+        fd = -1;
         goto err;
     }
+    fd = -1;
 
-    if (fclose(f))
-        goto err;
-    f = 0;
-
-    if (rename(newfilename,filename))
+    if (rename(newfilename, filename))
         goto err;
 
     rc = 0;
 
 err:
-    e = errno;
-    if (f) fclose(f);
-    if (fd>=0) close(fd);
+    if (fd >= 0) {
+        e = errno;
+        close(fd);
+        errno = e;
+    }
 
-    errno = e;
-    if ( rc )
-        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "cannot write %s for
%s",
+    if (rc)
+        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "cannot write/rename %s
for %s",
                  newfilename, filename);
 out:
     GC_FREE;
-- 
1.7.10.4

Use libxl__zallocs instead of plain mallocs + memset.

Coverity-ID: 1055889
Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
---
v2: Use libxl__zallocs instead of custom error handling. I''m not sure
if this
is what you had in mind, Ian, specifically the use of GC_INIT/GC_FREE and
NOGC vs. just using &ctx->nogc_gc

 tools/libxl/libxl.c |   19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index a64186a..bdd721e 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5108,29 +5108,18 @@ int libxl_send_debug_keys(libxl_ctx *ctx, char *keys)
 libxl_xen_console_reader *
     libxl_xen_console_read_start(libxl_ctx *ctx, int clear)
 {
+    GC_INIT(ctx);
     libxl_xen_console_reader *cr;
     unsigned int size = 16384;
-    char *buf = malloc(size);
-
-    if (!buf) {
-        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "cannot malloc buffer for
libxl_xen_console_reader,"
-            " size is %u", size);
-        return NULL;
-    }
 
-    cr = malloc(sizeof(libxl_xen_console_reader));
-    if (!cr) {
-        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "cannot malloc
libxl_xen_console_reader");
-        return NULL;
-    }
-
-    memset(cr, 0, sizeof(libxl_xen_console_reader));
-    cr->buffer = buf;
+    cr = libxl__zalloc(NOGC, sizeof(libxl_xen_console_reader));
+    cr->buffer = libxl__zalloc(NOGC, size);
     cr->size = size;
     cr->count = size;
     cr->clear = clear;
     cr->incremental = 1;
 
+    GC_FREE;
     return cr;
 }
 
-- 
1.7.10.4

While at it, tidy up the function; there''s no point in allocating more
than the amount of domains actually returned by xc_domain_getinfolist
(barring the caveat described in the newly-added comment)

Coverity-ID: 1055888
Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
---
v5: Use libxl__calloc instead of calloc

 tools/libxl/libxl.c |   27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 67a8e0e..3b73d99 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -671,20 +671,24 @@ out:
  * be an aggregate of multiple domains. */
 libxl_vminfo * libxl_list_vm(libxl_ctx *ctx, int *nb_vm_out)
 {
-    libxl_vminfo *ptr;
+    GC_INIT(ctx);
+    libxl_vminfo *ptr = NULL;
     int idx, i, ret;
     xc_domaininfo_t info[1024];
-    int size = 1024;
 
-    ptr = calloc(size, sizeof(libxl_vminfo));
-    if (!ptr)
-        return NULL;
-
-    ret = xc_domain_getinfolist(ctx->xch, 1, 1024, info);
-    if (ret<0) {
-        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "geting domain info
list");
-        return NULL;
+    ret = xc_domain_getinfolist(ctx->xch, 1, ARRAY_SIZE(info), info);
+    if (ret < 0) {
+        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting domain info
list");
+        goto out;
     }
+
+    /*
+     * Always make sure to allocate at least one element; if we don''t
and we
+     * request zero, libxl__calloc (might) think its internal call to calloc
+     * has failed (if it returns null), if so it would kill our process.
+     */
+    ptr = libxl__calloc(NOGC, ret ? ret : 1, sizeof(libxl_vminfo));
+
     for (idx = i = 0; i < ret; i++) {
         if (libxl_is_stubdom(ctx, info[i].domain, NULL))
             continue;
@@ -694,6 +698,9 @@ libxl_vminfo * libxl_list_vm(libxl_ctx *ctx, int *nb_vm_out)
         idx++;
     }
     *nb_vm_out = idx;
+
+out:
+    GC_FREE;
     return ptr;
 }
 
-- 
1.7.10.4

On Tue, 2013-12-03 at 14:29 +1300, Matthew Daley wrote:
> While at it, tidy up the function; there''s no point in allocating
more
> than the amount of domains actually returned by xc_domain_getinfolist
> (barring the caveat described in the newly-added comment)
> 
> Coverity-ID: 1055888
> Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
> ---
> v5: Use libxl__calloc instead of calloc
> 
>  tools/libxl/libxl.c |   27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 67a8e0e..3b73d99 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -671,20 +671,24 @@ out:
>   * be an aggregate of multiple domains. */
>  libxl_vminfo * libxl_list_vm(libxl_ctx *ctx, int *nb_vm_out)
>  {
> -    libxl_vminfo *ptr;
> +    GC_INIT(ctx);
> +    libxl_vminfo *ptr = NULL;
>      int idx, i, ret;
>      xc_domaininfo_t info[1024];
> -    int size = 1024;
>  
> -    ptr = calloc(size, sizeof(libxl_vminfo));
> -    if (!ptr)
> -        return NULL;
> -
> -    ret = xc_domain_getinfolist(ctx->xch, 1, 1024, info);
> -    if (ret<0) {
> -        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "geting domain info
list");
> -        return NULL;
> +    ret = xc_domain_getinfolist(ctx->xch, 1, ARRAY_SIZE(info), info);
> +    if (ret < 0) {
> +        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting domain info
list");
> +        goto out;
>      }
> +
> +    /*
> +     * Always make sure to allocate at least one element; if we
don''t and we
> +     * request zero, libxl__calloc (might) think its internal call to
calloc
> +     * has failed (if it returns null), if so it would kill our process.
Is size==0 something we could/should handle in our libxl__*alloc
wrappers?

Or maybe this is something we should handle here e.g. by returning NULL,
except perhaps our API doesn''t allow for that?
> +     */
> +    ptr = libxl__calloc(NOGC, ret ? ret : 1, sizeof(libxl_vminfo));
> +
>      for (idx = i = 0; i < ret; i++) {
>          if (libxl_is_stubdom(ctx, info[i].domain, NULL))
>              continue;
> @@ -694,6 +698,9 @@ libxl_vminfo * libxl_list_vm(libxl_ctx *ctx, int
*nb_vm_out)
>          idx++;
>      }
>      *nb_vm_out = idx;
> +
> +out:
> +    GC_FREE;
>      return ptr;
>  }
>

On 03/12/13 10:21, Ian Campbell wrote:
> On Tue, 2013-12-03 at 14:29 +1300, Matthew Daley wrote:
>> While at it, tidy up the function; there''s no point in
allocating more
>> than the amount of domains actually returned by xc_domain_getinfolist
>> (barring the caveat described in the newly-added comment)
>>
>> Coverity-ID: 1055888
>> Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
>> ---
>> v5: Use libxl__calloc instead of calloc
>>
>>  tools/libxl/libxl.c |   27 +++++++++++++++++----------
>>  1 file changed, 17 insertions(+), 10 deletions(-)
>>
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index 67a8e0e..3b73d99 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
>> @@ -671,20 +671,24 @@ out:
>>   * be an aggregate of multiple domains. */
>>  libxl_vminfo * libxl_list_vm(libxl_ctx *ctx, int *nb_vm_out)
>>  {
>> -    libxl_vminfo *ptr;
>> +    GC_INIT(ctx);
>> +    libxl_vminfo *ptr = NULL;
>>      int idx, i, ret;
>>      xc_domaininfo_t info[1024];
>> -    int size = 1024;
>>  
>> -    ptr = calloc(size, sizeof(libxl_vminfo));
>> -    if (!ptr)
>> -        return NULL;
>> -
>> -    ret = xc_domain_getinfolist(ctx->xch, 1, 1024, info);
>> -    if (ret<0) {
>> -        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "geting domain
info list");
>> -        return NULL;
>> +    ret = xc_domain_getinfolist(ctx->xch, 1, ARRAY_SIZE(info),
info);
>> +    if (ret < 0) {
>> +        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting domain
info list");
>> +        goto out;
>>      }
>> +
>> +    /*
>> +     * Always make sure to allocate at least one element; if we
don''t and we
>> +     * request zero, libxl__calloc (might) think its internal call to
calloc
>> +     * has failed (if it returns null), if so it would kill our
process.
> Is size==0 something we could/should handle in our libxl__*alloc
> wrappers?
>
> Or maybe this is something we should handle here e.g. by returning NULL,
> except perhaps our API doesn''t allow for that?
The current API means that returning NULL from here constitutes a
failure, which needs to be distinct from "I did what you asked and there
are no domains".

*nb_vm_out is a second return parameter from this function.

~Andrew
>
>> +     */
>> +    ptr = libxl__calloc(NOGC, ret ? ret : 1, sizeof(libxl_vminfo));
>> +
>>      for (idx = i = 0; i < ret; i++) {
>>          if (libxl_is_stubdom(ctx, info[i].domain, NULL))
>>              continue;
>> @@ -694,6 +698,9 @@ libxl_vminfo * libxl_list_vm(libxl_ctx *ctx, int
*nb_vm_out)
>>          idx++;
>>      }
>>      *nb_vm_out = idx;
>> +
>> +out:
>> +    GC_FREE;
>>      return ptr;
>>  }
>>  
>

Matthew Daley writes ("[PATCH 12/13 v2] libxl: don''t leak buf in
libxl_xen_console_read_start error handling"):
> Use libxl__zallocs instead of plain mallocs + memset.
> 
> Coverity-ID: 1055889
> Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>
> v2: Use libxl__zallocs instead of custom error handling. I''m not
sure if this
> is what you had in mind, Ian, specifically the use of GC_INIT/GC_FREE and
> NOGC vs. just using &ctx->nogc_gc
Yes, this was exactly what I meant.

As a bonus it''s now much shorter and also more obviously correct :-).

Thanks,
Ian.

Matthew Daley writes ("[PATCH 10/13 v3] libxl: don''t try to fclose
file twice on error in libxl_userdata_store"):
> Do this by changing the function to not use stdio file operations, but
> just use the fd directly with libxl_write_exactly.
> 
> While at it, tidy up the function''s style issues.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

Ping?

On Tue, Dec 3, 2013 at 2:29 PM, Matthew Daley <mattd@bugfuzz.com>
wrote:
> While at it, tidy up the function; there''s no point in allocating
more
> than the amount of domains actually returned by xc_domain_getinfolist
> (barring the caveat described in the newly-added comment)
>
> Coverity-ID: 1055888
> Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
> ---
> v5: Use libxl__calloc instead of calloc
>
>  tools/libxl/libxl.c |   27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 67a8e0e..3b73d99 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -671,20 +671,24 @@ out:
>   * be an aggregate of multiple domains. */
>  libxl_vminfo * libxl_list_vm(libxl_ctx *ctx, int *nb_vm_out)
>  {
> -    libxl_vminfo *ptr;
> +    GC_INIT(ctx);
> +    libxl_vminfo *ptr = NULL;
>      int idx, i, ret;
>      xc_domaininfo_t info[1024];
> -    int size = 1024;
>
> -    ptr = calloc(size, sizeof(libxl_vminfo));
> -    if (!ptr)
> -        return NULL;
> -
> -    ret = xc_domain_getinfolist(ctx->xch, 1, 1024, info);
> -    if (ret<0) {
> -        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "geting domain info
list");
> -        return NULL;
> +    ret = xc_domain_getinfolist(ctx->xch, 1, ARRAY_SIZE(info), info);
> +    if (ret < 0) {
> +        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting domain info
list");
> +        goto out;
>      }
> +
> +    /*
> +     * Always make sure to allocate at least one element; if we
don''t and we
> +     * request zero, libxl__calloc (might) think its internal call to
calloc
> +     * has failed (if it returns null), if so it would kill our process.
> +     */
> +    ptr = libxl__calloc(NOGC, ret ? ret : 1, sizeof(libxl_vminfo));
> +
>      for (idx = i = 0; i < ret; i++) {
>          if (libxl_is_stubdom(ctx, info[i].domain, NULL))
>              continue;
> @@ -694,6 +698,9 @@ libxl_vminfo * libxl_list_vm(libxl_ctx *ctx, int
*nb_vm_out)
>          idx++;
>      }
>      *nb_vm_out = idx;
> +
> +out:
> +    GC_FREE;
>      return ptr;
>  }
>
> --
> 1.7.10.4
>

Ping?

On Tue, Dec 3, 2013 at 1:11 AM, Matthew Daley <mattd@bugfuzz.com>
wrote:
> Coverity-ID: 1087115
> Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
> ---
> v2: Use LIBXL__LOG_ERRNO instead of LOG and return ERROR_FAIL instead of -1
>
>  tools/libxl/libxl_dom.c |    8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index e95eff8..e696fee 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -224,7 +224,6 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
>       * whatever that turns out to be.
>       */
>      if (libxl_defbool_val(info->numa_placement)) {
> -
>          if (!libxl_bitmap_is_full(&info->cpumap)) {
>              LOG(ERROR, "Can run NUMA placement only if no vcpu "
>                         "affinity is specified");
> @@ -238,7 +237,12 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
>      libxl_domain_set_nodeaffinity(ctx, domid, &info->nodemap);
>      libxl_set_vcpuaffinity_all(ctx, domid, info->max_vcpus,
&info->cpumap);
>
> -    xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb +
LIBXL_MAXMEM_CONSTANT);
> +    if (xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb +
> +        LIBXL_MAXMEM_CONSTANT) < 0) {
> +        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Couldn''t
set max memory");
> +        return ERROR_FAIL;
> +    }
> +
>      xs_domid = xs_read(ctx->xsh, XBT_NULL,
"/tool/xenstored/domid", NULL);
>      state->store_domid = xs_domid ? atoi(xs_domid) : 0;
>      free(xs_domid);
> --
> 1.7.10.4
>

Ping?

On Sun, Dec 1, 2013 at 11:14 PM, Matthew Daley <mattd@bugfuzz.com>
wrote:
> Both src[i].size and delta are unsigned, so checking their difference
> for being less than 0 doesn''t work.
>
> Coverity-ID: 1055615
> Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
> ---
>  tools/libxl/libxl_x86.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
> index e1c183f..b11d036 100644
> --- a/tools/libxl/libxl_x86.c
> +++ b/tools/libxl/libxl_x86.c
> @@ -125,7 +125,7 @@ static int e820_sanitize(libxl_ctx *ctx, struct
e820entry src[],
>              src[i].type = E820_UNUSABLE;
>              delta = ram_end - src[i].addr;
>              /* The end < ram_end should weed this out */
> -            if (src[i].size - delta < 0)
> +            if (src[i].size < delta)
>                  src[i].type = 0;
>              else {
>                  src[i].size -= delta;
> --
> 1.7.10.4
>

On ven, 2013-12-13 at 18:53 +1300, Matthew Daley wrote:
> Ping?
> 
> On Tue, Dec 3, 2013 at 1:11 AM, Matthew Daley <mattd@bugfuzz.com>
wrote:
> > Coverity-ID: 1087115
> > Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
>
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

Regards,
Dario
> > ---
> > v2: Use LIBXL__LOG_ERRNO instead of LOG and return ERROR_FAIL instead
of -1
> >
> >  tools/libxl/libxl_dom.c |    8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> > index e95eff8..e696fee 100644
> > --- a/tools/libxl/libxl_dom.c
> > +++ b/tools/libxl/libxl_dom.c
> > @@ -224,7 +224,6 @@ int libxl__build_pre(libxl__gc *gc, uint32_t
domid,
> >       * whatever that turns out to be.
> >       */
> >      if (libxl_defbool_val(info->numa_placement)) {
> > -
> >          if (!libxl_bitmap_is_full(&info->cpumap)) {
> >              LOG(ERROR, "Can run NUMA placement only if no vcpu
"
> >                         "affinity is specified");
> > @@ -238,7 +237,12 @@ int libxl__build_pre(libxl__gc *gc, uint32_t
domid,
> >      libxl_domain_set_nodeaffinity(ctx, domid, &info->nodemap);
> >      libxl_set_vcpuaffinity_all(ctx, domid, info->max_vcpus,
&info->cpumap);
> >
> > -    xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb +
LIBXL_MAXMEM_CONSTANT);
> > +    if (xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb
+
> > +        LIBXL_MAXMEM_CONSTANT) < 0) {
> > +        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
"Couldn''t set max memory");
> > +        return ERROR_FAIL;
> > +    }
> > +
> >      xs_domid = xs_read(ctx->xsh, XBT_NULL,
"/tool/xenstored/domid", NULL);
> >      state->store_domid = xs_domid ? atoi(xs_domid) : 0;
> >      free(xs_domid);
> > --
> > 1.7.10.4
> >
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

On 13/12/2013 05:54, Matthew Daley wrote:
> Ping?
>
> On Sun, Dec 1, 2013 at 11:14 PM, Matthew Daley <mattd@bugfuzz.com>
wrote:
>> Both src[i].size and delta are unsigned, so checking their difference
>> for being less than 0 doesn''t work.
>>
>> Coverity-ID: 1055615
>> Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>>  tools/libxl/libxl_x86.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
>> index e1c183f..b11d036 100644
>> --- a/tools/libxl/libxl_x86.c
>> +++ b/tools/libxl/libxl_x86.c
>> @@ -125,7 +125,7 @@ static int e820_sanitize(libxl_ctx *ctx, struct
e820entry src[],
>>              src[i].type = E820_UNUSABLE;
>>              delta = ram_end - src[i].addr;
>>              /* The end < ram_end should weed this out */
>> -            if (src[i].size - delta < 0)
>> +            if (src[i].size < delta)
>>                  src[i].type = 0;
>>              else {
>>                  src[i].size -= delta;
>> --
>> 1.7.10.4
>>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Ian Campbell writes ("Re: [PATCH 08/13 v5] libxl: don''t leak ptr
in libxl_list_vm error case"):
> On Tue, 2013-12-03 at 14:29 +1300, Matthew Daley wrote:
> > +    /*
> > +  * Always make sure to allocate at least one element; if we
don''t and we
> > +  * request zero, libxl__calloc (might) think its internal call to
calloc
> > +  * has failed (if it returns null), if so it would kill our process.
[rewrapped -iwj]
> 
> Is size==0 something we could/should handle in our libxl__*alloc
> wrappers?
I think so.  I think they should promise that if you pass size==0 you
get a non-null pointer.  Calling realloc with size==0 should crash.

Matthew Daley writes ("Re: [PATCH 08/13 v5] libxl: don''t leak ptr
in libxl_list_vm error case"):
> Ping?
See Ian C''s comment above, which AFAICT hasn''t been answered.

Thanks,
Ian.

On 13/12/2013 16:52, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH 08/13 v5] libxl: don''t leak
ptr in libxl_list_vm error case"):
>> On Tue, 2013-12-03 at 14:29 +1300, Matthew Daley wrote:
>>> +    /*
>>> +  * Always make sure to allocate at least one element; if we
don''t and we
>>> +  * request zero, libxl__calloc (might) think its internal call to
calloc
>>> +  * has failed (if it returns null), if so it would kill our
process.
> [rewrapped -iwj]
>> Is size==0 something we could/should handle in our libxl__*alloc
>> wrappers?
> I think so.  I think they should promise that if you pass size==0 you
> get a non-null pointer.  Calling realloc with size==0 should crash.
Can we not?

Having a non-NULL pointer to a 0 length buffer is madness, whose use
should not be further encouraged.

Furthermore, code which ends calling libxl__*alloc() with a size of 0
*is* buggy, and should suffer an abort(), just as much as attempting to
realloc to a size of 0.
>
> Matthew Daley writes ("Re: [PATCH 08/13 v5] libxl: don''t leak
ptr in libxl_list_vm error case"):
>> Ping?
> See Ian C''s comment above, which AFAICT hasn''t been
answered.
>
> Thanks,
> Ian.
I believe I suitably answered that question, and justified why it had to
stay.

There is an API difference between returning NULL (Call to list domains
failed), and non NULL but with nb_domains = 0 (Call to list domains
succeeded but there are no domains).

~Andrew

Andrew Cooper writes ("Re: [PATCH 08/13 v5] libxl: don''t leak ptr
in libxl_list_vm error case [and 1 more
messages]"):
> On 13/12/2013 16:52, Ian Jackson wrote:
> > I think so.  I think they should promise that if you pass size==0 you
> > get a non-null pointer.  Calling realloc with size==0 should crash.
> 
> Can we not?
> 
> Having a non-NULL pointer to a 0 length buffer is madness, whose use
> should not be further encouraged.
I don''t know where you get the idea that such a pointer is
"madness".
It''s a permitted response from malloc() according to the spec and
indeed it''s what glibc''s malloc does!
> Furthermore, code which ends calling libxl__*alloc() with a size of 0
> *is* buggy, and should suffer an abort(),
Calling malloc(0) is certainly not buggy.  It has behaviour which is
clearly defined by the spec, and if you are careful with your error
handling it is possible to use it correct.  It''s just a bit awkward.

libxl__*alloc should have an interface at least as capable as malloc()
- that is, libxl__zalloc(,0) should be fine.

Or to put it another way calling libxl_*alloc(,0) is only buggy
because we''ve made it be buggy (or defined it to be so).

I wouldn''t mind if libxl_*alloc(,0) succeeded and returned NULL, but I
think having them return non-NULL is probably better.
> just as much as attempting to realloc to a size of 0.
realloc(,0) is problematic in the standard C API because it''s not
possible to tell, if you get NULL back, whether the original block was
freed.

The problem with libxl__realloc(,,0) is that it''s not clear whether
the caller intends to free the block, or resize it to size 0.  I think
none of our use patterns will end up calling it.  But I am open to
arguments that libxl__realloc(,,0) should be defined to succeed and
return NULL, or to succeed and return non-NULL.


But since this is controversial I should probably just apply Matthew''s
patch, which is a clear improvement, while we argue about it.  So:

Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

Ian.

Dario Faggioli writes ("Re: [Xen-devel] [PATCH 02/13 v2] libxl: check for
xc_domain_setmaxmem failure in libxl__build_pre"):
> On ven, 2013-12-13 at 18:53 +1300, Matthew Daley wrote:
> > Ping?
> > 
> > On Tue, Dec 3, 2013 at 1:11 AM, Matthew Daley
<mattd@bugfuzz.com> wrote:
> > > Coverity-ID: 1087115
> > > Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
> >
> Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

Sorry, this one seems to have been dropped.

Ian.

Matthew Daley writes ("[PATCH 01/13] libxl: fix unsigned less-than-0
comparison in e820_sanitize"):
> Both src[i].size and delta are unsigned, so checking their difference
> for being less than 0 doesn''t work.
> 
> Coverity-ID: 1055615
> Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

Sorry to have dropped this.

Thanks,
Ian.

On Sat, Dec 14, 2013 at 5:52 AM, Ian Jackson <Ian.Jackson@eu.citrix.com>
wrote:
> Ian Campbell writes ("Re: [PATCH 08/13 v5] libxl: don''t leak
ptr in libxl_list_vm error case"):
>> On Tue, 2013-12-03 at 14:29 +1300, Matthew Daley wrote:
>> > +    /*
>> > +  * Always make sure to allocate at least one element; if we
don''t and we
>> > +  * request zero, libxl__calloc (might) think its internal call
to calloc
>> > +  * has failed (if it returns null), if so it would kill our
process.
> [rewrapped -iwj]
>>
>> Is size==0 something we could/should handle in our libxl__*alloc
>> wrappers?
>
> I think so.  I think they should promise that if you pass size==0 you
> get a non-null pointer.  Calling realloc with size==0 should crash.
>
> Matthew Daley writes ("Re: [PATCH 08/13 v5] libxl: don''t leak
ptr in libxl_list_vm error case"):
>> Ping?
>
> See Ian C''s comment above, which AFAICT hasn''t been
answered.
Sorry, I implicitly agreed with Andrew''s comment (at least, the second
part...)

For the record, for custom wrapper functions like these I usually
prefer to stick as close to the existing functions'' semantics as
possible, not so much out of love for standards but out of desiring
the absence of surprising behaviour.

As for the second part of the comment, as Andrew already mentioned,
the nb_domains output argument is sufficient to distinguish failure
vs. 0 domains from libxl_list_vm already.

Out of curiosity, looking through libxl_list_vm''s existing callers:
- libxl__domain_make is only (ab)using libxl_list_vm to get the number
of vms via nb_domains... it just frees the vm list right afterward!
- xl_cmdimpl.c:print_uptime doesn''t even check libxl_list_vm''s
result
:D  I will patch this shortly.

(Also, I see you have applied my patch regardless of this discussion; thanks.)

- Matthew
>
> Thanks,
> Ian.

On Sat, Dec 14, 2013 at 12:22 PM, Matthew Daley <mattd@bugfuzz.com>
wrote:
> On Sat, Dec 14, 2013 at 5:52 AM, Ian Jackson
<Ian.Jackson@eu.citrix.com> wrote:
>> Ian Campbell writes ("Re: [PATCH 08/13 v5] libxl: don''t
leak ptr in libxl_list_vm error case"):
>>> On Tue, 2013-12-03 at 14:29 +1300, Matthew Daley wrote:
>>> > +    /*
>>> > +  * Always make sure to allocate at least one element; if we
don''t and we
>>> > +  * request zero, libxl__calloc (might) think its internal
call to calloc
>>> > +  * has failed (if it returns null), if so it would kill our
process.
>> [rewrapped -iwj]
>>>
>>> Is size==0 something we could/should handle in our libxl__*alloc
>>> wrappers?
>>
>> I think so.  I think they should promise that if you pass size==0 you
>> get a non-null pointer.  Calling realloc with size==0 should crash.
>>
>> Matthew Daley writes ("Re: [PATCH 08/13 v5] libxl: don''t
leak ptr in libxl_list_vm error case"):
>>> Ping?
>>
>> See Ian C''s comment above, which AFAICT hasn''t been
answered.
>
> Sorry, I implicitly agreed with Andrew''s comment (at least, the
second part...)
>
> For the record, for custom wrapper functions like these I usually
> prefer to stick as close to the existing functions'' semantics as
> possible, not so much out of love for standards but out of desiring
> the absence of surprising behaviour.
Although, I suppose changing things wrt. the standards would just
introduce new slightly-surprising behaviours vs. having existing
really-surprising ones...

- Matthew
>
> As for the second part of the comment, as Andrew already mentioned,
> the nb_domains output argument is sufficient to distinguish failure
> vs. 0 domains from libxl_list_vm already.
>
> Out of curiosity, looking through libxl_list_vm''s existing
callers:
> - libxl__domain_make is only (ab)using libxl_list_vm to get the number
> of vms via nb_domains... it just frees the vm list right afterward!
> - xl_cmdimpl.c:print_uptime doesn''t even check
libxl_list_vm''s result
> :D  I will patch this shortly.
>
> (Also, I see you have applied my patch regardless of this discussion;
thanks.)
>
> - Matthew
>
>>
>> Thanks,
>> Ian.

Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
---
 tools/libxl/xl_cmdimpl.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index bd26bcc..7c5b9ec 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -6245,6 +6245,10 @@ static void print_uptime(int short_mode, uint32_t doms[],
int nb_doms)
     if (nb_doms == 0) {
         print_dom0_uptime(short_mode, now);
         info = libxl_list_vm(ctx, &nb_vm);
+        if (info == NULL) {
+            fprintf(stderr, "Could not list vms.\n");
+            return;
+        }
         for (i = 0; i < nb_vm; i++)
             print_domU_uptime(info[i].domid, short_mode, now);
         libxl_vminfo_list_free(info, nb_vm);
-- 
1.7.10.4