[PATCH v2 00/10] xl/libxl: fix issues discovered by coverity

[PATCH v2 00/10] xl/libxl: fix issues discovered by coverity

[Wei Liu]

Wei Liu (10):
  libxl: make libxl__abs_path correctly handle NULL argument
  libxl: turn two malloc's to libxl__malloc
  libxl: json string object can be NULL
  libxl: dispose dominfo to avoid leaking resource
  libxl: localtime(3) can return NULL
  libxl: qmp_init_handler can return NULL
  xl: correct handling of extra_config in main_cpupoolcreate
  xl: correctly handle null extra config in main_config_update
  xl: fix a typo in error string in create_domain
  libxl: fix caller of libxl_cpupool functions

 tools/libxl/libxl.c          |  6 +++++-
 tools/libxl/libxl_aoutils.c  |  3 +--
 tools/libxl/libxl_device.c   |  2 ++
 tools/libxl/libxl_dm.c       |  2 +-
 tools/libxl/libxl_dom.c      |  5 +++--
 tools/libxl/libxl_internal.c |  4 ++--
 tools/libxl/libxl_json.c     |  9 +++++++--
 tools/libxl/libxl_qmp.c      |  1 +
 tools/libxl/libxl_x86.c      |  6 ++++++
 tools/libxl/xl_cmdimpl.c     | 11 ++++++-----
 10 files changed, 34 insertions(+), 15 deletions(-)

-- 
1.9.1

[PATCH v2 01/10] libxl: make libxl__abs_path correctly handle NULL argument

[Wei Liu]

If s is NULL, just return NULL to avoid libxl__strdup dereferencing NULL
pointer.

Coverity-ID: 1198722

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_internal.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 42d548e..6402c1b 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -233,8 +233,8 @@ void libxl__log(libxl_ctx *ctx, xentoollog_level msglevel, int errnoval,
 
 char *libxl__abs_path(libxl__gc *gc, const char *s, const char *path)
 {
-    if (!s || s[0] == '/')
-        return libxl__strdup(gc, s);
+    if (!s) return NULL;
+    if (s[0] == '/') return libxl__strdup(gc, s);
     return libxl__sprintf(gc, "%s/%s", path, s);
 }
 
-- 
1.9.1

[PATCH v2 02/10] libxl: turn two malloc's to libxl__malloc

[Wei Liu]

One is to combine malloc + libxl__alloc_failed. The other is to avoid
dereferencing NULL pointer in case of malloc failure.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_aoutils.c | 3 +--
 tools/libxl/libxl_dm.c      | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
index 0931eee..0300396 100644
--- a/tools/libxl/libxl_aoutils.c
+++ b/tools/libxl/libxl_aoutils.c
@@ -245,8 +245,7 @@ static void datacopier_readable(libxl__egc *egc, libxl__ev_fd *ev,
 
             buf = LIBXL_TAILQ_LAST(&dc->bufs, libxl__datacopier_bufs);
             if (!buf || buf->used >= sizeof(buf->buf)) {
-                buf = malloc(sizeof(*buf));
-                if (!buf) libxl__alloc_failed(CTX, __func__, 1, sizeof(*buf));
+                buf = libxl__malloc(NOGC, sizeof(*buf));
                 buf->used = 0;
                 LIBXL_TAILQ_INSERT_TAIL(&dc->bufs, buf, entry);
             }
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index ad434f0..0cc73be 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1010,7 +1010,7 @@ static int libxl__write_stub_dmargs(libxl__gc *gc,
         i++;
     }
     dmargs_size++;
-    dmargs = (char *) malloc(dmargs_size);
+    dmargs = (char *) libxl__malloc(NOGC, dmargs_size);
     i = 1;
     dmargs[0] = '\0';
     while (args[i] != NULL) {
-- 
1.9.1

Re: [PATCH v2 02/10] libxl: turn two malloc's to libxl__malloc

[Ian Jackson]

Wei Liu writes ("[PATCH v2 02/10] libxl: turn two malloc's to libxl__malloc"):
> One is to combine malloc + libxl__alloc_failed. The other is to avoid
> dereferencing NULL pointer in case of malloc failure.

The former fix is simply corredt.

> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index ad434f0..0cc73be 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -1010,7 +1010,7 @@ static int libxl__write_stub_dmargs(libxl__gc *gc,
>          i++;
>      }
>      dmargs_size++;
> -    dmargs = (char *) malloc(dmargs_size);
> +    dmargs = (char *) libxl__malloc(NOGC, dmargs_size);

This latter should IMO be fixed by using the gc, as well.

Ian.

[PATCH v2 03/10] libxl: json string object can be NULL

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_json.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
index 346929a..652b3f4 100644
--- a/tools/libxl/libxl_json.c
+++ b/tools/libxl/libxl_json.c
@@ -433,8 +433,13 @@ int libxl__string_parse_json(libxl__gc *gc, const libxl__json_object *o,
 
     if (libxl__json_object_is_null(o))
         *p = NULL;
-    else
-        *p = libxl__strdup(NOGC, libxl__json_object_get_string(o));
+    else {
+        const char *s = libxl__json_object_get_string(o);
+        if (!s)
+            *p = NULL;
+        else
+            *p = libxl__strdup(NOGC, s);
+    }
 
     return 0;
 }
-- 
1.9.1

[PATCH v2 04/10] libxl: dispose dominfo to avoid leaking resource

[Wei Liu]

Add libxl_dominfo_dispose to one return path that doesn't have it.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_device.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 2493972..3f8b555 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -816,6 +816,8 @@ void libxl__initiate_device_remove(libxl__egc *egc,
                            be_path);
                 goto out;
             }
+
+            libxl_dominfo_dispose(&info);
             return;
         }
     }
-- 
1.9.1

[PATCH v2 05/10] libxl: localtime(3) can return NULL

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_x86.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
index 8cd15ca..6be9b1b 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -311,6 +311,11 @@ int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
         t = time(NULL);
         tm = localtime(&t);
 
+        if (!tm) {
+            ret = ERROR_FAIL;
+            goto out;
+        }
+
         rtc_timeoffset += tm->tm_gmtoff;
     }
 
@@ -335,6 +340,7 @@ int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
         }
     }
 
+out:
     return ret;
 }
 
-- 
1.9.1

[PATCH v2 06/10] libxl: qmp_init_handler can return NULL

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_qmp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 6484f5e..965c507 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -694,6 +694,7 @@ libxl__qmp_handler *libxl__qmp_initialize(libxl__gc *gc, uint32_t domid)
     char *qmp_socket;
 
     qmp = qmp_init_handler(gc, domid);
+    if (!qmp) return NULL;
 
     qmp_socket = GCSPRINTF("%s/qmp-libxl-%d", libxl__run_dir_path(), domid);
     if ((ret = qmp_open(qmp, qmp_socket, QMP_SOCKET_CONNECT_TIMEOUT)) < 0) {
-- 
1.9.1

[PATCH v2 07/10] xl: correct handling of extra_config in main_cpupoolcreate

[Wei Liu]

Don't dereference extra_config if it's NULL. Don't leak extra_config in
the end.

Also fixed a typo in error string while I was there.

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

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 971209c..607db4e 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -7228,9 +7228,9 @@ int main_cpupoolcreate(int argc, char **argv)
     else
         config_src="command line";
 
-    if (strlen(extra_config)) {
+    if (extra_config && strlen(extra_config)) {
         if (config_len > INT_MAX - (strlen(extra_config) + 2)) {
-            fprintf(stderr, "Failed to attach extra configration\n");
+            fprintf(stderr, "Failed to attach extra configuration\n");
             goto out;
         }
         config_data = xrealloc(config_data,
@@ -7365,6 +7365,7 @@ out_cfg:
 out:
     free(name);
     free(config_data);
+    free(extra_config);
     return rc;
 }
 
-- 
1.9.1

[PATCH v2 08/10] xl: correctly handle null extra config in main_config_update

[Wei Liu]

Don't dereference NULL.

Also fixed a typo in error string while I was there.

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

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 607db4e..888f729 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5010,9 +5010,9 @@ int main_config_update(int argc, char **argv)
         if (rc) { fprintf(stderr, "Failed to read config file: %s: %s\n",
                            filename, strerror(errno));
                   free(extra_config); return ERROR_FAIL; }
-        if (strlen(extra_config)) {
+        if (extra_config && strlen(extra_config)) {
             if (config_len > INT_MAX - (strlen(extra_config) + 2 + 1)) {
-                fprintf(stderr, "Failed to attach extra configration\n");
+                fprintf(stderr, "Failed to attach extra configuration\n");
                 exit(1);
             }
             /* allocate space for the extra config plus two EOLs plus \0 */
-- 
1.9.1

[PATCH v2 09/10] xl: fix a typo in error string in create_domain

[Wei Liu]

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

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 888f729..8b4be72 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -2658,7 +2658,7 @@ static uint32_t create_domain(struct domain_create *dom_info)
         }
         if (!restoring && extra_config && strlen(extra_config)) {
             if (config_len > INT_MAX - (strlen(extra_config) + 2 + 1)) {
-                fprintf(stderr, "Failed to attach extra configration\n");
+                fprintf(stderr, "Failed to attach extra configuration\n");
                 return ERROR_FAIL;
             }
             /* allocate space for the extra config plus two EOLs plus \0 */
-- 
1.9.1

[PATCH v2 10/10] libxl: fix caller of libxl_cpupool functions

[Wei Liu]

Coverity complains cpupool_info leaks a string in failure path. Instead
of fixing that path, we rely on the callers (two public APIs at the
moment) of cpupool_info correctly call libxl_cpupoolinfo_dispose in
their failure path to dispose of any resources.

That involves:
1. Call _init and _dispose in libxl_list_cpupool
2. Call _init in numa_place_domain

Fix numa_place_domain to use goto style to make things more clearer.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Dario Faggioli <dario.faggioli@citrix.com>
---
 tools/libxl/libxl.c     | 6 +++++-
 tools/libxl/libxl_dom.c | 5 +++--
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 38aff8d..02b4a26 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -771,8 +771,11 @@ 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)) {
+            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");
@@ -783,6 +786,7 @@ libxl_cpupoolinfo * libxl_list_cpupool(libxl_ctx *ctx, int *nb_pool_out)
         ptr = tmp;
         ptr[i] = info;
         poolid = info.poolid + 1;
+        /* Don't dispose of info. It needs to be returned to caller. */
     }
 
     *nb_pool_out = i;
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 8642192..26e7c67 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -142,6 +142,7 @@ static int numa_place_domain(libxl__gc *gc, uint32_t domid,
 
     libxl__numa_candidate_init(&candidate);
     libxl_bitmap_init(&cpupool_nodemap);
+    libxl_cpupoolinfo_init(&cpupool_info);
 
     /*
      * Extract the cpumap from the cpupool the domain belong to. In fact,
@@ -150,10 +151,10 @@ static int numa_place_domain(libxl__gc *gc, uint32_t domid,
      */
     rc = cpupool = libxl__domain_cpupool(gc, domid);
     if (rc < 0)
-        return rc;
+        goto out;
     rc = libxl_cpupool_info(CTX, &cpupool_info, cpupool);
     if (rc)
-        return rc;
+        goto out;
 
     rc = libxl_domain_need_memory(CTX, info, &memkb);
     if (rc)
-- 
1.9.1

Re: [PATCH v2 10/10] libxl: fix caller of libxl_cpupool functions

[Dario Faggioli]

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

On Mon, 2015-07-13 at 17:22 +0100, Wei Liu wrote:
> Coverity complains cpupool_info leaks a string in failure path. Instead
> of fixing that path, we rely on the callers (two public APIs at the
> moment) of cpupool_info correctly call libxl_cpupoolinfo_dispose in
> their failure path to dispose of any resources.
> 
> That involves:
> 1. Call _init and _dispose in libxl_list_cpupool
> 2. Call _init in numa_place_domain
> 
> Fix numa_place_domain to use goto style to make things more clearer.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

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