[PATCH for-4.6 00/13] tools: fixes inspired by Coverity scan

[PATCH for-4.6 00/13] tools: fixes inspired by Coverity scan

[Wei Liu]

Wei Liu (13):
  libxl: use thread-safe localtime_t and handle NULL
  libxl: properly clean up array in libxl_list_cpupool failure path
  libxl: remove a bunch of pointless assignments
  xl: free pid string in do_daemonize
  xl: check json string is not null before printing in create_domain
  xl: call libxl_dominfo_{init,dispose} in psr_cmt_show
  xl: call libxl_dominfo_{init,dispose} in main_cpupoolnumasplit
  xl: call libxl_dominfo_init in main_list
  xl: call libxl_bitmap_{init,dispose} in main_cpupoolcreate
  xl: valid fd can be 0 in main_loadpolicy
  xl: free event struct after use in main_shutdown_or_reboot
  tools/ocaml: call libxl_dominfo_{init,dispose} in stub
  tools/ocaml: handle strdup failure in stub_xl_device_disk_of_vdev

 tools/libxl/libxl.c                  | 31 ++++++++++++++++++++---------
 tools/libxl/libxl_dom.c              |  2 +-
 tools/libxl/libxl_remus_disk_drbd.c  |  2 +-
 tools/libxl/libxl_x86.c              | 11 +++++++++--
 tools/libxl/xl_cmdimpl.c             | 38 ++++++++++++++++++++++++++++--------
 tools/ocaml/libs/xl/xenlight_stubs.c | 12 ++++++++++--
 6 files changed, 73 insertions(+), 23 deletions(-)

-- 
1.9.1

[PATCH for-4.6 01/13] libxl: use thread-safe localtime_t and handle NULL

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_x86.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
index 8cd15ca..bac0b8f 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -306,10 +306,16 @@ int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
     rtc_timeoffset = d_config->b_info.rtc_timeoffset;
     if (libxl_defbool_val(d_config->b_info.localtime)) {
         time_t t;
-        struct tm *tm;
+        struct tm *tm, result;
 
         t = time(NULL);
-        tm = localtime(&t);
+        tm = localtime_r(&t, &result);
+
+        if (!tm) {
+            LOGE(ERROR, "Failed to call localtime_r");
+            ret = ERROR_FAIL;
+            goto out;
+        }
 
         rtc_timeoffset += tm->tm_gmtoff;
     }
@@ -335,6 +341,7 @@ int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
         }
     }
 
+out:
     return ret;
 }
 
-- 
1.9.1

[PATCH for-4.6 02/13] libxl: properly clean up array in libxl_list_cpupool failure path

[Wei Liu]

Document how cpupool_info works.  Distinguish success (ERROR_FAIL +
ENOENT) vs failure in libxl_list_cpupool and properly clean up the array
in failure path.

Also switch to libxl__realloc and call libxl_cpupool_{init,dispose}
where appropriate.

There is change of behaviour. Previously if memory allocation fails the
said function returns NULL. Now memory allocation failure is fatal. This
is in line with how we deal with memory allocation failure in other
places in libxl though.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Dario Faggioli <dario.faggioli@citrix.com>
---
 tools/libxl/libxl.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index ff0d616..db88ecc 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -682,6 +682,11 @@ int libxl_domain_info(libxl_ctx *ctx, libxl_dominfo *info_r,
     return 0;
 }
 
+/* Returns:
+ *   0 - success
+ *   ERROR_FAIL + errno == ENOENT - no entry found
+ *   ERROR_$FOO + errno != ENOENT - other failure
+ */
 static int cpupool_info(libxl__gc *gc,
                         libxl_cpupoolinfo *info,
                         uint32_t poolid,
@@ -737,7 +742,9 @@ int libxl_cpupool_info(libxl_ctx *ctx,
 libxl_cpupoolinfo * libxl_list_cpupool(libxl_ctx *ctx, int *nb_pool_out)
 {
     GC_INIT(ctx);
-    libxl_cpupoolinfo info, *ptr, *tmp;
+    libxl_cpupoolinfo info, *ptr;
+    bool failed = false;
+
     int i;
     uint32_t poolid;
 
@@ -745,18 +752,24 @@ libxl_cpupoolinfo * libxl_list_cpupool(libxl_ctx *ctx, int *nb_pool_out)
 
     poolid = 0;
     for (i = 0;; i++) {
-        if (cpupool_info(gc, &info, poolid, false))
+        libxl_cpupoolinfo_init(&info);
+        if (cpupool_info(gc, &info, poolid, false)) {
+            if (errno != ENOENT) failed = true;
+            libxl_cpupoolinfo_dispose(&info);
             break;
-        tmp = realloc(ptr, (i + 1) * sizeof(libxl_cpupoolinfo));
-        if (!tmp) {
-            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "allocating cpupool info");
-            libxl_cpupoolinfo_list_free(ptr, i);
-            ptr = NULL;
-            goto out;
         }
-        ptr = tmp;
+
+        ptr = libxl__realloc(NOGC, ptr, (i+1) * sizeof(libxl_cpupoolinfo));
         ptr[i] = info;
         poolid = info.poolid + 1;
+        /* Don't dispose of info because it will be returned to caller */
+    }
+
+    if (failed) {
+        libxl_cpupoolinfo_list_free(ptr, i);
+        ptr = NULL;
+        *nb_pool_out = 0;
+        goto out;
     }
 
     *nb_pool_out = i;
-- 
1.9.1

[PATCH for-4.6 03/13] xl/libxl: remove a bunch of pointless assignments

[Wei Liu]

Those values are  overwritten before they can be of any use.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_dom.c             | 2 +-
 tools/libxl/libxl_remus_disk_drbd.c | 2 +-
 tools/libxl/xl_cmdimpl.c            | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index edd7f3f..93ab8fe 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1686,7 +1686,7 @@ void libxl__domain_save(libxl__egc *egc, libxl__domain_suspend_state *dss)
 {
     STATE_AO_GC(dss->ao);
     int port;
-    int rc = ERROR_FAIL;
+    int rc;
 
     /* Convenience aliases */
     const uint32_t domid = dss->domid;
diff --git a/tools/libxl/libxl_remus_disk_drbd.c b/tools/libxl/libxl_remus_disk_drbd.c
index fc76b89..1c3a88a 100644
--- a/tools/libxl/libxl_remus_disk_drbd.c
+++ b/tools/libxl/libxl_remus_disk_drbd.c
@@ -47,7 +47,7 @@ static void drbd_async_call(libxl__egc *egc,
                             void func(libxl__remus_device *),
                             libxl__ev_child_callback callback)
 {
-    int pid = -1, rc;
+    int pid, rc;
     libxl__ao_device *aodev = &dev->aodev;
     STATE_AO_GC(dev->rds->ao);
 
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 5c6d1b0..5c74a73 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -4037,7 +4037,7 @@ static pid_t create_migration_child(const char *rune, int *send_fd,
                                         int *recv_fd)
 {
     int sendpipe[2], recvpipe[2];
-    pid_t child = -1;
+    pid_t child;
 
     if (!rune || !send_fd || !recv_fd)
         return -1;
@@ -7788,7 +7788,7 @@ int main_getenforce(int argc, char **argv)
 
 int main_setenforce(int argc, char **argv)
 {
-    int ret, mode = -1;
+    int ret, mode;
     const char *p = NULL;
 
     if (optind >= argc) {
-- 
1.9.1

[PATCH for-4.6 04/13] xl: free pid string in do_daemonize

[Wei Liu]

Pid is a null terminated string allocated by asprintf. It should be
freed after use.

Also fixed a coding style problem while I was there.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 5c74a73..bfd8e59 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -524,10 +524,12 @@ static int do_daemonize(char *name, const char *pidfile)
             exit(1);
         }
 
-        if ( close(fd) < 0 ) {
+        if (close(fd) < 0) {
             perror("Closing pidfile");
             exit(1);
         }
+
+        free(pid);
     }
 
 out:
-- 
1.9.1

[PATCH for-4.6 05/13] xl: check json string is not null before printing in create_domain

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index bfd8e59..fc61650 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -2763,6 +2763,11 @@ static uint32_t create_domain(struct domain_create *dom_info)
             printf_info_sexp(-1, &d_config, cfg_print_fh);
         } else {
             char *json = libxl_domain_config_to_json(ctx, &d_config);
+            if (!json) {
+                fprintf(stderr,
+                        "Failed to convert domain configuration to JSON\n");
+                exit(1);
+            }
             fputs(json, cfg_print_fh);
             free(json);
             flush_stream(cfg_print_fh);
-- 
1.9.1

[PATCH for-4.6 06/13] xl: call libxl_dominfo_{init, dispose} in psr_cmt_show

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index fc61650..2d95e35 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -8217,11 +8217,14 @@ static int psr_cmt_show(libxl_psr_cmt_type type, uint32_t domid)
     /* Each domain */
     if (domid != INVALID_DOMID) {
         libxl_dominfo dominfo;
+
+        libxl_dominfo_init(&dominfo);
         if (libxl_domain_info(ctx, &dominfo, domid)) {
             fprintf(stderr, "Failed to get domain info for %d\n", domid);
             return -1;
         }
         psr_cmt_print_domain_info(&dominfo, type, nr_sockets);
+        libxl_dominfo_dispose(&dominfo);
     }
     else
     {
-- 
1.9.1

[PATCH for-4.6 07/13] xl: call libxl_dominfo_{init, dispose} in main_cpupoolnumasplit

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 2d95e35..a4e56ef 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -7660,6 +7660,8 @@ int main_cpupoolnumasplit(int argc, char **argv)
         /* No options */
     }
 
+    libxl_dominfo_init(&info);
+
     rc = 1;
 
     libxl_bitmap_init(&cpumap);
@@ -7768,6 +7770,7 @@ int main_cpupoolnumasplit(int argc, char **argv)
 out:
     libxl_cputopology_list_free(topology, n_cpus);
     libxl_bitmap_dispose(&cpumap);
+    libxl_dominfo_dispose(&info);
     free(name);
 
     return rc;
-- 
1.9.1

[PATCH for-4.6 08/13] xl: call libxl_dominfo_init in main_list

[Wei Liu]

Also change the path that disposes of the buffer to use info_buf to
avoid confusing coverity.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index a4e56ef..79cef7a 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -4843,6 +4843,8 @@ int main_list(int argc, char **argv)
         info_free = info;
     } else if (optind == argc-1) {
         uint32_t domid = find_domain(argv[optind]);
+
+        libxl_dominfo_init(&info_buf);
         rc = libxl_domain_info(ctx, &info_buf, domid);
         if (rc == ERROR_DOMAIN_NOTFOUND) {
             fprintf(stderr, "Error: Domain \'%s\' does not exist.\n",
@@ -4869,7 +4871,7 @@ int main_list(int argc, char **argv)
     if (info_free)
         libxl_dominfo_list_free(info, nb_domain);
     else
-        libxl_dominfo_dispose(info);
+        libxl_dominfo_dispose(&info_buf);
 
     return 0;
 }
-- 
1.9.1

[PATCH for-4.6 09/13] xl: call libxl_bitmap_{init, dispose} in main_cpupoolcreate

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 79cef7a..7e279cd 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -7244,6 +7244,9 @@ int main_cpupoolcreate(int argc, char **argv)
         break;
     }
 
+    libxl_bitmap_init(&freemap);
+    libxl_bitmap_init(&cpumap);
+
     while (optind < argc) {
         if ((p = strchr(argv[optind], '='))) {
             string_realloc_append(&extra_config, "\n");
@@ -7405,6 +7408,8 @@ int main_cpupoolcreate(int argc, char **argv)
 out_cfg:
     xlu_cfg_destroy(config);
 out:
+    libxl_bitmap_dispose(&freemap);
+    libxl_bitmap_dispose(&cpumap);
     free(name);
     free(config_data);
     free(extra_config);
-- 
1.9.1

[PATCH for-4.6 10/13] xl: valid fd can be 0 in main_loadpolicy

[Wei Liu]

Also fixed some style problems while I was there.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 7e279cd..3717568 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -7856,7 +7856,7 @@ int main_loadpolicy(int argc, char **argv)
 
     polFName = argv[optind];
     polFd = open(polFName, O_RDONLY);
-    if ( polFd < 0 ) {
+    if (polFd < 0) {
         fprintf(stderr, "Error occurred opening policy file '%s': %s\n",
                 polFName, strerror(errno));
         ret = -1;
@@ -7864,7 +7864,7 @@ int main_loadpolicy(int argc, char **argv)
     }
 
     ret = stat(polFName, &info);
-    if ( ret < 0 ) {
+    if (ret < 0) {
         fprintf(stderr, "Error occurred retrieving information about"
                 "policy file '%s': %s\n", polFName, strerror(errno));
         goto done;
@@ -7896,7 +7896,7 @@ int main_loadpolicy(int argc, char **argv)
 
 done:
     free(polMemCp);
-    if ( polFd > 0 )
+    if (polFd >= 0)
         close(polFd);
 
     return ret;
-- 
1.9.1

[PATCH for-4.6 11/13] xl: free event struct after use in main_shutdown_or_reboot

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 3717568..9edc0db 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -4766,8 +4766,10 @@ static int main_shutdown_or_reboot(int do_reboot, int argc, char **argv)
                fallback_trigger);
         }
 
-        if (wait_for_it)
+        if (wait_for_it) {
             wait_for_domain_deaths(deathws, nb_domain - 1 /* not dom 0 */);
+            free(deathws);
+        }
 
         libxl_dominfo_list_free(dominfo, nb_domain);
     } else {
-- 
1.9.1

[PATCH for-4.6 12/13] tools/ocaml: call libxl_dominfo_{init, dispose} in stub

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: David Scott <dave.scott@eu.citrix.com>

As far as I can tell, all Val_$foo function does deep-copy, so we can
safely call dispose in said function.
---
 tools/ocaml/libs/xl/xenlight_stubs.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/ocaml/libs/xl/xenlight_stubs.c b/tools/ocaml/libs/xl/xenlight_stubs.c
index 4133527..7b8d6db 100644
--- a/tools/ocaml/libs/xl/xenlight_stubs.c
+++ b/tools/ocaml/libs/xl/xenlight_stubs.c
@@ -979,14 +979,18 @@ value stub_xl_dominfo_get(value ctx, value domid)
 	int ret;
 	uint32_t c_domid = Int_val(domid);
 
+	libxl_dominfo_init(&c_dominfo);
+
 	caml_enter_blocking_section();
 	ret = libxl_domain_info(CTX, &c_dominfo, c_domid);
 	caml_leave_blocking_section();
 
-	if (ret != 0)
+	if (ret != 0) {
 		failwith_xl(ERROR_FAIL, "domain_info");
+		libxl_dominfo_dispose(&c_dominfo);
+	}
 	dominfo = Val_dominfo(&c_dominfo);
-
+	libxl_dominfo_dispose(&c_dominfo);
 	CAMLreturn(dominfo);
 }
 
-- 
1.9.1

[PATCH for-4.6 13/13] tools/ocaml: handle strdup failure in stub_xl_device_disk_of_vdev

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: dave.scott@eu.citrix.com

Please check if the use of caml_failwith is correct.
---
 tools/ocaml/libs/xl/xenlight_stubs.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/ocaml/libs/xl/xenlight_stubs.c b/tools/ocaml/libs/xl/xenlight_stubs.c
index 7b8d6db..dccd7ed 100644
--- a/tools/ocaml/libs/xl/xenlight_stubs.c
+++ b/tools/ocaml/libs/xl/xenlight_stubs.c
@@ -780,6 +780,10 @@ value stub_xl_device_disk_of_vdev(value ctx, value domid, value vdev)
 
 	c_vdev = strdup(String_val(vdev));
 
+	if (!c_vdev) {
+		caml_failwith("Failed to duplicate vdev string.");
+	}
+
 	caml_enter_blocking_section();
 	libxl_vdev_to_device_disk(CTX, c_domid, c_vdev, &c_disk);
 	caml_leave_blocking_section();
-- 
1.9.1

Re: [PATCH for-4.6 12/13] tools/ocaml: call libxl_dominfo_{init, dispose} in stub

[Andrew Cooper]

On 23/07/2015 08:59, Wei Liu wrote:
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> Cc: David Scott <dave.scott@eu.citrix.com>
>
> As far as I can tell, all Val_$foo function does deep-copy, so we can
> safely call dispose in said function.

Sadly this is insufficient.  failwith_xl() longjump()s back into the
ocaml runtime, which ends up leaking any allocations made for dominfo.

This is a systemic problem with the Ocaml bindings and I have a proposed
solution but it involves rewriting quite a lot of this code and is
definitely not 4.6 material.

~Andrew

> ---
>  tools/ocaml/libs/xl/xenlight_stubs.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/tools/ocaml/libs/xl/xenlight_stubs.c b/tools/ocaml/libs/xl/xenlight_stubs.c
> index 4133527..7b8d6db 100644
> --- a/tools/ocaml/libs/xl/xenlight_stubs.c
> +++ b/tools/ocaml/libs/xl/xenlight_stubs.c
> @@ -979,14 +979,18 @@ value stub_xl_dominfo_get(value ctx, value domid)
>  	int ret;
>  	uint32_t c_domid = Int_val(domid);
>  
> +	libxl_dominfo_init(&c_dominfo);
> +
>  	caml_enter_blocking_section();
>  	ret = libxl_domain_info(CTX, &c_dominfo, c_domid);
>  	caml_leave_blocking_section();
>  
> -	if (ret != 0)
> +	if (ret != 0) {
>  		failwith_xl(ERROR_FAIL, "domain_info");
> +		libxl_dominfo_dispose(&c_dominfo);
> +	}
>  	dominfo = Val_dominfo(&c_dominfo);
> -
> +	libxl_dominfo_dispose(&c_dominfo);
>  	CAMLreturn(dominfo);
>  }
>  

Re: [PATCH for-4.6 12/13] tools/ocaml: call libxl_dominfo_{init, dispose} in stub

[Wei Liu]

On Thu, Jul 23, 2015 at 09:32:44AM +0100, Andrew Cooper wrote:
> On 23/07/2015 08:59, Wei Liu wrote:
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> > Cc: David Scott <dave.scott@eu.citrix.com>
> >
> > As far as I can tell, all Val_$foo function does deep-copy, so we can
> > safely call dispose in said function.
> 
> Sadly this is insufficient.  failwith_xl() longjump()s back into the
> ocaml runtime, which ends up leaking any allocations made for dominfo.
> 
> This is a systemic problem with the Ocaml bindings and I have a proposed
> solution but it involves rewriting quite a lot of this code and is
> definitely not 4.6 material.
> 

Fine then. Systematic problem requires systematic fix.  Let's ignore the
last two patches in this series.

Wei.

Re: [PATCH for-4.6 13/13] tools/ocaml: handle strdup failure in stub_xl_device_disk_of_vdev

[Andrew Cooper]

On 23/07/2015 08:59, Wei Liu wrote:
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> Cc: dave.scott@eu.citrix.com
>
> Please check if the use of caml_failwith is correct.

This issue I have a different patch for, which could be 4.6 material, if
it weren't for the same issue as identified in patch 12.

In this case, Ocaml strings may contain embedded NULs which generally
makes the use of  the strxxx() functions incorrect.  OTOH, the first
thing libxl will do is assume that they are NUL terminated, so it
probably isn't too much of a problem.

There is already a dup_String_Val() function which attempts to deal with
this, although it has failure cases from integer overflows.

~Andrew

> ---
>  tools/ocaml/libs/xl/xenlight_stubs.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/tools/ocaml/libs/xl/xenlight_stubs.c b/tools/ocaml/libs/xl/xenlight_stubs.c
> index 7b8d6db..dccd7ed 100644
> --- a/tools/ocaml/libs/xl/xenlight_stubs.c
> +++ b/tools/ocaml/libs/xl/xenlight_stubs.c
> @@ -780,6 +780,10 @@ value stub_xl_device_disk_of_vdev(value ctx, value domid, value vdev)
>  
>  	c_vdev = strdup(String_val(vdev));
>  
> +	if (!c_vdev) {
> +		caml_failwith("Failed to duplicate vdev string.");
> +	}
> +
>  	caml_enter_blocking_section();
>  	libxl_vdev_to_device_disk(CTX, c_domid, c_vdev, &c_disk);
>  	caml_leave_blocking_section();

Re: [PATCH for-4.6 13/13] tools/ocaml: handle strdup failure in stub_xl_device_disk_of_vdev

[Dave Scott]

> On 23 Jul 2015, at 08:59, Wei Liu <wei.liu2@citrix.com> wrote:
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> Cc: dave.scott@eu.citrix.com
> 
> Please check if the use of caml_failwith is correct.

This looks fine to me.

Acked-by: David Scott <dave.scott@citrix.com>

> ---
> tools/ocaml/libs/xl/xenlight_stubs.c | 4 ++++
> 1 file changed, 4 insertions(+)
> 
> diff --git a/tools/ocaml/libs/xl/xenlight_stubs.c b/tools/ocaml/libs/xl/xenlight_stubs.c
> index 7b8d6db..dccd7ed 100644
> --- a/tools/ocaml/libs/xl/xenlight_stubs.c
> +++ b/tools/ocaml/libs/xl/xenlight_stubs.c
> @@ -780,6 +780,10 @@ value stub_xl_device_disk_of_vdev(value ctx, value domid, value vdev)
> 
> 	c_vdev = strdup(String_val(vdev));
> 
> +	if (!c_vdev) {
> +		caml_failwith("Failed to duplicate vdev string.");
> +	}
> +
> 	caml_enter_blocking_section();
> 	libxl_vdev_to_device_disk(CTX, c_domid, c_vdev, &c_disk);
> 	caml_leave_blocking_section();
> -- 
> 1.9.1
> 

Re: [PATCH for-4.6 12/13] tools/ocaml: call libxl_dominfo_{init, dispose} in stub

[Andrew Cooper]

On 23/07/15 09:38, Wei Liu wrote:
> On Thu, Jul 23, 2015 at 09:32:44AM +0100, Andrew Cooper wrote:
>> On 23/07/2015 08:59, Wei Liu wrote:
>>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>>> ---
>>> Cc: David Scott <dave.scott@eu.citrix.com>
>>>
>>> As far as I can tell, all Val_$foo function does deep-copy, so we can
>>> safely call dispose in said function.
>> Sadly this is insufficient.  failwith_xl() longjump()s back into the
>> ocaml runtime, which ends up leaking any allocations made for dominfo.
>>
>> This is a systemic problem with the Ocaml bindings and I have a proposed
>> solution but it involves rewriting quite a lot of this code and is
>> definitely not 4.6 material.
>>
> Fine then. Systematic problem requires systematic fix.  Let's ignore the
> last two patches in this series.

Agreed.

What I not yet figured out how to fix is libxl__alloc_failed().

_exit() is not a reasonable out-of-memory action in a library,
especially not in use in a runtime which actually has a garbage collector.

~Andrew

Re: [PATCH for-4.6 13/13] tools/ocaml: handle strdup failure in stub_xl_device_disk_of_vdev

[Dave Scott]

> On 23 Jul 2015, at 09:38, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> 
> On 23/07/2015 08:59, Wei Liu wrote:
>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>> ---
>> Cc: dave.scott@eu.citrix.com
>> 
>> Please check if the use of caml_failwith is correct.
> 
> This issue I have a different patch for, which could be 4.6 material, if
> it weren't for the same issue as identified in patch 12.
> 
> In this case, Ocaml strings may contain embedded NULs which generally
> makes the use of  the strxxx() functions incorrect.  OTOH, the first
> thing libxl will do is assume that they are NUL terminated, so it
> probably isn't too much of a problem.

Yeah I would assume if my OCaml program sent libxl a legal OCaml string containing NULLs, that libxl would end up truncating it at the first NULL. I think it would be ok to document this at the OCaml level.

Cheers,
Dave

> 
> There is already a dup_String_Val() function which attempts to deal with
> this, although it has failure cases from integer overflows.
> 
> ~Andrew
> 
>> ---
>> tools/ocaml/libs/xl/xenlight_stubs.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>> 
>> diff --git a/tools/ocaml/libs/xl/xenlight_stubs.c b/tools/ocaml/libs/xl/xenlight_stubs.c
>> index 7b8d6db..dccd7ed 100644
>> --- a/tools/ocaml/libs/xl/xenlight_stubs.c
>> +++ b/tools/ocaml/libs/xl/xenlight_stubs.c
>> @@ -780,6 +780,10 @@ value stub_xl_device_disk_of_vdev(value ctx, value domid, value vdev)
>> 
>> 	c_vdev = strdup(String_val(vdev));
>> 
>> +	if (!c_vdev) {
>> +		caml_failwith("Failed to duplicate vdev string.");
>> +	}
>> +
>> 	caml_enter_blocking_section();
>> 	libxl_vdev_to_device_disk(CTX, c_domid, c_vdev, &c_disk);
>> 	caml_leave_blocking_section();
> 

Re: [PATCH for-4.6 01/13] libxl: use thread-safe localtime_t and handle NULL

[Ian Campbell]

On Thu, 2015-07-23 at 08:59 +0100, Wei Liu wrote:
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH for-4.6 02/13] libxl: properly clean up array in libxl_list_cpupool failure path

[Ian Campbell]

On Thu, 2015-07-23 at 08:59 +0100, Wei Liu wrote:
> Document how cpupool_info works.  Distinguish success (ERROR_FAIL +
> ENOENT) vs failure in libxl_list_cpupool and properly clean up the 
> array
> in failure path.
> 
> Also switch to libxl__realloc and call libxl_cpupool_{init,dispose}
> where appropriate.
> 
> There is change of behaviour. Previously if memory allocation fails 
> the
> said function returns NULL. Now memory allocation failure is fatal. 
> This
> is in line with how we deal with memory allocation failure in other
> places in libxl though.

I think this function would benefit from making the out: label be the
error path and the success case just a return ptr (just before the
label).

Then your error handling for cpupool_info would become
            libxl_cpupoolinfo_dispose(&info);
            if (errno != ENOENT) goto out;
            break;

and the "if (failed)" block would be at the out label (without the
iff).

Such splitting of the success/failure case is allowed if the only
shared code would be the GC_FREE, which is the case here.

Ian.

Re: [PATCH for-4.6 03/13] xl/libxl: remove a bunch of pointless assignments

[Ian Campbell]

On Thu, 2015-07-23 at 08:59 +0100, Wei Liu wrote:
> Those values are  overwritten before they can be of any use.

I commented on a similar patch recently (having acked it):
    The flip side is that if the code uses the "init everything and
    goto out on error" idiom then the init might need to be put back if
    the code changes, hopefully even a regular compiler would spot this
    though.

The rc = ERROR_FAIL one is consistent with coding style.

coding style does say that things which might need cleaning up should
be initialised as they are declared, however I don't think any of the
pid/ret's here need cleanup. So:

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH for-4.6 01/13] libxl: use thread-safe localtime_t and handle NULL

[Wei Liu]

On Thu, Jul 23, 2015 at 10:14:43AM +0100, Ian Campbell wrote:
> On Thu, 2015-07-23 at 08:59 +0100, Wei Liu wrote:
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

I notice that I fat-fingered localtime_r as localtime_t in subject line.
Ironically I pointed out the same error your reply to a previous
version of this patch...  :-/

Wei.

Re: [PATCH for-4.6 04/13] xl: free pid string in do_daemonize

[Ian Campbell]

On Thu, 2015-07-23 at 08:59 +0100, Wei Liu wrote:
> Pid is a null terminated string allocated by asprintf. It should be
> freed after use.
> 
> Also fixed a coding style problem while I was there.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH for-4.6 05/13] xl: check json string is not null before printing in create_domain

[Ian Campbell]

On Thu, 2015-07-23 at 08:59 +0100, Wei Liu wrote:
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH for-4.6 06/13] xl: call libxl_dominfo_{init, dispose} in psr_cmt_show

[Ian Campbell]

On Thu, 2015-07-23 at 08:59 +0100, Wei Liu wrote:
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH for-4.6 07/13] xl: call libxl_dominfo_{init, dispose} in main_cpupoolnumasplit

[Ian Campbell]

On Thu, 2015-07-23 at 08:59 +0100, Wei Liu wrote:
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

info here seems to be used in a loop, without any init or free. I think
the init should be moved there, and the looping case needs a dispose,
with only the final one being left until the end.

(that loop looks a bit odd to me, but that's independent...)

Re: [PATCH for-4.6 08/13] xl: call libxl_dominfo_init in main_list

[Ian Campbell]

On Thu, 2015-07-23 at 08:59 +0100, Wei Liu wrote:
> Also change the path that disposes of the buffer to use info_buf to
> avoid confusing coverity.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

But would it be less confusing to both human and machine readers if
info_buf was always init'd and dispose'd unconditionally at the head
and tail of the function? i.e. ignoring the fact that in one code path
we use a different info?


> ---
>  tools/libxl/xl_cmdimpl.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index a4e56ef..79cef7a 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -4843,6 +4843,8 @@ int main_list(int argc, char **argv)
>          info_free = info;
>      } else if (optind == argc-1) {
>          uint32_t domid = find_domain(argv[optind]);
> +
> +        libxl_dominfo_init(&info_buf);
>          rc = libxl_domain_info(ctx, &info_buf, domid);
>          if (rc == ERROR_DOMAIN_NOTFOUND) {
>              fprintf(stderr, "Error: Domain \'%s\' does not 
> exist.\n",
> @@ -4869,7 +4871,7 @@ int main_list(int argc, char **argv)
>      if (info_free)
>          libxl_dominfo_list_free(info, nb_domain);
>      else
> -        libxl_dominfo_dispose(info);
> +        libxl_dominfo_dispose(&info_buf);
>  
>      return 0;
>  }

Re: [PATCH for-4.6 09/13] xl: call libxl_bitmap_{init, dispose} in main_cpupoolcreate

[Ian Campbell]

On Thu, 2015-07-23 at 08:59 +0100, Wei Liu wrote:
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH for-4.6 10/13] xl: valid fd can be 0 in main_loadpolicy

[Ian Campbell]

On Thu, 2015-07-23 at 08:59 +0100, Wei Liu wrote:
> Also fixed some style problems while I was there.

polFd is initialised to 0, I think it should be initialised to -1 as
well as your change here, otherwise you can unexpectedly close stdin on
some error paths.

(also: studly caps, yuk, not your fault I expect)

> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxl/xl_cmdimpl.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 7e279cd..3717568 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -7856,7 +7856,7 @@ int main_loadpolicy(int argc, char **argv)
>  
>      polFName = argv[optind];
>      polFd = open(polFName, O_RDONLY);
> -    if ( polFd < 0 ) {
> +    if (polFd < 0) {
>          fprintf(stderr, "Error occurred opening policy file '%s': 
> %s\n",
>                  polFName, strerror(errno));
>          ret = -1;
> @@ -7864,7 +7864,7 @@ int main_loadpolicy(int argc, char **argv)
>      }
>  
>      ret = stat(polFName, &info);
> -    if ( ret < 0 ) {
> +    if (ret < 0) {
>          fprintf(stderr, "Error occurred retrieving information 
> about"
>                  "policy file '%s': %s\n", polFName, 
> strerror(errno));
>          goto done;
> @@ -7896,7 +7896,7 @@ int main_loadpolicy(int argc, char **argv)
>  
>  done:
>      free(polMemCp);
> -    if ( polFd > 0 )
> +    if (polFd >= 0)
>          close(polFd);
>  
>      return ret;

Re: [PATCH for-4.6 11/13] xl: free event struct after use in main_shutdown_or_reboot

[Ian Campbell]

On Thu, 2015-07-23 at 08:59 +0100, Wei Liu wrote:
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

It appears there is no libxl_evgen_domain_death_dispose to call on the
individual members of the array. Ah, because it is opaque.

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Although since it is init'd to NULL you could also call free
unconditionally.

> ---
>  tools/libxl/xl_cmdimpl.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 3717568..9edc0db 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -4766,8 +4766,10 @@ static int main_shutdown_or_reboot(int 
> do_reboot, int argc, char **argv)
>                 fallback_trigger);
>          }
>  
> -        if (wait_for_it)
> +        if (wait_for_it) {
>              wait_for_domain_deaths(deathws, nb_domain - 1 /* not dom 
> 0 */);
> +            free(deathws);
> +        }
>  
>          libxl_dominfo_list_free(dominfo, nb_domain);
>      } else {

Re: [PATCH for-4.6 12/13] tools/ocaml: call libxl_dominfo_{init, dispose} in stub

[Ian Campbell]

On Thu, 2015-07-23 at 09:32 +0100, Andrew Cooper wrote:
> On 23/07/2015 08:59, Wei Liu wrote:
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> > Cc: David Scott <dave.scott@eu.citrix.com>
> > 
> > As far as I can tell, all Val_$foo function does deep-copy, so we 
> > can
> > safely call dispose in said function.
> 
> Sadly this is insufficient.  failwith_xl() longjump()s back into the
> ocaml runtime, which ends up leaking any allocations made for 
> dominfo.
> 
> This is a systemic problem with the Ocaml bindings and I have a 
> proposed
> solution but it involves rewriting quite a lot of this code and is
> definitely not 4.6 material.

Is it not sufficient to treat failwith_xl as a longjump statement (or
any sort of "return-y" thing), which would simply necessitate doing the
cleanup before calling it?

Perhaps Coverity could model it as such and would therefore warn about
the dead code being added here?

Re: [PATCH for-4.6 12/13] tools/ocaml: call libxl_dominfo_{init, dispose} in stub

[Andrew Cooper]

On 23/07/15 10:55, Ian Campbell wrote:
> On Thu, 2015-07-23 at 09:32 +0100, Andrew Cooper wrote:
>> On 23/07/2015 08:59, Wei Liu wrote:
>>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>>> ---
>>> Cc: David Scott <dave.scott@eu.citrix.com>
>>>
>>> As far as I can tell, all Val_$foo function does deep-copy, so we 
>>> can
>>> safely call dispose in said function.
>> Sadly this is insufficient.  failwith_xl() longjump()s back into the
>> ocaml runtime, which ends up leaking any allocations made for 
>> dominfo.
>>
>> This is a systemic problem with the Ocaml bindings and I have a 
>> proposed
>> solution but it involves rewriting quite a lot of this code and is
>> definitely not 4.6 material.
> Is it not sufficient to treat failwith_xl as a longjump statement (or
> any sort of "return-y" thing), which would simply necessitate doing the
> cleanup before calling it?
>
> Perhaps Coverity could model it as such and would therefore warn about
> the dead code being added here?
>

Part of my Ocaml series is to properly mark failwith_xl() as a Noreturn
function.  Currently as far as the compiler and Coverity can tell,
failwith_xl() may return normally.

While it is possible to rearrange this code to avoid leaking in the ret
!= 0 case, it is not possible to rearrange it to avoid leaking if
Val_dominfo() uses failwith_xl()/caml_out_of_memory() itself.

The solution I have in mind is to wrap all libxl IDL objects in Ocaml
Custom blocks, which allows the Ocaml runtime to garbage collect them.

~Andrew

Re: [PATCH for-4.6 07/13] xl: call libxl_dominfo_{init, dispose} in main_cpupoolnumasplit

[Wei Liu]

On Thu, Jul 23, 2015 at 10:41:40AM +0100, Ian Campbell wrote:
> On Thu, 2015-07-23 at 08:59 +0100, Wei Liu wrote:
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> info here seems to be used in a loop, without any init or free. I think
> the init should be moved there, and the looping case needs a dispose,
> with only the final one being left until the end.
> 

Indeed, I will look into fixing that.

> (that loop looks a bit odd to me, but that's independent...)

Re: [PATCH for-4.6 02/13] libxl: properly clean up array in libxl_list_cpupool failure path

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1271 bytes --]

On Thu, 2015-07-23 at 10:22 +0100, Ian Campbell wrote:
> On Thu, 2015-07-23 at 08:59 +0100, Wei Liu wrote:

> > There is change of behaviour. Previously if memory allocation fails 
> > the
> > said function returns NULL. Now memory allocation failure is fatal. 
> > This
> > is in line with how we deal with memory allocation failure in other
> > places in libxl though.
> 
> I think this function would benefit from making the out: label be the
> error path and the success case just a return ptr (just before the
> label).
> 
> Then your error handling for cpupool_info would become
>             libxl_cpupoolinfo_dispose(&info);
>             if (errno != ENOENT) goto out;
>             break;
> 
> and the "if (failed)" block would be at the out label (without the
> iff).
> 
I was about to give my Reviewed-by, but yes, I like Ian's suggestion
too.

In any case, apart from how error handling is done, I think this patch
is fine, as far as dealing with cpupools goes.

Dario
-- 
<<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)

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH for-4.6 00/13] tools: fixes inspired by Coverity scan

[Ian Campbell]

On Thu, 2015-07-23 at 08:59 +0100, Wei Liu wrote:
> Wei Liu (13):
>   libxl: properly clean up array in libxl_list_cpupool failure path
>   xl: call libxl_dominfo_{init,dispose} in main_cpupoolnumasplit
>   xl: call libxl_dominfo_init in main_list
>   xl: valid fd can be 0 in main_loadpolicy
>   tools/ocaml: call libxl_dominfo_{init, dispose} in stub
>   tools/ocaml: handle strdup failure in stub_xl_device_disk_of_vdev

I applied all but these.

I fixed localtime_r in the subject of #1.

Ian.