[PATCH 0/4] Coverity fixes for tools/libxl

[PATCH 0/4] Coverity fixes for tools/libxl

[Andrew Cooper]

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

-- 
1.7.10.4

[PATCH 1/4] tools/libxl: Avoid deliberate NULL pointer dereference

[Andrew Cooper]

Coverity ID: 1055290

Calling LIBXL__LOG_ERRNO(ctx,) with a ctx pointer we have just failed to
allocate is going to end badly.  Opencode a suitable use of xtl_log() instead.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.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 9b93262..6263d14 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -31,7 +31,9 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
 
     ctx = malloc(sizeof(*ctx));
     if (!ctx) {
-        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Failed to allocate context\n");
+        xtl_log(lg, XTL_ERROR, errno, "libxl",
+                "%s:%d:%s: Failed to allocate context\n",
+                __FILE__, __LINE__, __func__);
         rc = ERROR_NOMEM; goto out;
     }
 
-- 
1.7.10.4

Re: [PATCH 1/4] tools/libxl: Avoid deliberate NULL pointer dereference

[Ian Jackson]

Andrew Cooper writes ("[PATCH 1/4] tools/libxl: Avoid deliberate NULL pointer dereference"):
> Coverity ID: 1055290
> 
> Calling LIBXL__LOG_ERRNO(ctx,) with a ctx pointer we have just failed to
> allocate is going to end badly.  Opencode a suitable use of xtl_log() instead.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>

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

This patch is correct, although the usual approach in libxl would be
to crash by calling libxl__alloc_failed.  It would be possible to do
this in libxl_ctx_init by allocating a dummy ctx on the stack.

Ian.

[PATCH 2/4] tools/libxl: Fix integer overflows in sched_sedf_domain_set()

[Andrew Cooper]

Coverity ID: 1055662 1055663 1055664

Widen from int to uint64_t before multiplcation, rather than afterwards.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/libxl.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 6263d14..2b847ef 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4924,11 +4924,11 @@ static int sched_sedf_domain_set(libxl__gc *gc, uint32_t domid,
     }
 
     if (scinfo->period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT)
-        period = scinfo->period * 1000000;
+        period = (uint64_t)scinfo->period * 1000000;
     if (scinfo->slice != LIBXL_DOMAIN_SCHED_PARAM_SLICE_DEFAULT)
-        slice = scinfo->slice * 1000000;
+        slice = (uint64_t)scinfo->slice * 1000000;
     if (scinfo->latency != LIBXL_DOMAIN_SCHED_PARAM_LATENCY_DEFAULT)
-        latency = scinfo->latency * 1000000;
+        latency = (uint64_t)scinfo->latency * 1000000;
     if (scinfo->extratime != LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT)
         extratime = scinfo->extratime;
     if (scinfo->weight != LIBXL_DOMAIN_SCHED_PARAM_WEIGHT_DEFAULT)
-- 
1.7.10.4

Re: [PATCH 2/4] tools/libxl: Fix integer overflows in sched_sedf_domain_set()

[Ian Jackson]

Andrew Cooper writes ("[PATCH 2/4] tools/libxl: Fix integer overflows in sched_sedf_domain_set()"):
> Coverity ID: 1055662 1055663 1055664
> 
> Widen from int to uint64_t before multiplcation, rather than afterwards.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>

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

On the backport list.

Ian.

[PATCH 3/4] tools/libxl: Fix libxl__device_nic_from_xs_be()

[Andrew Cooper]

Coverity ID: 1055886

The callers of xs_read() is required to free the allocated memory.  Also avoid
calling libxl__parse_mac(NULL,) if the second xs_read() fails.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/libxl.c |   11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 2b847ef..d8dae3e 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2983,14 +2983,15 @@ static void libxl__device_nic_from_xs_be(libxl__gc *gc,
     libxl_ctx *ctx = libxl__gc_owner(gc);
     unsigned int len;
     char *tmp;
-    int rc;
 
     libxl_device_nic_init(nic);
 
     tmp = xs_read(ctx->xsh, XBT_NULL,
                   libxl__sprintf(gc, "%s/handle", be_path), &len);
-    if ( tmp )
+    if ( tmp ) {
         nic->devid = atoi(tmp);
+        free(tmp);
+    }
     else
         nic->devid = 0;
 
@@ -2998,10 +2999,12 @@ static void libxl__device_nic_from_xs_be(libxl__gc *gc,
 
     tmp = xs_read(ctx->xsh, XBT_NULL,
                   libxl__sprintf(gc, "%s/mac", be_path), &len);
-    rc = libxl__parse_mac(tmp, nic->mac);
-    if (rc)
+
+    if ( !tmp || libxl__parse_mac(tmp, nic->mac) != 0 )
         memset(nic->mac, 0, sizeof(nic->mac));
 
+    free(tmp);
+
     nic->ip = xs_read(ctx->xsh, XBT_NULL,
                       libxl__sprintf(gc, "%s/ip", be_path), &len);
 
-- 
1.7.10.4

Re: [PATCH 3/4] tools/libxl: Fix libxl__device_nic_from_xs_be()

[Roger Pau Monné]

On 25/11/13 12:12, Andrew Cooper wrote:
> Coverity ID: 1055886
> 
> The callers of xs_read() is required to free the allocated memory.  Also avoid
> calling libxl__parse_mac(NULL,) if the second xs_read() fails.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
>  tools/libxl/libxl.c |   11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 2b847ef..d8dae3e 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -2983,14 +2983,15 @@ static void libxl__device_nic_from_xs_be(libxl__gc *gc,
>      libxl_ctx *ctx = libxl__gc_owner(gc);
>      unsigned int len;
>      char *tmp;
> -    int rc;
>  
>      libxl_device_nic_init(nic);
>  
>      tmp = xs_read(ctx->xsh, XBT_NULL,
>                    libxl__sprintf(gc, "%s/handle", be_path), &len);

It might be easier to just use libxl__xs_read_checked since you are
changing that code.

> -    if ( tmp )
> +    if ( tmp ) {
>          nic->devid = atoi(tmp);
> +        free(tmp);
> +    }
>      else
>          nic->devid = 0;
>  
> @@ -2998,10 +2999,12 @@ static void libxl__device_nic_from_xs_be(libxl__gc *gc,
>  
>      tmp = xs_read(ctx->xsh, XBT_NULL,
>                    libxl__sprintf(gc, "%s/mac", be_path), &len);
> -    rc = libxl__parse_mac(tmp, nic->mac);
> -    if (rc)
> +
> +    if ( !tmp || libxl__parse_mac(tmp, nic->mac) != 0 )
>          memset(nic->mac, 0, sizeof(nic->mac));
>  
> +    free(tmp);
> +
>      nic->ip = xs_read(ctx->xsh, XBT_NULL,
>                        libxl__sprintf(gc, "%s/ip", be_path), &len);
>  
> 

[Patch v2 3/4] tools/libxl: Fix libxl__device_nic_from_xs_be()

[Andrew Cooper]

Coverity ID: 1055886

Replace uses of xs_read() with libxl__xs_read_checked() which appropriately
garbage-collects the allocated string, and avoid executing
libxl__parse_mac(NULL,) if the second xenstore read fails.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Roger Pau Monne <roger.pau@citrix.com>

---
Changes in v2:
 * Use libxl__xs_read_checked() in preference to xs_read() and free()
---
 tools/libxl/libxl.c |   18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 2b847ef..52f4c68 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2982,24 +2982,24 @@ static void libxl__device_nic_from_xs_be(libxl__gc *gc,
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
     unsigned int len;
-    char *tmp;
-    int rc;
+    const char *tmp;
 
     libxl_device_nic_init(nic);
 
-    tmp = xs_read(ctx->xsh, XBT_NULL,
-                  libxl__sprintf(gc, "%s/handle", be_path), &len);
-    if ( tmp )
+    if ( (libxl__xs_read_checked(gc, XBT_NULL,
+                                 libxl__sprintf(gc, "%s/handle", be_path),
+                                 &tmp) == 0) && strlen(tmp) )
         nic->devid = atoi(tmp);
     else
         nic->devid = 0;
 
     /* nic->mtu = */
 
-    tmp = xs_read(ctx->xsh, XBT_NULL,
-                  libxl__sprintf(gc, "%s/mac", be_path), &len);
-    rc = libxl__parse_mac(tmp, nic->mac);
-    if (rc)
+    if ( (libxl__xs_read_checked(gc, XBT_NULL,
+                                 libxl__sprintf(gc, "%s/mac", be_path),
+                                 &tmp) != 0) ||
+         (strlen(tmp) == 0) ||
+         (libxl__parse_mac(tmp, nic->mac) != 0) )
         memset(nic->mac, 0, sizeof(nic->mac));
 
     nic->ip = xs_read(ctx->xsh, XBT_NULL,
-- 
1.7.10.4

Re: [Patch v2 3/4] tools/libxl: Fix libxl__device_nic_from_xs_be()

[Roger Pau Monné]

On 25/11/13 16:19, Andrew Cooper wrote:
> Coverity ID: 1055886
> 
> Replace uses of xs_read() with libxl__xs_read_checked() which appropriately
> garbage-collects the allocated string, and avoid executing
> libxl__parse_mac(NULL,) if the second xenstore read fails.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Roger Pau Monne <roger.pau@citrix.com>
> 
> ---
> Changes in v2:
>  * Use libxl__xs_read_checked() in preference to xs_read() and free()
> ---
>  tools/libxl/libxl.c |   18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 2b847ef..52f4c68 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -2982,24 +2982,24 @@ static void libxl__device_nic_from_xs_be(libxl__gc *gc,
>  {
>      libxl_ctx *ctx = libxl__gc_owner(gc);
>      unsigned int len;
> -    char *tmp;
> -    int rc;
> +    const char *tmp;
>  
>      libxl_device_nic_init(nic);
>  
> -    tmp = xs_read(ctx->xsh, XBT_NULL,
> -                  libxl__sprintf(gc, "%s/handle", be_path), &len);
> -    if ( tmp )
> +    if ( (libxl__xs_read_checked(gc, XBT_NULL,
> +                                 libxl__sprintf(gc, "%s/handle", be_path),
> +                                 &tmp) == 0) && strlen(tmp) )

libxl coding style doesn't add spaces between parentheses. Also,
consider using rc instead of directly checking the return value of the
function.

>          nic->devid = atoi(tmp);
>      else
>          nic->devid = 0;
>  
>      /* nic->mtu = */
>  
> -    tmp = xs_read(ctx->xsh, XBT_NULL,
> -                  libxl__sprintf(gc, "%s/mac", be_path), &len);
> -    rc = libxl__parse_mac(tmp, nic->mac);
> -    if (rc)
> +    if ( (libxl__xs_read_checked(gc, XBT_NULL,
> +                                 libxl__sprintf(gc, "%s/mac", be_path),
> +                                 &tmp) != 0) ||
> +         (strlen(tmp) == 0) ||
> +         (libxl__parse_mac(tmp, nic->mac) != 0) )

No inner spaces in parentheses and use rc to keep this if condition much
more readable IMHO.

Roger.

>          memset(nic->mac, 0, sizeof(nic->mac));
>  
>      nic->ip = xs_read(ctx->xsh, XBT_NULL,
> 

[Patch v3 3/4] tools/libxl: Fix libxl__device_nic_from_xs_be()

[Andrew Cooper]

Coverity ID: 1055886

Replace uses of xs_read() with libxl__xs_read_checked() which appropriately
garbage-collects the allocated string, and avoid executing
libxl__parse_mac(NULL,) if the second xenstore read fails.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Roger Pau Monne <roger.pau@citrix.com>

---
Changes in v3:
 * Less hypervisor coding style
 * Slightly easier-to-read error conditional for nic->mac

Changes in v2:
 * Use libxl__xs_read_checked() in preference to xs_read() and free()
---
 tools/libxl/libxl.c |   20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 2b847ef..3237354 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2982,24 +2982,28 @@ static void libxl__device_nic_from_xs_be(libxl__gc *gc,
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
     unsigned int len;
-    char *tmp;
+    const char *tmp;
     int rc;
 
     libxl_device_nic_init(nic);
 
-    tmp = xs_read(ctx->xsh, XBT_NULL,
-                  libxl__sprintf(gc, "%s/handle", be_path), &len);
-    if ( tmp )
+    rc = libxl__xs_read_checked(gc, XBT_NULL,
+                                libxl__sprintf(gc, "%s/handle", be_path),
+                                &tmp);
+
+    if ((rc == 0) && strlen(tmp))
         nic->devid = atoi(tmp);
     else
         nic->devid = 0;
 
     /* nic->mtu = */
 
-    tmp = xs_read(ctx->xsh, XBT_NULL,
-                  libxl__sprintf(gc, "%s/mac", be_path), &len);
-    rc = libxl__parse_mac(tmp, nic->mac);
-    if (rc)
+    rc = libxl__xs_read_checked(gc, XBT_NULL,
+                                libxl__sprintf(gc, "%s/mac", be_path),
+                                &tmp);
+
+    if ((rc != 0) || (strlen(tmp) == 0) ||
+        (libxl__parse_mac(tmp, nic->mac) != 0))
         memset(nic->mac, 0, sizeof(nic->mac));
 
     nic->ip = xs_read(ctx->xsh, XBT_NULL,
-- 
1.7.10.4

Re: [Patch v3 3/4] tools/libxl: Fix libxl__device_nic_from_xs_be()

[Roger Pau Monné]

On 25/11/13 21:49, Andrew Cooper wrote:
> Coverity ID: 1055886
> 
> Replace uses of xs_read() with libxl__xs_read_checked() which appropriately
> garbage-collects the allocated string, and avoid executing
> libxl__parse_mac(NULL,) if the second xenstore read fails.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Roger Pau Monne <roger.pau@citrix.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

> 
> ---
> Changes in v3:
>  * Less hypervisor coding style
>  * Slightly easier-to-read error conditional for nic->mac
> 
> Changes in v2:
>  * Use libxl__xs_read_checked() in preference to xs_read() and free()
> ---
>  tools/libxl/libxl.c |   20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 2b847ef..3237354 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -2982,24 +2982,28 @@ static void libxl__device_nic_from_xs_be(libxl__gc *gc,
>  {
>      libxl_ctx *ctx = libxl__gc_owner(gc);
>      unsigned int len;
> -    char *tmp;
> +    const char *tmp;
>      int rc;
>  
>      libxl_device_nic_init(nic);
>  
> -    tmp = xs_read(ctx->xsh, XBT_NULL,
> -                  libxl__sprintf(gc, "%s/handle", be_path), &len);
> -    if ( tmp )
> +    rc = libxl__xs_read_checked(gc, XBT_NULL,
> +                                libxl__sprintf(gc, "%s/handle", be_path),
> +                                &tmp);
> +
> +    if ((rc == 0) && strlen(tmp))
>          nic->devid = atoi(tmp);
>      else
>          nic->devid = 0;
>  
>      /* nic->mtu = */
>  
> -    tmp = xs_read(ctx->xsh, XBT_NULL,
> -                  libxl__sprintf(gc, "%s/mac", be_path), &len);
> -    rc = libxl__parse_mac(tmp, nic->mac);
> -    if (rc)
> +    rc = libxl__xs_read_checked(gc, XBT_NULL,
> +                                libxl__sprintf(gc, "%s/mac", be_path),
> +                                &tmp);
> +
> +    if ((rc != 0) || (strlen(tmp) == 0) ||
> +        (libxl__parse_mac(tmp, nic->mac) != 0))
>          memset(nic->mac, 0, sizeof(nic->mac));
>  
>      nic->ip = xs_read(ctx->xsh, XBT_NULL,
> 

Re: [Patch v3 3/4] tools/libxl: Fix libxl__device_nic_from_xs_be()

[Ian Jackson]

Andrew Cooper writes ("[Xen-devel] [Patch v3 3/4] tools/libxl: Fix libxl__device_nic_from_xs_be()"):
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> -    tmp = xs_read(ctx->xsh, XBT_NULL,
> -                  libxl__sprintf(gc, "%s/handle", be_path), &len);
> -    if ( tmp )
> +    rc = libxl__xs_read_checked(gc, XBT_NULL,
> +                                libxl__sprintf(gc, "%s/handle", be_path),
> +                                &tmp);
> +
> +    if ((rc == 0) && strlen(tmp))

Nacked-by: Ian Jackson <ian.jackson@eu.citrix.com>
(for the benefit of Ian C.)

This is not correct.  See the doc comment for libxl__xs_read_checked:

    /* On success, *result_out came from the gc.
     * On error, *result_out is undefined.
     * ENOENT counts as success but sets *result_out=0
     */
    int libxl__xs_read_checked(libxl__gc *gc, xs_transaction_t t,
                               const char *path, const char **result_out);

So the correct pattern is:

    rc = libxl__xs_read_checked(gc, XBT_NULL, blah blah blah, &tmp);
    if (rc) goto out;

    if (tmp) {
        use tmp;
    } else {
        the path doesn't exist, do the other thing;
    }

I don't think there should be any need to check for empty strings
written to xenstore here ?  The old code doesn't.  Please someone tell
me there isn't.

Thanks,
Ian.

Re: [Patch v3 3/4] tools/libxl: Fix libxl__device_nic_from_xs_be()

[Andrew Cooper]

On 26/11/13 11:32, Ian Jackson wrote:
> Andrew Cooper writes ("[Xen-devel] [Patch v3 3/4] tools/libxl: Fix libxl__device_nic_from_xs_be()"):
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> -    tmp = xs_read(ctx->xsh, XBT_NULL,
>> -                  libxl__sprintf(gc, "%s/handle", be_path), &len);
>> -    if ( tmp )
>> +    rc = libxl__xs_read_checked(gc, XBT_NULL,
>> +                                libxl__sprintf(gc, "%s/handle", be_path),
>> +                                &tmp);
>> +
>> +    if ((rc == 0) && strlen(tmp))
> Nacked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> (for the benefit of Ian C.)
>
> This is not correct.  See the doc comment for libxl__xs_read_checked:
>
>     /* On success, *result_out came from the gc.
>      * On error, *result_out is undefined.
>      * ENOENT counts as success but sets *result_out=0
>      */
>     int libxl__xs_read_checked(libxl__gc *gc, xs_transaction_t t,
>                                const char *path, const char **result_out);
>
> So the correct pattern is:
>
>     rc = libxl__xs_read_checked(gc, XBT_NULL, blah blah blah, &tmp);
>     if (rc) goto out;
>
>     if (tmp) {
>         use tmp;
>     } else {
>         the path doesn't exist, do the other thing;
>     }
>
> I don't think there should be any need to check for empty strings
> written to xenstore here ?  The old code doesn't.  Please someone tell
> me there isn't.
>
> Thanks,
> Ian.

Ah - I think I have gotten the wrong indirection on tmp when attempting
to apply the documented ENOENT behaviour.

As this function cant fail, I was trying to force all error paths to
apply safe defaults to the libxl_device_nic structure.

I believe substituting the strlen(tmp) check for NULL checks will
produce the intended behaviour?

~Andrew

Re: [Patch v3 3/4] tools/libxl: Fix libxl__device_nic_from_xs_be()

[Ian Jackson]

Andrew Cooper writes ("Re: [Xen-devel] [Patch v3 3/4] tools/libxl: Fix libxl__device_nic_from_xs_be()"):
> As this function cant fail, I was trying to force all error paths to
> apply safe defaults to the libxl_device_nic structure.

Perhaps the function should be able to fail.

>From 3cea493c97f23eeb8e175915186f7ca2701da60a Mon Sep 17 00:00:00 2001
From: Ian Jackson <ian.jackson@eu.citrix.com>
Date: Tue, 26 Nov 2013 12:08:09 +0000
Subject: [PATCH] libxl: Fix error handling in libxl__device_nic_from_xs_be

This requires changing its return type and fixing the callers.

Introduce here a READ_BACKEND macro to make the code less repetitive.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/libxl.c |   62 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 37 insertions(+), 25 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 2b847ef..62ff6db 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2976,45 +2976,51 @@ out:
     return;
 }
 
-static void libxl__device_nic_from_xs_be(libxl__gc *gc,
-                                         const char *be_path,
-                                         libxl_device_nic *nic)
+static int libxl__device_nic_from_xs_be(libxl__gc *gc,
+                                        const char *be_path,
+                                        libxl_device_nic *nic)
 {
-    libxl_ctx *ctx = libxl__gc_owner(gc);
-    unsigned int len;
-    char *tmp;
+    const char *tmp;
     int rc;
 
     libxl_device_nic_init(nic);
 
-    tmp = xs_read(ctx->xsh, XBT_NULL,
-                  libxl__sprintf(gc, "%s/handle", be_path), &len);
-    if ( tmp )
+#define READ_BACKEND(subpath) ({                                        \
+        rc = libxl__xs_read_checked(gc, XBT_NULL,                       \
+                                    GCSPRINTF("%s/" subpath, be_path),  \
+                                    &tmp);                              \
+        if (rc) goto out;                                               \
+        (char*)tmp;                                                     \
+    });
+
+    tmp = READ_BACKEND("handle");
+    if (tmp)
         nic->devid = atoi(tmp);
     else
         nic->devid = 0;
 
     /* nic->mtu = */
 
-    tmp = xs_read(ctx->xsh, XBT_NULL,
-                  libxl__sprintf(gc, "%s/mac", be_path), &len);
-    rc = libxl__parse_mac(tmp, nic->mac);
-    if (rc)
+    tmp = READ_BACKEND("mac");
+    if (tmp) {
+        rc = libxl__parse_mac(tmp, nic->mac);
+        if (rc) goto out;
+    } else {
         memset(nic->mac, 0, sizeof(nic->mac));
+    }
 
-    nic->ip = xs_read(ctx->xsh, XBT_NULL,
-                      libxl__sprintf(gc, "%s/ip", be_path), &len);
-
-    nic->bridge = xs_read(ctx->xsh, XBT_NULL,
-                      libxl__sprintf(gc, "%s/bridge", be_path), &len);
-
-    nic->script = xs_read(ctx->xsh, XBT_NULL,
-                      libxl__sprintf(gc, "%s/script", be_path), &len);
+    nic->ip = READ_BACKEND("ip");
+    nic->bridge = READ_BACKEND("bridge");
+    nic->script = READ_BACKEND("script");
 
     /* vif_ioemu nics use the same xenstore entries as vif interfaces */
     nic->nictype = LIBXL_NIC_TYPE_VIF;
     nic->model = NULL; /* XXX Only for TYPE_IOEMU */
     nic->ifname = NULL; /* XXX Only for TYPE_IOEMU */
+
+    rc = 0;
+ out:
+    return rc;
 }
 
 int libxl_devid_to_device_nic(libxl_ctx *ctx, uint32_t domid,
@@ -3035,7 +3041,8 @@ int libxl_devid_to_device_nic(libxl_ctx *ctx, uint32_t domid,
     if (!path)
         goto out;
 
-    libxl__device_nic_from_xs_be(gc, path, nic);
+    rc = libxl__device_nic_from_xs_be(gc, path, nic);
+    if (rc) goto out;
 
     rc = 0;
 out:
@@ -3053,6 +3060,7 @@ static int libxl__append_nic_list_of_type(libxl__gc *gc,
     char **dir = NULL;
     unsigned int n = 0;
     libxl_device_nic *pnic = NULL, *pnic_end = NULL;
+    int rc;
 
     be_path = libxl__sprintf(gc, "%s/backend/%s/%d",
                              libxl__xs_get_dompath(gc, 0), type, domid);
@@ -3064,16 +3072,20 @@ static int libxl__append_nic_list_of_type(libxl__gc *gc,
             return ERROR_NOMEM;
         *nics = tmp;
         pnic = *nics + *nnics;
-        *nnics += n;
-        pnic_end = *nics + *nnics;
+        pnic_end = *nics + *nnics + n;
         for (; pnic < pnic_end; pnic++, dir++) {
             const char *p;
             p = libxl__sprintf(gc, "%s/%s", be_path, *dir);
-            libxl__device_nic_from_xs_be(gc, p, pnic);
+            rc = libxl__device_nic_from_xs_be(gc, p, pnic);
+            if (rc) goto out;
             pnic->backend_domid = 0;
         }
+        *nnics += n;
     }
     return 0;
+
+ out:
+    return rc;
 }
 
 libxl_device_nic *libxl_device_nic_list(libxl_ctx *ctx, uint32_t domid, int *num)
-- 
1.7.10.4

Re: [Patch v3 3/4] tools/libxl: Fix libxl__device_nic_from_xs_be()

[Andrew Cooper]

On 26/11/13 12:09, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [Xen-devel] [Patch v3 3/4] tools/libxl: Fix libxl__device_nic_from_xs_be()"):
>> As this function cant fail, I was trying to force all error paths to
>> apply safe defaults to the libxl_device_nic structure.
> Perhaps the function should be able to fail.
>
> From 3cea493c97f23eeb8e175915186f7ca2701da60a Mon Sep 17 00:00:00 2001
> From: Ian Jackson <ian.jackson@eu.citrix.com>
> Date: Tue, 26 Nov 2013 12:08:09 +0000
> Subject: [PATCH] libxl: Fix error handling in libxl__device_nic_from_xs_be
>
> This requires changing its return type and fixing the callers.
>
> Introduce here a READ_BACKEND macro to make the code less repetitive.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Commit message should include the Coverity ID 1055886, and perhaps a
reference to the fact that it is a memory leak.

> ---
>  tools/libxl/libxl.c |   62 ++++++++++++++++++++++++++++++---------------------
>  1 file changed, 37 insertions(+), 25 deletions(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 2b847ef..62ff6db 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -2976,45 +2976,51 @@ out:
>      return;
>  }
>  
> -static void libxl__device_nic_from_xs_be(libxl__gc *gc,
> -                                         const char *be_path,
> -                                         libxl_device_nic *nic)
> +static int libxl__device_nic_from_xs_be(libxl__gc *gc,
> +                                        const char *be_path,
> +                                        libxl_device_nic *nic)
>  {
> -    libxl_ctx *ctx = libxl__gc_owner(gc);
> -    unsigned int len;
> -    char *tmp;
> +    const char *tmp;
>      int rc;
>  
>      libxl_device_nic_init(nic);
>  
> -    tmp = xs_read(ctx->xsh, XBT_NULL,
> -                  libxl__sprintf(gc, "%s/handle", be_path), &len);
> -    if ( tmp )
> +#define READ_BACKEND(subpath) ({                                        \
> +        rc = libxl__xs_read_checked(gc, XBT_NULL,                       \
> +                                    GCSPRINTF("%s/" subpath, be_path),  \
> +                                    &tmp);                              \
> +        if (rc) goto out;                                               \
> +        (char*)tmp;                                                     \
> +    });
> +
> +    tmp = READ_BACKEND("handle");
> +    if (tmp)
>          nic->devid = atoi(tmp);
>      else
>          nic->devid = 0;
>  
>      /* nic->mtu = */
>  
> -    tmp = xs_read(ctx->xsh, XBT_NULL,
> -                  libxl__sprintf(gc, "%s/mac", be_path), &len);
> -    rc = libxl__parse_mac(tmp, nic->mac);
> -    if (rc)
> +    tmp = READ_BACKEND("mac");
> +    if (tmp) {
> +        rc = libxl__parse_mac(tmp, nic->mac);
> +        if (rc) goto out;
> +    } else {
>          memset(nic->mac, 0, sizeof(nic->mac));
> +    }
>  
> -    nic->ip = xs_read(ctx->xsh, XBT_NULL,
> -                      libxl__sprintf(gc, "%s/ip", be_path), &len);
> -
> -    nic->bridge = xs_read(ctx->xsh, XBT_NULL,
> -                      libxl__sprintf(gc, "%s/bridge", be_path), &len);
> -
> -    nic->script = xs_read(ctx->xsh, XBT_NULL,
> -                      libxl__sprintf(gc, "%s/script", be_path), &len);
> +    nic->ip = READ_BACKEND("ip");
> +    nic->bridge = READ_BACKEND("bridge");
> +    nic->script = READ_BACKEND("script");

This is not correct.  libxl_device_nic_dispose() is in charge of freeing
these pointers, but now they are part of the gc.

~Andrew

>  
>      /* vif_ioemu nics use the same xenstore entries as vif interfaces */
>      nic->nictype = LIBXL_NIC_TYPE_VIF;
>      nic->model = NULL; /* XXX Only for TYPE_IOEMU */
>      nic->ifname = NULL; /* XXX Only for TYPE_IOEMU */
> +
> +    rc = 0;
> + out:
> +    return rc;
>  }
>  
>  int libxl_devid_to_device_nic(libxl_ctx *ctx, uint32_t domid,
> @@ -3035,7 +3041,8 @@ int libxl_devid_to_device_nic(libxl_ctx *ctx, uint32_t domid,
>      if (!path)
>          goto out;
>  
> -    libxl__device_nic_from_xs_be(gc, path, nic);
> +    rc = libxl__device_nic_from_xs_be(gc, path, nic);
> +    if (rc) goto out;
>  
>      rc = 0;
>  out:
> @@ -3053,6 +3060,7 @@ static int libxl__append_nic_list_of_type(libxl__gc *gc,
>      char **dir = NULL;
>      unsigned int n = 0;
>      libxl_device_nic *pnic = NULL, *pnic_end = NULL;
> +    int rc;
>  
>      be_path = libxl__sprintf(gc, "%s/backend/%s/%d",
>                               libxl__xs_get_dompath(gc, 0), type, domid);
> @@ -3064,16 +3072,20 @@ static int libxl__append_nic_list_of_type(libxl__gc *gc,
>              return ERROR_NOMEM;
>          *nics = tmp;
>          pnic = *nics + *nnics;
> -        *nnics += n;
> -        pnic_end = *nics + *nnics;
> +        pnic_end = *nics + *nnics + n;
>          for (; pnic < pnic_end; pnic++, dir++) {
>              const char *p;
>              p = libxl__sprintf(gc, "%s/%s", be_path, *dir);
> -            libxl__device_nic_from_xs_be(gc, p, pnic);
> +            rc = libxl__device_nic_from_xs_be(gc, p, pnic);
> +            if (rc) goto out;
>              pnic->backend_domid = 0;
>          }
> +        *nnics += n;
>      }
>      return 0;
> +
> + out:
> +    return rc;
>  }
>  
>  libxl_device_nic *libxl_device_nic_list(libxl_ctx *ctx, uint32_t domid, int *num)

Re: [Patch v3 3/4] tools/libxl: Fix libxl__device_nic_from_xs_be()

[Ian Jackson]

Andrew Cooper writes ("Re: [Xen-devel] [Patch v3 3/4] tools/libxl: Fix libxl__device_nic_from_xs_be()"):
> Commit message should include the Coverity ID 1055886, and perhaps a
> reference to the fact that it is a memory leak.
...
> > +    nic->ip = READ_BACKEND("ip");
> > +    nic->bridge = READ_BACKEND("bridge");
> > +    nic->script = READ_BACKEND("script");
> 
> This is not correct.  libxl_device_nic_dispose() is in charge of freeing
> these pointers, but now they are part of the gc.

Oops.

>From 5bdaef8453bc8fd06da956df90fe33a12190342c Mon Sep 17 00:00:00 2001
From: Ian Jackson <ian.jackson@eu.citrix.com>
Date: Tue, 26 Nov 2013 12:08:09 +0000
Subject: [PATCH] libxl: Fix error handling in libxl__device_nic_from_xs_be

Previously, this function had a memory leak.  Fix this, and the rest
of the error handling.

This requires changing its return type and fixing the callers.

Introduce here a READ_BACKEND macro to make the code less repetitive.

Coverity ID: 1055886

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

---
v2: Strings nic-> allocated from NOGC, not the gc.
    Improve commit message.
---
 tools/libxl/libxl.c |   62 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 37 insertions(+), 25 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 2b847ef..ae1b7aa 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2976,45 +2976,51 @@ out:
     return;
 }
 
-static void libxl__device_nic_from_xs_be(libxl__gc *gc,
-                                         const char *be_path,
-                                         libxl_device_nic *nic)
+static int libxl__device_nic_from_xs_be(libxl__gc *gc,
+                                        const char *be_path,
+                                        libxl_device_nic *nic)
 {
-    libxl_ctx *ctx = libxl__gc_owner(gc);
-    unsigned int len;
-    char *tmp;
+    const char *tmp;
     int rc;
 
     libxl_device_nic_init(nic);
 
-    tmp = xs_read(ctx->xsh, XBT_NULL,
-                  libxl__sprintf(gc, "%s/handle", be_path), &len);
-    if ( tmp )
+#define READ_BACKEND(tgc, subpath) ({                                   \
+        rc = libxl__xs_read_checked(tgc, XBT_NULL,                      \
+                                    GCSPRINTF("%s/" subpath, be_path),  \
+                                    &tmp);                              \
+        if (rc) goto out;                                               \
+        (char*)tmp;                                                     \
+    });
+
+    tmp = READ_BACKEND(gc, "handle");
+    if (tmp)
         nic->devid = atoi(tmp);
     else
         nic->devid = 0;
 
     /* nic->mtu = */
 
-    tmp = xs_read(ctx->xsh, XBT_NULL,
-                  libxl__sprintf(gc, "%s/mac", be_path), &len);
-    rc = libxl__parse_mac(tmp, nic->mac);
-    if (rc)
+    tmp = READ_BACKEND(gc, "mac");
+    if (tmp) {
+        rc = libxl__parse_mac(tmp, nic->mac);
+        if (rc) goto out;
+    } else {
         memset(nic->mac, 0, sizeof(nic->mac));
+    }
 
-    nic->ip = xs_read(ctx->xsh, XBT_NULL,
-                      libxl__sprintf(gc, "%s/ip", be_path), &len);
-
-    nic->bridge = xs_read(ctx->xsh, XBT_NULL,
-                      libxl__sprintf(gc, "%s/bridge", be_path), &len);
-
-    nic->script = xs_read(ctx->xsh, XBT_NULL,
-                      libxl__sprintf(gc, "%s/script", be_path), &len);
+    nic->ip = READ_BACKEND(NOGC, "ip");
+    nic->bridge = READ_BACKEND(NOGC, "bridge");
+    nic->script = READ_BACKEND(NOGC, "script");
 
     /* vif_ioemu nics use the same xenstore entries as vif interfaces */
     nic->nictype = LIBXL_NIC_TYPE_VIF;
     nic->model = NULL; /* XXX Only for TYPE_IOEMU */
     nic->ifname = NULL; /* XXX Only for TYPE_IOEMU */
+
+    rc = 0;
+ out:
+    return rc;
 }
 
 int libxl_devid_to_device_nic(libxl_ctx *ctx, uint32_t domid,
@@ -3035,7 +3041,8 @@ int libxl_devid_to_device_nic(libxl_ctx *ctx, uint32_t domid,
     if (!path)
         goto out;
 
-    libxl__device_nic_from_xs_be(gc, path, nic);
+    rc = libxl__device_nic_from_xs_be(gc, path, nic);
+    if (rc) goto out;
 
     rc = 0;
 out:
@@ -3053,6 +3060,7 @@ static int libxl__append_nic_list_of_type(libxl__gc *gc,
     char **dir = NULL;
     unsigned int n = 0;
     libxl_device_nic *pnic = NULL, *pnic_end = NULL;
+    int rc;
 
     be_path = libxl__sprintf(gc, "%s/backend/%s/%d",
                              libxl__xs_get_dompath(gc, 0), type, domid);
@@ -3064,16 +3072,20 @@ static int libxl__append_nic_list_of_type(libxl__gc *gc,
             return ERROR_NOMEM;
         *nics = tmp;
         pnic = *nics + *nnics;
-        *nnics += n;
-        pnic_end = *nics + *nnics;
+        pnic_end = *nics + *nnics + n;
         for (; pnic < pnic_end; pnic++, dir++) {
             const char *p;
             p = libxl__sprintf(gc, "%s/%s", be_path, *dir);
-            libxl__device_nic_from_xs_be(gc, p, pnic);
+            rc = libxl__device_nic_from_xs_be(gc, p, pnic);
+            if (rc) goto out;
             pnic->backend_domid = 0;
         }
+        *nnics += n;
     }
     return 0;
+
+ out:
+    return rc;
 }
 
 libxl_device_nic *libxl_device_nic_list(libxl_ctx *ctx, uint32_t domid, int *num)
-- 
1.7.10.4

Re: [Patch v3 3/4] tools/libxl: Fix libxl__device_nic_from_xs_be()

[Andrew Cooper]

On 26/11/13 15:08, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [Xen-devel] [Patch v3 3/4] tools/libxl: Fix libxl__device_nic_from_xs_be()"):
>> Commit message should include the Coverity ID 1055886, and perhaps a
>> reference to the fact that it is a memory leak.
> ...
>>> +    nic->ip = READ_BACKEND("ip");
>>> +    nic->bridge = READ_BACKEND("bridge");
>>> +    nic->script = READ_BACKEND("script");
>> This is not correct.  libxl_device_nic_dispose() is in charge of freeing
>> these pointers, but now they are part of the gc.
> Oops.
>
> From 5bdaef8453bc8fd06da956df90fe33a12190342c Mon Sep 17 00:00:00 2001
> From: Ian Jackson <ian.jackson@eu.citrix.com>
> Date: Tue, 26 Nov 2013 12:08:09 +0000
> Subject: [PATCH] libxl: Fix error handling in libxl__device_nic_from_xs_be
>
> Previously, this function had a memory leak.  Fix this, and the rest
> of the error handling.
>
> This requires changing its return type and fixing the callers.
>
> Introduce here a READ_BACKEND macro to make the code less repetitive.
>
> Coverity ID: 1055886
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

This looks like it might plausibly work.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>
> ---
> v2: Strings nic-> allocated from NOGC, not the gc.
>     Improve commit message.
> ---
>  tools/libxl/libxl.c |   62 ++++++++++++++++++++++++++++++---------------------
>  1 file changed, 37 insertions(+), 25 deletions(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 2b847ef..ae1b7aa 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -2976,45 +2976,51 @@ out:
>      return;
>  }
>  
> -static void libxl__device_nic_from_xs_be(libxl__gc *gc,
> -                                         const char *be_path,
> -                                         libxl_device_nic *nic)
> +static int libxl__device_nic_from_xs_be(libxl__gc *gc,
> +                                        const char *be_path,
> +                                        libxl_device_nic *nic)
>  {
> -    libxl_ctx *ctx = libxl__gc_owner(gc);
> -    unsigned int len;
> -    char *tmp;
> +    const char *tmp;
>      int rc;
>  
>      libxl_device_nic_init(nic);
>  
> -    tmp = xs_read(ctx->xsh, XBT_NULL,
> -                  libxl__sprintf(gc, "%s/handle", be_path), &len);
> -    if ( tmp )
> +#define READ_BACKEND(tgc, subpath) ({                                   \
> +        rc = libxl__xs_read_checked(tgc, XBT_NULL,                      \
> +                                    GCSPRINTF("%s/" subpath, be_path),  \
> +                                    &tmp);                              \
> +        if (rc) goto out;                                               \
> +        (char*)tmp;                                                     \
> +    });
> +
> +    tmp = READ_BACKEND(gc, "handle");
> +    if (tmp)
>          nic->devid = atoi(tmp);
>      else
>          nic->devid = 0;
>  
>      /* nic->mtu = */
>  
> -    tmp = xs_read(ctx->xsh, XBT_NULL,
> -                  libxl__sprintf(gc, "%s/mac", be_path), &len);
> -    rc = libxl__parse_mac(tmp, nic->mac);
> -    if (rc)
> +    tmp = READ_BACKEND(gc, "mac");
> +    if (tmp) {
> +        rc = libxl__parse_mac(tmp, nic->mac);
> +        if (rc) goto out;
> +    } else {
>          memset(nic->mac, 0, sizeof(nic->mac));
> +    }
>  
> -    nic->ip = xs_read(ctx->xsh, XBT_NULL,
> -                      libxl__sprintf(gc, "%s/ip", be_path), &len);
> -
> -    nic->bridge = xs_read(ctx->xsh, XBT_NULL,
> -                      libxl__sprintf(gc, "%s/bridge", be_path), &len);
> -
> -    nic->script = xs_read(ctx->xsh, XBT_NULL,
> -                      libxl__sprintf(gc, "%s/script", be_path), &len);
> +    nic->ip = READ_BACKEND(NOGC, "ip");
> +    nic->bridge = READ_BACKEND(NOGC, "bridge");
> +    nic->script = READ_BACKEND(NOGC, "script");
>  
>      /* vif_ioemu nics use the same xenstore entries as vif interfaces */
>      nic->nictype = LIBXL_NIC_TYPE_VIF;
>      nic->model = NULL; /* XXX Only for TYPE_IOEMU */
>      nic->ifname = NULL; /* XXX Only for TYPE_IOEMU */
> +
> +    rc = 0;
> + out:
> +    return rc;
>  }
>  
>  int libxl_devid_to_device_nic(libxl_ctx *ctx, uint32_t domid,
> @@ -3035,7 +3041,8 @@ int libxl_devid_to_device_nic(libxl_ctx *ctx, uint32_t domid,
>      if (!path)
>          goto out;
>  
> -    libxl__device_nic_from_xs_be(gc, path, nic);
> +    rc = libxl__device_nic_from_xs_be(gc, path, nic);
> +    if (rc) goto out;
>  
>      rc = 0;
>  out:
> @@ -3053,6 +3060,7 @@ static int libxl__append_nic_list_of_type(libxl__gc *gc,
>      char **dir = NULL;
>      unsigned int n = 0;
>      libxl_device_nic *pnic = NULL, *pnic_end = NULL;
> +    int rc;
>  
>      be_path = libxl__sprintf(gc, "%s/backend/%s/%d",
>                               libxl__xs_get_dompath(gc, 0), type, domid);
> @@ -3064,16 +3072,20 @@ static int libxl__append_nic_list_of_type(libxl__gc *gc,
>              return ERROR_NOMEM;
>          *nics = tmp;
>          pnic = *nics + *nnics;
> -        *nnics += n;
> -        pnic_end = *nics + *nnics;
> +        pnic_end = *nics + *nnics + n;
>          for (; pnic < pnic_end; pnic++, dir++) {
>              const char *p;
>              p = libxl__sprintf(gc, "%s/%s", be_path, *dir);
> -            libxl__device_nic_from_xs_be(gc, p, pnic);
> +            rc = libxl__device_nic_from_xs_be(gc, p, pnic);
> +            if (rc) goto out;
>              pnic->backend_domid = 0;
>          }
> +        *nnics += n;
>      }
>      return 0;
> +
> + out:
> +    return rc;
>  }
>  
>  libxl_device_nic *libxl_device_nic_list(libxl_ctx *ctx, uint32_t domid, int *num)

Re: [Patch v3 3/4] tools/libxl: Fix libxl__device_nic_from_xs_be()

[Ian Jackson]

Andrew Cooper writes ("Re: [Xen-devel] [Patch v3 3/4] tools/libxl: Fix libxl__device_nic_from_xs_be()"):
> On 26/11/13 15:08, Ian Jackson wrote:
> > From: Ian Jackson <ian.jackson@eu.citrix.com>
> > Date: Tue, 26 Nov 2013 12:08:09 +0000
> > Subject: [PATCH] libxl: Fix error handling in libxl__device_nic_from_xs_be
...
> > Introduce here a READ_BACKEND macro to make the code less repetitive.

I was thinking about READ_BACKEND and wondered whether we should have
something like this in libxl_internal.h:

 /*
  * const char *XSREADF_OR_RCOUT(libxl__gc*, const char *format, ...);
  *
  * Reads the xenstore key at sprintf(format, ...).
  * On success returns the string (from the gc tgc), or NULL for ENOENT.
  * On other errors, logs, sets rc, and does "goto out".
  *
  * Expects in its scope:
  *   libxl__gc *gc;  // for the sprintf
  *   int rc;         // trashed
  *   out:            // used on error only; jumped to with rc set
  */
 #define XSREADF_OR_RCOUT(tgc, format, ...) ({                            \
         const char *xsreadf_tmp;                                         \
         rc = libxl__xs_read_checked((tgc), XBT_NULL,                     \
                                     GCSPRINTF((format), __VA_ARGS__),    \
                                     &xsreadf_tmp);                       \
         if (rc) goto out;                                                \
         xsreadf_tmp;                                                     \
     )}

We don't presently have anywhere in libxl_internal.h that assumes the
existence of "out" and "rc" in their scope.

Ian.

Re: [Patch v3 3/4] tools/libxl: Fix libxl__device_nic_from_xs_be()

[Andrew Cooper]

On 26/11/13 15:39, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [Xen-devel] [Patch v3 3/4] tools/libxl: Fix libxl__device_nic_from_xs_be()"):
>> On 26/11/13 15:08, Ian Jackson wrote:
>>> From: Ian Jackson <ian.jackson@eu.citrix.com>
>>> Date: Tue, 26 Nov 2013 12:08:09 +0000
>>> Subject: [PATCH] libxl: Fix error handling in libxl__device_nic_from_xs_be
> ...
>>> Introduce here a READ_BACKEND macro to make the code less repetitive.
> I was thinking about READ_BACKEND and wondered whether we should have
> something like this in libxl_internal.h:
>
>  /*
>   * const char *XSREADF_OR_RCOUT(libxl__gc*, const char *format, ...);
>   *
>   * Reads the xenstore key at sprintf(format, ...).
>   * On success returns the string (from the gc tgc), or NULL for ENOENT.
>   * On other errors, logs, sets rc, and does "goto out".
>   *
>   * Expects in its scope:
>   *   libxl__gc *gc;  // for the sprintf
>   *   int rc;         // trashed
>   *   out:            // used on error only; jumped to with rc set
>   */
>  #define XSREADF_OR_RCOUT(tgc, format, ...) ({                            \
>          const char *xsreadf_tmp;                                         \
>          rc = libxl__xs_read_checked((tgc), XBT_NULL,                     \
>                                      GCSPRINTF((format), __VA_ARGS__),    \
>                                      &xsreadf_tmp);                       \
>          if (rc) goto out;                                                \
>          xsreadf_tmp;                                                     \
>      )}
>
> We don't presently have anywhere in libxl_internal.h that assumes the
> existence of "out" and "rc" in their scope.
>
> Ian.

So what is happening with this?  The fix in the parent email should be
ok, and might be better this late in the development cycle.

~Andrew

Re: [Patch v3 3/4] tools/libxl: Fix libxl__device_nic_from_xs_be()

[Ian Campbell]

On Tue, 2013-11-26 at 15:39 +0000, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [Xen-devel] [Patch v3 3/4] tools/libxl: Fix libxl__device_nic_from_xs_be()"):
> > On 26/11/13 15:08, Ian Jackson wrote:
> > > From: Ian Jackson <ian.jackson@eu.citrix.com>
> > > Date: Tue, 26 Nov 2013 12:08:09 +0000
> > > Subject: [PATCH] libxl: Fix error handling in libxl__device_nic_from_xs_be
> ...
> > > Introduce here a READ_BACKEND macro to make the code less repetitive.
> 
> I was thinking about READ_BACKEND and wondered whether we should have
> something like this in libxl_internal.h:

Irrespective of this I've applied your patch, since this discussion can
only lead to a future, post-4.4, cleanup.

>  /*
>   * const char *XSREADF_OR_RCOUT(libxl__gc*, const char *format, ...);
>   *
>   * Reads the xenstore key at sprintf(format, ...).
>   * On success returns the string (from the gc tgc), or NULL for ENOENT.
>   * On other errors, logs, sets rc, and does "goto out".
>   *
>   * Expects in its scope:
>   *   libxl__gc *gc;  // for the sprintf
>   *   int rc;         // trashed
>   *   out:            // used on error only; jumped to with rc set
>   */
>  #define XSREADF_OR_RCOUT(tgc, format, ...) ({                            \
>          const char *xsreadf_tmp;                                         \
>          rc = libxl__xs_read_checked((tgc), XBT_NULL,                     \
>                                      GCSPRINTF((format), __VA_ARGS__),    \
>                                      &xsreadf_tmp);                       \
>          if (rc) goto out;                                                \
>          xsreadf_tmp;                                                     \
>      )}
> 
> We don't presently have anywhere in libxl_internal.h that assumes the
> existence of "out" and "rc" in their scope.

Not sure how I feel about this. I suppose we are slowing getting more
consistent about the use of rc as a libxl error code at least.

Ian.

Re: [Patch v3 3/4] tools/libxl: Fix libxl__device_nic_from_xs_be()

[Ian Campbell]

On Tue, 2013-11-26 at 15:15 +0000, Andrew Cooper wrote:
> On 26/11/13 15:08, Ian Jackson wrote:
> > Andrew Cooper writes ("Re: [Xen-devel] [Patch v3 3/4] tools/libxl: Fix libxl__device_nic_from_xs_be()"):
> >> Commit message should include the Coverity ID 1055886, and perhaps a
> >> reference to the fact that it is a memory leak.
> > ...
> >>> +    nic->ip = READ_BACKEND("ip");
> >>> +    nic->bridge = READ_BACKEND("bridge");
> >>> +    nic->script = READ_BACKEND("script");
> >> This is not correct.  libxl_device_nic_dispose() is in charge of freeing
> >> these pointers, but now they are part of the gc.
> > Oops.
> >
> > From 5bdaef8453bc8fd06da956df90fe33a12190342c Mon Sep 17 00:00:00 2001
> > From: Ian Jackson <ian.jackson@eu.citrix.com>
> > Date: Tue, 26 Nov 2013 12:08:09 +0000
> > Subject: [PATCH] libxl: Fix error handling in libxl__device_nic_from_xs_be
> >
> > Previously, this function had a memory leak.  Fix this, and the rest
> > of the error handling.

I had to go back to the original to figure out what the memory leak was,
and so I rewrote this as:

    Previously, this function would leak the temporary return from xs_read for
    handle and mac address.  Fix both of these and the rest of the error handling.
    
> >
> > This requires changing its return type and fixing the callers.
> >
> > Introduce here a READ_BACKEND macro to make the code less repetitive.
> >
> > Coverity ID: 1055886
> >
> > Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> 
> This looks like it might plausibly work.
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked + committed with the above.

Ian.

Re: [PATCH 3/4] tools/libxl: Fix libxl__device_nic_from_xs_be()

[Ian Jackson]

Andrew Cooper writes ("[PATCH 3/4] tools/libxl: Fix libxl__device_nic_from_xs_be()"):
> Coverity ID: 1055886
> 
> The callers of xs_read() is required to free the allocated memory.
> Also avoid calling libxl__parse_mac(NULL,) if the second xs_read()
> fails.

I think the first problem would be better fixed by calling
libxl__xs_read_checked with an appropriate gc.

> -    rc = libxl__parse_mac(tmp, nic->mac);
> -    if (rc)
> +
> +    if ( !tmp || libxl__parse_mac(tmp, nic->mac) != 0 )
>          memset(nic->mac, 0, sizeof(nic->mac));

Also, the libxl coding style doesn't have the spaces inside the
parens there.

Thanks,
Ian.

[PATCH 4/4] tools/libxl: Fix memory leak in sched_domain_output()

[Andrew Cooper]

Coverity ID: 1055904

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 tools/libxl/xl_cmdimpl.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 341863e..bdb4be3 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5094,6 +5094,7 @@ static int sched_domain_output(libxl_scheduler sched, int (*output)(int),
     poolinfo = libxl_list_cpupool(ctx, &n_pools);
     if (!poolinfo) {
         fprintf(stderr, "error getting cpupool info\n");
+        libxl_dominfo_list_free(info, nb_domain);
         return -ERROR_NOMEM;
     }
 
@@ -5115,6 +5116,7 @@ static int sched_domain_output(libxl_scheduler sched, int (*output)(int),
     }
 
     libxl_cpupoolinfo_list_free(poolinfo, n_pools);
+    libxl_dominfo_list_free(info, nb_domain);
     return 0;
 }
 
-- 
1.7.10.4

Re: [PATCH 4/4] tools/libxl: Fix memory leak in sched_domain_output()

[Ian Jackson]

Andrew Cooper writes ("[Xen-devel] [PATCH 4/4] tools/libxl: Fix memory leak in sched_domain_output()"):
> Coverity ID: 1055904
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>

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

The error handling in this function is odd.  It would be better to
change it to "goto out" but then I started looking at the return types
of this and of other functions in xl_cmdimpl.c.

"return -ERROR_FAIL" ?!

And later we have functions which
  return -sched_domain_output(...)
but also
  return 1

Ian.

Re: [PATCH 4/4] tools/libxl: Fix memory leak in sched_domain_output()

[Andrew Cooper]

On 25/11/13 13:46, Ian Jackson wrote:
> Andrew Cooper writes ("[Xen-devel] [PATCH 4/4] tools/libxl: Fix memory leak in sched_domain_output()"):
>> Coverity ID: 1055904
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Keir Fraser <keir@xen.org>
>> CC: Jan Beulich <JBeulich@suse.com>
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Ian Campbell <Ian.Campbell@citrix.com>
> Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>
>
> The error handling in this function is odd.  It would be better to
> change it to "goto out" but then I started looking at the return types
> of this and of other functions in xl_cmdimpl.c.
>
> "return -ERROR_FAIL" ?!
>
> And later we have functions which
>   return -sched_domain_output(...)
> but also
>   return 1
>
> Ian.

Yes - I found the error handling quite odd.  I decided not to poke the
tangled mess.

~Andrew