[PATCH 0/9] xl/libxl: fix issues discovered by Coverity scan

[PATCH 0/9] xl/libxl: fix issues discovered by Coverity scan

[Wei Liu]

Wei Liu (9):
  libxl: fix libxl__abs_path
  libxl: turn two malloc's to libxl__malloc
  libxl: json string object can be NULL
  libxl: dispose dominfo to avoid leaking resource
  libxl: avoid leaking string in cpupool_info
  libxl: localtime(3) can return NULL
  libxl: qmp_init_handler can return NULL
  xl: fix main_cpupoolcreate
  xl: fix main_config_update

 tools/libxl/libxl.c          | 4 +++-
 tools/libxl/libxl_aoutils.c  | 3 +--
 tools/libxl/libxl_device.c   | 2 ++
 tools/libxl/libxl_dm.c       | 2 +-
 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     | 5 +++--
 9 files changed, 26 insertions(+), 10 deletions(-)

-- 
1.9.1

[PATCH 1/9] libxl: fix libxl__abs_path

[Wei Liu]

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

Signed-off-by: Wei Liu <wei.liu2@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 2/9] 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>
---
 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

[PATCH 3/9] libxl: json string object can be NULL

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@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 4/9] 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>
---
 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 5/9] libxl: avoid leaking string in cpupool_info

[Wei Liu]

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

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 38aff8d..4151dcb 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -740,8 +740,10 @@ static int cpupool_info(libxl__gc *gc,
     info->sched = xcinfo->sched_id;
     info->n_dom = xcinfo->n_dom;
     rc = libxl_cpu_bitmap_alloc(CTX, &info->cpumap, 0);
-    if (rc)
+    if (rc) {
+        free(info->pool_name);
         goto out;
+    }
 
     memcpy(info->cpumap.map, xcinfo->cpumap, info->cpumap.size);
 
-- 
1.9.1

[PATCH 6/9] libxl: localtime(3) can return NULL

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@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 7/9] libxl: qmp_init_handler can return NULL

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@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 8/9] xl: fix main_cpupoolcreate

[Wei Liu]

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

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

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 971209c..d44eb4b 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -7228,7 +7228,7 @@ 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");
             goto out;
@@ -7365,6 +7365,7 @@ out_cfg:
 out:
     free(name);
     free(config_data);
+    free(extra_config);
     return rc;
 }
 
-- 
1.9.1

[PATCH 9/9] xl: fix main_config_update

[Wei Liu]

Don't dereference NULL.

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 d44eb4b..631dbd1 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5010,7 +5010,7 @@ 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");
                 exit(1);
-- 
1.9.1

Re: [PATCH 0/9] xl/libxl: fix issues discovered by Coverity scan

[Andrew Cooper]

On 10/07/15 19:00, Wei Liu wrote:
> Wei Liu (9):
>   libxl: fix libxl__abs_path
>   libxl: turn two malloc's to libxl__malloc
>   libxl: json string object can be NULL
>   libxl: dispose dominfo to avoid leaking resource
>   libxl: avoid leaking string in cpupool_info
>   libxl: localtime(3) can return NULL
>   libxl: qmp_init_handler can return NULL
>   xl: fix main_cpupoolcreate
>   xl: fix main_config_update

Correlating with Coverity Scan, the only defect in common is

1/9 - Coverity-ID: 1198722

All other defects are only found in XenServers instance (we do have all
the knobs turned up to 11).

~Andrew

Re: [PATCH 1/9] libxl: fix libxl__abs_path

[Ian Campbell]

On Fri, 2015-07-10 at 19:00 +0100, Wei Liu wrote:

I rather dislike subjects of the form "fix $function", since it gives
very little clue to someone reading the shortlog what is going on.

In this case I think "libxl: make libxl__abs_path correctly handle a
NULL argument" would be an accurate description.

> If s is NULL, just return NULL to avoid libxl__strdup dereferencing NULL
> pointer.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

For the change itself:
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);
>  }
>  

Re: [PATCH 2/9] libxl: turn two malloc's to libxl__malloc

[Ian Campbell]

On Fri, 2015-07-10 at 19:00 +0100, Wei Liu wrote:
> One is to combine malloc + libxl__alloc_failed. The other is to avoid
> dereferencing NULL pointer in case of malloc failure.

The non-use of a gc for the latter in particular looks a bit suspicious
to me, but nonetheless this is an improvement:

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

Re: [PATCH 1/9] libxl: fix libxl__abs_path

[Wei Liu]

On Mon, Jul 13, 2015 at 10:57:32AM +0100, Ian Campbell wrote:
> On Fri, 2015-07-10 at 19:00 +0100, Wei Liu wrote:
> 
> I rather dislike subjects of the form "fix $function", since it gives
> very little clue to someone reading the shortlog what is going on.
> 
> In this case I think "libxl: make libxl__abs_path correctly handle a
> NULL argument" would be an accurate description.

Ack.

I can resend the whole series after updating subject for patch #1, #8
and #9.

Wei.

Re: [PATCH 3/9] libxl: json string object can be NULL

[Ian Campbell]

On Fri, 2015-07-10 at 19:00 +0100, Wei Liu wrote:
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

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

It occurs to me that since libxl__strdup can never return NULL due to
allocation failure we could consider make libxl__strdup(gc, NULL) be
well defined as returning NULL.

> ---
>  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;
>  }

Re: [PATCH 4/9] libxl: dispose dominfo to avoid leaking resource

[Ian Campbell]

On Fri, 2015-07-10 at 19:00 +0100, Wei Liu wrote:
> Add libxl_dominfo_dispose to one return path that doesn't have it.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

That return is a bit at odds with the generally correct error handling
in that function, but this improves things at least a little and I can
sort of see why this a slightly special case, so:

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;
>          }
>      }

Re: [PATCH 5/9] libxl: avoid leaking string in cpupool_info

[Ian Campbell]

On Fri, 2015-07-10 at 19:00 +0100, Wei Liu wrote:
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Would an "if (rc) libxl_cpupoolinfo_dispose(info)" on the exit path be a
more robust alternative? Might require the addition of a
libxl_cpupoolinfo_init() somewhere before any possible error.

> ---
>  tools/libxl/libxl.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 38aff8d..4151dcb 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -740,8 +740,10 @@ static int cpupool_info(libxl__gc *gc,
>      info->sched = xcinfo->sched_id;
>      info->n_dom = xcinfo->n_dom;
>      rc = libxl_cpu_bitmap_alloc(CTX, &info->cpumap, 0);
> -    if (rc)
> +    if (rc) {
> +        free(info->pool_name);
>          goto out;
> +    }
>  
>      memcpy(info->cpumap.map, xcinfo->cpumap, info->cpumap.size);
>  

Re: [PATCH 6/9] libxl: localtime(3) can return NULL

[Ian Campbell]

On Fri, 2015-07-10 at 19:00 +0100, Wei Liu wrote:
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH 7/9] libxl: qmp_init_handler can return NULL

[Ian Campbell]

On Fri, 2015-07-10 at 19:00 +0100, Wei Liu wrote:
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

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

(although the only actual reason for a failure today is a memory
allocation failure, which ought to abort really).

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

Re: [PATCH 8/9] xl: fix main_cpupoolcreate

[Ian Campbell]

On Fri, 2015-07-10 at 19:00 +0100, Wei Liu wrote:
> Don't dereference extra_config if it's NULL. Don't leak extra_config in
> the end.

Subject should be more descriptive. "xl: correct handling of
extra_config in main_cpupoolcreate" perhaps? (It's a lot easier to write
non-vague messages for patches which only do one thing)

> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxl/xl_cmdimpl.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 971209c..d44eb4b 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -7228,7 +7228,7 @@ 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");

There's a typo in this line of context...

>              goto out;
> @@ -7365,6 +7365,7 @@ out_cfg:
>  out:
>      free(name);
>      free(config_data);
> +    free(extra_config);
>      return rc;
>  }
>  

Re: [PATCH 9/9] xl: fix main_config_update

[Ian Campbell]

On Fri, 2015-07-10 at 19:00 +0100, Wei Liu wrote:
> Don't dereference NULL.

Subject: xl: correctly handle null extra config in main_config_update

> 
> 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 d44eb4b..631dbd1 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -5010,7 +5010,7 @@ 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");
>                  exit(1);

Re: [PATCH 2/9] libxl: turn two malloc's to libxl__malloc

[Wei Liu]

On Mon, Jul 13, 2015 at 11:00:15AM +0100, Ian Campbell wrote:
> On Fri, 2015-07-10 at 19:00 +0100, Wei Liu wrote:
> > One is to combine malloc + libxl__alloc_failed. The other is to avoid
> > dereferencing NULL pointer in case of malloc failure.
> 
> The non-use of a gc for the latter in particular looks a bit suspicious
> to me, but nonetheless this is an improvement:
> 

There is a free() later in this function. I wanted to make this patch
minimum so I didn't switch to using gc and  delete that free.

> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Thanks.

Wei.

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

Re: [PATCH 5/9] libxl: avoid leaking string in cpupool_info

[Wei Liu]

On Mon, Jul 13, 2015 at 11:07:31AM +0100, Ian Campbell wrote:
> On Fri, 2015-07-10 at 19:00 +0100, Wei Liu wrote:
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> Would an "if (rc) libxl_cpupoolinfo_dispose(info)" on the exit path be a
> more robust alternative? Might require the addition of a
> libxl_cpupoolinfo_init() somewhere before any possible error.
> 

Yes, you're right, it would be better to fix the caller.

This results in a larger patch than this one. I will send out the new
version soon.

Wei.

> > ---
> >  tools/libxl/libxl.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > index 38aff8d..4151dcb 100644
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -740,8 +740,10 @@ static int cpupool_info(libxl__gc *gc,
> >      info->sched = xcinfo->sched_id;
> >      info->n_dom = xcinfo->n_dom;
> >      rc = libxl_cpu_bitmap_alloc(CTX, &info->cpumap, 0);
> > -    if (rc)
> > +    if (rc) {
> > +        free(info->pool_name);
> >          goto out;
> > +    }
> >  
> >      memcpy(info->cpumap.map, xcinfo->cpumap, info->cpumap.size);
> >  
> 

Re: [PATCH 5/9] libxl: avoid leaking string in cpupool_info

[Ian Campbell]

On Mon, 2015-07-13 at 17:10 +0100, Wei Liu wrote:
> On Mon, Jul 13, 2015 at 11:07:31AM +0100, Ian Campbell wrote:
> > On Fri, 2015-07-10 at 19:00 +0100, Wei Liu wrote:
> > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > 
> > Would an "if (rc) libxl_cpupoolinfo_dispose(info)" on the exit path be a
> > more robust alternative? Might require the addition of a
> > libxl_cpupoolinfo_init() somewhere before any possible error.
> > 
> 
> Yes, you're right, it would be better to fix the caller.

I was suggesting to do it in this function, not the caller.


> 
> This results in a larger patch than this one. I will send out the new
> version soon.
> 
> Wei.
> 
> > > ---
> > >  tools/libxl/libxl.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > > index 38aff8d..4151dcb 100644
> > > --- a/tools/libxl/libxl.c
> > > +++ b/tools/libxl/libxl.c
> > > @@ -740,8 +740,10 @@ static int cpupool_info(libxl__gc *gc,
> > >      info->sched = xcinfo->sched_id;
> > >      info->n_dom = xcinfo->n_dom;
> > >      rc = libxl_cpu_bitmap_alloc(CTX, &info->cpumap, 0);
> > > -    if (rc)
> > > +    if (rc) {
> > > +        free(info->pool_name);
> > >          goto out;
> > > +    }
> > >  
> > >      memcpy(info->cpumap.map, xcinfo->cpumap, info->cpumap.size);
> > >  
> > 

Re: [PATCH 5/9] libxl: avoid leaking string in cpupool_info

[Wei Liu]

On Mon, Jul 13, 2015 at 05:23:10PM +0100, Ian Campbell wrote:
> On Mon, 2015-07-13 at 17:10 +0100, Wei Liu wrote:
> > On Mon, Jul 13, 2015 at 11:07:31AM +0100, Ian Campbell wrote:
> > > On Fri, 2015-07-10 at 19:00 +0100, Wei Liu wrote:
> > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > > 
> > > Would an "if (rc) libxl_cpupoolinfo_dispose(info)" on the exit path be a
> > > more robust alternative? Might require the addition of a
> > > libxl_cpupoolinfo_init() somewhere before any possible error.
> > > 
> > 
> > Yes, you're right, it would be better to fix the caller.
> 
> I was suggesting to do it in this function, not the caller.
> 

No, that's not right, that violates libxl calling style. The caller is
supposed to call _init function before coming to this function.

Wei.

> 
> > 
> > This results in a larger patch than this one. I will send out the new
> > version soon.
> > 
> > Wei.
> > 
> > > > ---
> > > >  tools/libxl/libxl.c | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > > > index 38aff8d..4151dcb 100644
> > > > --- a/tools/libxl/libxl.c
> > > > +++ b/tools/libxl/libxl.c
> > > > @@ -740,8 +740,10 @@ static int cpupool_info(libxl__gc *gc,
> > > >      info->sched = xcinfo->sched_id;
> > > >      info->n_dom = xcinfo->n_dom;
> > > >      rc = libxl_cpu_bitmap_alloc(CTX, &info->cpumap, 0);
> > > > -    if (rc)
> > > > +    if (rc) {
> > > > +        free(info->pool_name);
> > > >          goto out;
> > > > +    }
> > > >  
> > > >      memcpy(info->cpumap.map, xcinfo->cpumap, info->cpumap.size);
> > > >  
> > > 
> 

Re: [PATCH 1/9] libxl: fix libxl__abs_path

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH 1/9] libxl: fix libxl__abs_path"):
> I rather dislike subjects of the form "fix $function", since it gives
> very little clue to someone reading the shortlog what is going on.

Yes.

> In this case I think "libxl: make libxl__abs_path correctly handle a
> NULL argument" would be an accurate description.

But: it is quite surprising that libxl__abs_path can be legally passed
a NULL for any of its parameters.

There are no call sites in libxl which can pass a NULL.

I think that if we are to retain this feature, it ought to be
documented, at least.

Maybe

  _hidden char *libxl__abs_path(libxl__gc *gc,
                                const char *s /* NULL OK */,
                                const char *path);

?

Ian.

Re: [PATCH 3/9] libxl: json string object can be NULL

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH 3/9] libxl: json string object can be NULL"):
> On Fri, 2015-07-10 at 19:00 +0100, Wei Liu wrote:
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> It occurs to me that since libxl__strdup can never return NULL due to
> allocation failure we could consider make libxl__strdup(gc, NULL) be
> well defined as returning NULL.

This would be a nicer fix, yes.

Ian.

Re: [PATCH 4/9] libxl: dispose dominfo to avoid leaking resource

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH 4/9] libxl: dispose dominfo to avoid leaking resource"):
> On Fri, 2015-07-10 at 19:00 +0100, Wei Liu wrote:
> > Add libxl_dominfo_dispose to one return path that doesn't have it.
> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> That return is a bit at odds with the generally correct error handling
> in that function, but this improves things at least a little and I can
> sort of see why this a slightly special case, so:

I think this should be done by changing `return' to `goto
out_success' or some such.

Ian.

Re: [PATCH 1/9] libxl: fix libxl__abs_path

[Ian Campbell]

On Mon, 2015-07-13 at 18:12 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH 1/9] libxl: fix libxl__abs_path"):
> > I rather dislike subjects of the form "fix $function", since it gives
> > very little clue to someone reading the shortlog what is going on.
> 
> Yes.
> 
> > In this case I think "libxl: make libxl__abs_path correctly handle a
> > NULL argument" would be an accurate description.
> 
> But: it is quite surprising that libxl__abs_path can be legally passed
> a NULL for any of its parameters.

True.
> 
> There are no call sites in libxl which can pass a NULL.
> 
> I think that if we are to retain this feature, it ought to be
> documented, at least.
> 
> Maybe
> 
>   _hidden char *libxl__abs_path(libxl__gc *gc,
>                                 const char *s /* NULL OK */,
>                                 const char *path);

Or add an assert if we don't wish to support this?

Re: [PATCH 1/9] libxl: fix libxl__abs_path

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH 1/9] libxl: fix libxl__abs_path"):
> On Mon, 2015-07-13 at 18:12 +0100, Ian Jackson wrote:
> > There are no call sites in libxl which can pass a NULL.
> > 
> > I think that if we are to retain this feature, it ought to be
> > documented, at least.
> 
> Or add an assert if we don't wish to support this?

If we don't want to support it we can just remove the check and let it
crash.

Ian.

Re: [PATCH 1/9] libxl: fix libxl__abs_path

[Ian Campbell]

On Tue, 2015-07-14 at 11:23 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH 1/9] libxl: fix libxl__abs_path"):
> > On Mon, 2015-07-13 at 18:12 +0100, Ian Jackson wrote:
> > > There are no call sites in libxl which can pass a NULL.
> > > 
> > > I think that if we are to retain this feature, it ought to be
> > > documented, at least.
> > 
> > Or add an assert if we don't wish to support this?
> 
> If we don't want to support it we can just remove the check and let it
> crash.

An assert at least gets us a slightly improved message when it happens.

Ian.

Re: [PATCH 1/9] libxl: fix libxl__abs_path

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH 1/9] libxl: fix libxl__abs_path"):
> On Tue, 2015-07-14 at 11:23 +0100, Ian Jackson wrote:
> > If we don't want to support it we can just remove the check and let it
> > crash.
> 
> An assert at least gets us a slightly improved message when it happens.

If that logic applies here it applies to almost every function in
libxl.  I don't think this is a sensible approach.

Ian.

Re: [PATCH 1/9] libxl: fix libxl__abs_path

[Ian Campbell]

On Tue, 2015-07-14 at 14:58 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH 1/9] libxl: fix libxl__abs_path"):
> > On Tue, 2015-07-14 at 11:23 +0100, Ian Jackson wrote:
> > > If we don't want to support it we can just remove the check and let it
> > > crash.
> > 
> > An assert at least gets us a slightly improved message when it happens.
> 
> If that logic applies here it applies to almost every function in
> libxl.  I don't think this is a sensible approach.

OK.

Re: [PATCH 1/9] libxl: fix libxl__abs_path

[Wei Liu]

On Tue, Jul 14, 2015 at 11:23:27AM +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH 1/9] libxl: fix libxl__abs_path"):
> > On Mon, 2015-07-13 at 18:12 +0100, Ian Jackson wrote:
> > > There are no call sites in libxl which can pass a NULL.
> > > 
> > > I think that if we are to retain this feature, it ought to be
> > > documented, at least.
> > 
> > Or add an assert if we don't wish to support this?
> 
> If we don't want to support it we can just remove the check and let it
> crash.
> 

I will drop this patch and create a new one with this approach.

Wei.

> Ian.