[PATCH 0/7] libxc: Fix a number of coverity issues.

[PATCH 0/7] libxc: Fix a number of coverity issues.

[Jennifer Herbert]

Fix a number of coverity issues in libxc.

[PATCH 1/7] libxc: fix uninitialized variable in xc_cpuid_pv_policy()

[Jennifer Herbert]

If xc_domain_get_guest_width were to fail, guest_width is not set, and
hence guest_64bit becomes undefined.
Fix is to initialise to 0, and report error if call fails.

Signed-off-by: Jennifer Herbert <jennifer.herbert@citrix.com>
---
 tools/libxc/xc_cpuid_x86.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index c97f91a..847b701 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -437,14 +437,16 @@ static void xc_cpuid_pv_policy(
 {
     DECLARE_DOMCTL;
     unsigned int guest_width;
-    int guest_64bit;
+    int guest_64bit = 0;
     char brand[13];
     uint64_t xfeature_mask;
 
     xc_cpuid_brand_get(brand);
 
-    xc_domain_get_guest_width(xch, domid, &guest_width);
-    guest_64bit = (guest_width == 8);
+    if (xc_domain_get_guest_width(xch, domid, &guest_width) == 0)
+        guest_64bit = (guest_width == 8);
+    else
+        ERROR("Could not read guest word width.");
 
     /* Detecting Xen's atitude towards XSAVE */
     memset(&domctl, 0, sizeof(domctl));
-- 
1.7.10.4

Re: [PATCH 1/7] libxc: fix uninitialized variable in xc_cpuid_pv_policy()

[Andrew Cooper]

On 01/07/15 18:37, Jennifer Herbert wrote:
> If xc_domain_get_guest_width were to fail, guest_width is not set, and
> hence guest_64bit becomes undefined.
> Fix is to initialise to 0, and report error if call fails.
>
> Signed-off-by: Jennifer Herbert <jennifer.herbert@citrix.com>
> ---
>  tools/libxc/xc_cpuid_x86.c |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
> index c97f91a..847b701 100644
> --- a/tools/libxc/xc_cpuid_x86.c
> +++ b/tools/libxc/xc_cpuid_x86.c
> @@ -437,14 +437,16 @@ static void xc_cpuid_pv_policy(

Urgh - let another libxc function which can fail hard in several ways,
yet is void.

>  {
>      DECLARE_DOMCTL;
>      unsigned int guest_width;
> -    int guest_64bit;
> +    int guest_64bit = 0;

The default in Xen is that a PV guest is 64bit until explicitly
converted to being compat.  The better default therefore is 1.

>      char brand[13];
>      uint64_t xfeature_mask;
>  
>      xc_cpuid_brand_get(brand);
>  
> -    xc_domain_get_guest_width(xch, domid, &guest_width);
> -    guest_64bit = (guest_width == 8);
> +    if (xc_domain_get_guest_width(xch, domid, &guest_width) == 0)
> +        guest_64bit = (guest_width == 8);
> +    else
> +        ERROR("Could not read guest word width.");

No full stop please.

>  
>      /* Detecting Xen's atitude towards XSAVE */
>      memset(&domctl, 0, sizeof(domctl));

Re: [PATCH 1/7] libxc: fix uninitialized variable in xc_cpuid_pv_policy()

[Andrew Cooper]

On 01/07/15 18:37, Jennifer Herbert wrote:
> If xc_domain_get_guest_width were to fail, guest_width is not set, and
> hence guest_64bit becomes undefined.
> Fix is to initialise to 0, and report error if call fails.
>
> Signed-off-by: Jennifer Herbert <jennifer.herbert@citrix.com>

Coverity-ID: 1090350

~Andrew

> ---
>  tools/libxc/xc_cpuid_x86.c |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
> index c97f91a..847b701 100644
> --- a/tools/libxc/xc_cpuid_x86.c
> +++ b/tools/libxc/xc_cpuid_x86.c
> @@ -437,14 +437,16 @@ static void xc_cpuid_pv_policy(
>  {
>      DECLARE_DOMCTL;
>      unsigned int guest_width;
> -    int guest_64bit;
> +    int guest_64bit = 0;
>      char brand[13];
>      uint64_t xfeature_mask;
>  
>      xc_cpuid_brand_get(brand);
>  
> -    xc_domain_get_guest_width(xch, domid, &guest_width);
> -    guest_64bit = (guest_width == 8);
> +    if (xc_domain_get_guest_width(xch, domid, &guest_width) == 0)
> +        guest_64bit = (guest_width == 8);
> +    else
> +        ERROR("Could not read guest word width.");
>  
>      /* Detecting Xen's atitude towards XSAVE */
>      memset(&domctl, 0, sizeof(domctl));

Re: [PATCH 1/7] libxc: fix uninitialized variable in xc_cpuid_pv_policy()

[Ian Jackson]

Jennifer Herbert writes ("[Xen-devel] [PATCH 1/7] libxc: fix uninitialized variable in xc_cpuid_pv_policy()"):
> If xc_domain_get_guest_width were to fail, guest_width is not set, and
> hence guest_64bit becomes undefined.
> Fix is to initialise to 0, and report error if call fails.
...
> diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
> index c97f91a..847b701 100644
> --- a/tools/libxc/xc_cpuid_x86.c
> +++ b/tools/libxc/xc_cpuid_x86.c
> @@ -437,14 +437,16 @@ static void xc_cpuid_pv_policy(
>  {
>      DECLARE_DOMCTL;
>      unsigned int guest_width;
> -    int guest_64bit;
> +    int guest_64bit = 0;

I'm not a huge fan of this style, which some people might describe as
`defensive initialisations'.  They turn failures to initialise a
variable (which can be detected by tools like Coverity and some
compilers), into uses of the wrong value.

> -    xc_domain_get_guest_width(xch, domid, &guest_width);
> -    guest_64bit = (guest_width == 8);
> +    if (xc_domain_get_guest_width(xch, domid, &guest_width) == 0)
> +        guest_64bit = (guest_width == 8);
> +    else
> +        ERROR("Could not read guest word width.");

Surely after failure of xc_domain_get_guest_width we should not
blunder on, making unwarranted assumptions about the guest bit width.

Unfortunately xc_cpuid_pv_policy doesn't return an error code.  I
think it needs to.  So that's rather a yak.

Sorry,
Ian.

Re: [PATCH 1/7] libxc: fix uninitialized variable in xc_cpuid_pv_policy()

[Ian Campbell]

On Fri, 2015-07-03 at 16:15 +0100, Ian Jackson wrote:
> Jennifer Herbert writes ("[Xen-devel] [PATCH 1/7] libxc: fix uninitialized variable in xc_cpuid_pv_policy()"):
> > If xc_domain_get_guest_width were to fail, guest_width is not set, and
> > hence guest_64bit becomes undefined.
> > Fix is to initialise to 0, and report error if call fails.
> ...
> > diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
> > index c97f91a..847b701 100644
> > --- a/tools/libxc/xc_cpuid_x86.c
> > +++ b/tools/libxc/xc_cpuid_x86.c
> > @@ -437,14 +437,16 @@ static void xc_cpuid_pv_policy(
> >  {
> >      DECLARE_DOMCTL;
> >      unsigned int guest_width;
> > -    int guest_64bit;
> > +    int guest_64bit = 0;
> 
> I'm not a huge fan of this style, which some people might describe as
> `defensive initialisations'.  They turn failures to initialise a
> variable (which can be detected by tools like Coverity and some
> compilers), into uses of the wrong value.
> 
> > -    xc_domain_get_guest_width(xch, domid, &guest_width);
> > -    guest_64bit = (guest_width == 8);
> > +    if (xc_domain_get_guest_width(xch, domid, &guest_width) == 0)
> > +        guest_64bit = (guest_width == 8);
> > +    else
> > +        ERROR("Could not read guest word width.");
> 
> Surely after failure of xc_domain_get_guest_width we should not
> blunder on, making unwarranted assumptions about the guest bit width.
> 
> Unfortunately xc_cpuid_pv_policy doesn't return an error code.  I
> think it needs to.  So that's rather a yak.

I was about to say it's not one worth shaving, but actually although
this returns void it is static and has exactly one caller which can
return errors -- so it's a very easy yakk to shave it seems.

Ian.

Re: [PATCH 1/7] libxc: fix uninitialized variable in xc_cpuid_pv_policy()

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH 1/7] libxc: fix uninitialized variable in xc_cpuid_pv_policy()"):
> On Fri, 2015-07-03 at 16:15 +0100, Ian Jackson wrote:
> > Unfortunately xc_cpuid_pv_policy doesn't return an error code.  I
> > think it needs to.  So that's rather a yak.
> 
> I was about to say it's not one worth shaving, but actually although
> this returns void it is static and has exactly one caller which can
> return errors -- so it's a very easy yakk to shave it seems.

I guess part of my view is that the point of having Coverity spot
anomalies is to find broken code and fix it.

To then look at the issues which we spot as a result of looking at the
code, when prodded by Coverity, and decide to fix only half of the
problem, is to waste the opportunity for improvement presented by
Coverity.  (Or to look at it another way it is a waste of the effort
of setting up Coverity and then wading through the false positives.)

Ian.

Re: [PATCH 1/7] libxc: fix uninitialized variable in xc_cpuid_pv_policy()

[Ian Campbell]

On Fri, 2015-07-03 at 16:47 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH 1/7] libxc: fix uninitialized variable in xc_cpuid_pv_policy()"):
> > On Fri, 2015-07-03 at 16:15 +0100, Ian Jackson wrote:
> > > Unfortunately xc_cpuid_pv_policy doesn't return an error code.  I
> > > think it needs to.  So that's rather a yak.
> > 
> > I was about to say it's not one worth shaving, but actually although
> > this returns void it is static and has exactly one caller which can
> > return errors -- so it's a very easy yakk to shave it seems.
> 
> I guess part of my view is that the point of having Coverity spot
> anomalies is to find broken code and fix it.
> 
> To then look at the issues which we spot as a result of looking at the
> code, when prodded by Coverity, and decide to fix only half of the
> problem, is to waste the opportunity for improvement presented by
> Coverity.  (Or to look at it another way it is a waste of the effort
> of setting up Coverity and then wading through the false positives.)

This is a good way to think of it, thanks.

Ian.

Re: [PATCH 1/7] libxc: fix uninitialized variable in xc_cpuid_pv_policy()

[Andrew Cooper]

On 03/07/15 16:50, Ian Campbell wrote:
> On Fri, 2015-07-03 at 16:47 +0100, Ian Jackson wrote:
>> Ian Campbell writes ("Re: [Xen-devel] [PATCH 1/7] libxc: fix uninitialized variable in xc_cpuid_pv_policy()"):
>>> On Fri, 2015-07-03 at 16:15 +0100, Ian Jackson wrote:
>>>> Unfortunately xc_cpuid_pv_policy doesn't return an error code.  I
>>>> think it needs to.  So that's rather a yak.
>>> I was about to say it's not one worth shaving, but actually although
>>> this returns void it is static and has exactly one caller which can
>>> return errors -- so it's a very easy yakk to shave it seems.
>> I guess part of my view is that the point of having Coverity spot
>> anomalies is to find broken code and fix it.
>>
>> To then look at the issues which we spot as a result of looking at the
>> code, when prodded by Coverity, and decide to fix only half of the
>> problem, is to waste the opportunity for improvement presented by
>> Coverity.  (Or to look at it another way it is a waste of the effort
>> of setting up Coverity and then wading through the false positives.)
> This is a good way to think of it, thanks.

FWIW, I already have "all of cpuid handling everywhere" covered in the
herd of yakks needing shaving for the feature levelling fixes (design
doc already on the list).

~Andrew

[PATCH 2/7] libxc: Use const pointer in local_file_dump()

[Jennifer Herbert]

By adding the const keyword, it is clearer to people and static analysis
tools that no changes to the data are to be made.

Signed-off-by: Jennifer Herbert <jennifer.herbert@citrix.com>
---
 tools/libxc/xc_core.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxc/xc_core.c b/tools/libxc/xc_core.c
index dfa424b..f759fd6 100644
--- a/tools/libxc/xc_core.c
+++ b/tools/libxc/xc_core.c
@@ -926,7 +926,7 @@ struct dump_args {
 static int local_file_dump(xc_interface *xch,
                            void *args, char *buffer, unsigned int length)
 {
-    struct dump_args *da = args;
+    const struct dump_args *da = args;
 
     if ( write_exact(da->fd, buffer, length) == -1 )
     {
-- 
1.7.10.4

Re: [PATCH 2/7] libxc: Use const pointer in local_file_dump()

[Ian Jackson]

Jennifer Herbert writes ("[Xen-devel] [PATCH 2/7] libxc: Use const pointer in local_file_dump()"):
> By adding the const keyword, it is clearer to people and static analysis
> tools that no changes to the data are to be made.

Would it be wrong for a future patch to add a field to dump_args which
gets modified ?  AFAICT the answer is `no'.  So I don't understand why
it ought to be const.

Does your Coverity instance complain about every instance where a
struct exists which is not marked const but which could be ?

Ian.

Re: [PATCH 2/7] libxc: Use const pointer in local_file_dump()

[Jennifer Herbert]

On 03/07/15 16:27, Ian Jackson wrote:
> Jennifer Herbert writes ("[Xen-devel] [PATCH 2/7] libxc: Use const pointer in local_file_dump()"):
>> By adding the const keyword, it is clearer to people and static analysis
>> tools that no changes to the data are to be made.
> Would it be wrong for a future patch to add a field to dump_args which
> gets modified ?  AFAICT the answer is `no'.  So I don't understand why
> it ought to be const.
>
> Does your Coverity instance complain about every instance where a
> struct exists which is not marked const but which could be ?

Coverity see da->fd being passed to local_file_dump, as a modifiable 
entity and concludes  this function may be overwriting this, and end up 
leaking the handle.

It wouldn't be wrong to modify a new field, I just hadn't considered 
that likely. Since maybe it is, I'll just mark as false positive.

-jenny

Re: [PATCH 2/7] libxc: Use const pointer in local_file_dump()

[Ian Jackson]

Jennifer Herbert writes ("Re: [Xen-devel] [PATCH 2/7] libxc: Use const pointer in local_file_dump()"):
> On 03/07/15 16:27, Ian Jackson wrote:
> > Jennifer Herbert writes ("[Xen-devel] [PATCH 2/7] libxc: Use const pointer in local_file_dump()"):
> > Would it be wrong for a future patch to add a field to dump_args which
> > gets modified ?  AFAICT the answer is `no'.  So I don't understand why
> > it ought to be const.
...
> Coverity see da->fd being passed to local_file_dump, as a modifiable 
> entity and concludes  this function may be overwriting this, and end up 
> leaking the handle.

Ah.

> It wouldn't be wrong to modify a new field, I just hadn't considered 
> that likely. Since maybe it is, I'll just mark as false positive.

Right.  I think that's probably best.

Thanks,
Ian.

[PATCH 3/7] libxc: Fix uninitialized valiables in xc_cpuid_hvm_policy()

[Jennifer Herbert]

If xc_hvm_param_get fails, is_pae and/or is_nestedhvm are left undefined.
This patch Indicates error and defaults to false.

Signed-off-by: Jennifer Herbert <jennifer.herbert@citrix.com>
---
 tools/libxc/xc_cpuid_x86.c |   18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index 847b701..cc6f18a 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -268,11 +268,16 @@ static void xc_cpuid_hvm_policy(
 {
     DECLARE_DOMCTL;
     char brand[13];
-    uint64_t val;
-    int is_pae, is_nestedhvm;
+    uint64_t val = 0;
+    int is_pae;
+    int is_nestedhvm = 0;
     uint64_t xfeature_mask;
+    int rc;
+
+    rc = xc_hvm_param_get(xch, domid, HVM_PARAM_PAE_ENABLED, &val);
+    if ( rc < 0 )
+         ERROR("xc_hvm_param_get failed to get PAE_ENABLED with rc %d\n", rc);
 
-    xc_hvm_param_get(xch, domid, HVM_PARAM_PAE_ENABLED, &val);
     is_pae = !!val;
 
     /* Detecting Xen's atitude towards XSAVE */
@@ -282,8 +287,11 @@ static void xc_cpuid_hvm_policy(
     do_domctl(xch, &domctl);
     xfeature_mask = domctl.u.vcpuextstate.xfeature_mask;
 
-    xc_hvm_param_get(xch, domid, HVM_PARAM_NESTEDHVM, &val);
-    is_nestedhvm = !!val;
+    rc = xc_hvm_param_get(xch, domid, HVM_PARAM_NESTEDHVM, &val);
+    if ( rc >= 0 )
+        is_nestedhvm = !!val;
+    else
+        ERROR("xc_hvm_param_get failed to get NESTEDHVM with rc %d\n", rc);
 
     switch ( input[0] )
     {
-- 
1.7.10.4

Re: [PATCH 3/7] libxc: Fix uninitialized valiables in xc_cpuid_hvm_policy()

[Ian Jackson]

Jennifer Herbert writes ("[Xen-devel] [PATCH 3/7] libxc: Fix uninitialized valiables in xc_cpuid_hvm_policy()"):
> If xc_hvm_param_get fails, is_pae and/or is_nestedhvm are left undefined.
> This patch Indicates error and defaults to false.

I think my comments on xc_cpuid_pv_policy apply to this patch too.

Thanks,
Ian.

[PATCH 4/7] libxc: Prevent dereferencing NULL pointers returned from xc_dom_allocate()

[Jennifer Herbert]

The return from xc_dom_allocate is not checked for a NULL value.
This patch fixes this, causing it to return from the function with an error.

Signed-off-by: Jennifer Herbert <jennifer.herbert@citrix.com>
---
 tools/libxc/xc_dom_compat_linux.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/libxc/xc_dom_compat_linux.c b/tools/libxc/xc_dom_compat_linux.c
index 2c14a0f..617cd96 100644
--- a/tools/libxc/xc_dom_compat_linux.c
+++ b/tools/libxc/xc_dom_compat_linux.c
@@ -91,6 +91,8 @@ int xc_linux_build_mem(xc_interface *xch, uint32_t domid,
 
     xc_dom_loginit(xch);
     dom = xc_dom_allocate(xch, cmdline, features);
+    if (dom == NULL)
+        return -1;
     if ( (rc = xc_dom_kernel_mem(dom, image_buffer, image_size)) != 0 )
         goto out;
     if ( initrd && ((rc = xc_dom_ramdisk_mem(dom, initrd, initrd_len)) != 0) )
@@ -123,6 +125,8 @@ int xc_linux_build(xc_interface *xch, uint32_t domid,
 
     xc_dom_loginit(xch);
     dom = xc_dom_allocate(xch, cmdline, features);
+    if (dom == NULL)
+        return -1;
     if ( (rc = xc_dom_kernel_file(dom, image_name)) != 0 )
         goto out;
     if ( initrd_name && strlen(initrd_name) &&
@@ -146,6 +150,8 @@ int xc_get_bit_size(xc_interface *xch,
     int rc;
     *bit_size = 0;
     dom = xc_dom_allocate(xch, cmdline, features);
+    if (dom == NULL)
+        return -1;
     if ( (rc = xc_dom_kernel_file(dom, image_name)) != 0 )
         goto out;
     if ( (rc = xc_dom_parse_image(dom)) != 0 )
-- 
1.7.10.4

Re: [PATCH 4/7] libxc: Prevent dereferencing NULL pointers returned from xc_dom_allocate()

[Ian Jackson]

Jennifer Herbert writes ("[Xen-devel] [PATCH 4/7] libxc: Prevent dereferencing NULL pointers returned from xc_dom_allocate()"):
> The return from xc_dom_allocate is not checked for a NULL value.
> This patch fixes this, causing it to return from the function with an error.

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

Re: [PATCH 4/7] libxc: Prevent dereferencing NULL pointers returned from xc_dom_allocate()

[Ian Campbell]

On Fri, 2015-07-03 at 16:17 +0100, Ian Jackson wrote:
> Jennifer Herbert writes ("[Xen-devel] [PATCH 4/7] libxc: Prevent dereferencing NULL pointers returned from xc_dom_allocate()"):
> > The return from xc_dom_allocate is not checked for a NULL value.
> > This patch fixes this, causing it to return from the function with an error.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Applied.

[PATCH 5/7] libxc: Removing dead code from xc_dom_allocate()

[Jennifer Herbert]

The only place that jumps to 'err:' does so because !dom, which is
rechecked in 'err:'.  This patch simplifies, giving the same result.

Signed-off-by: Jennifer Herbert <jennifer.herbert@citrix.com>
---
 tools/libxc/xc_dom_core.c |    7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
index b100ce1..cde943c 100644
--- a/tools/libxc/xc_dom_core.c
+++ b/tools/libxc/xc_dom_core.c
@@ -720,7 +720,7 @@ struct xc_dom_image *xc_dom_allocate(xc_interface *xch,
                   __FUNCTION__, cmdline, features);
     dom = malloc(sizeof(*dom));
     if ( !dom )
-        goto err;
+        return NULL;
 
     memset(dom, 0, sizeof(*dom));
     dom->xch = xch;
@@ -742,11 +742,6 @@ struct xc_dom_image *xc_dom_allocate(xc_interface *xch,
 
     dom->alloc_malloc += sizeof(*dom);
     return dom;
-
- err:
-    if ( dom )
-        xc_dom_release(dom);
-    return NULL;
 }
 
 int xc_dom_kernel_max_size(struct xc_dom_image *dom, size_t sz)
-- 
1.7.10.4

Re: [PATCH 5/7] libxc: Removing dead code from xc_dom_allocate()

[Andrew Cooper]

On 01/07/15 18:37, Jennifer Herbert wrote:
> The only place that jumps to 'err:' does so because !dom, which is
> rechecked in 'err:'.  This patch simplifies, giving the same result.
>
> Signed-off-by: Jennifer Herbert <jennifer.herbert@citrix.com>

Coverity-ID: 1055188

~Andrew

> ---
>  tools/libxc/xc_dom_core.c |    7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
> index b100ce1..cde943c 100644
> --- a/tools/libxc/xc_dom_core.c
> +++ b/tools/libxc/xc_dom_core.c
> @@ -720,7 +720,7 @@ struct xc_dom_image *xc_dom_allocate(xc_interface *xch,
>                    __FUNCTION__, cmdline, features);
>      dom = malloc(sizeof(*dom));
>      if ( !dom )
> -        goto err;
> +        return NULL;
>  
>      memset(dom, 0, sizeof(*dom));
>      dom->xch = xch;
> @@ -742,11 +742,6 @@ struct xc_dom_image *xc_dom_allocate(xc_interface *xch,
>  
>      dom->alloc_malloc += sizeof(*dom);
>      return dom;
> -
> - err:
> -    if ( dom )
> -        xc_dom_release(dom);
> -    return NULL;
>  }
>  
>  int xc_dom_kernel_max_size(struct xc_dom_image *dom, size_t sz)

Re: [PATCH 5/7] libxc: Removing dead code from xc_dom_allocate()

[Ian Jackson]

Jennifer Herbert writes ("[Xen-devel] [PATCH 5/7] libxc: Removing dead code from xc_dom_allocate()"):
> The only place that jumps to 'err:' does so because !dom, which is
> rechecked in 'err:'.  This patch simplifies, giving the same result.

I'm not particularly convinced by this change, but maybe Ian Campbell
disagrees.

I presume that your Coverity instance is complaining about the fact
that the if (dom) clause's test is always false.  This is true with
the current code, but if this function were to gain any other code it
might stop being true and the first thing to do to get a good error
handling pattern would be to revert this patch.

However: If we're doing teh `initialise everything; single goto for
error paths; dispose of everything' error handling pattern, dom ought
to be initialised to NULL.

Ian C, what do you think ?

Thanks,
Ian.

Re: [PATCH 5/7] libxc: Removing dead code from xc_dom_allocate()

[Ian Campbell]

On Fri, 2015-07-03 at 16:24 +0100, Ian Jackson wrote:
> Jennifer Herbert writes ("[Xen-devel] [PATCH 5/7] libxc: Removing dead code from xc_dom_allocate()"):
> > The only place that jumps to 'err:' does so because !dom, which is
> > rechecked in 'err:'.  This patch simplifies, giving the same result.
> 
> I'm not particularly convinced by this change, but maybe Ian Campbell
> disagrees.
> 
> I presume that your Coverity instance is complaining about the fact
> that the if (dom) clause's test is always false.  This is true with
> the current code, but if this function were to gain any other code it
> might stop being true and the first thing to do to get a good error
> handling pattern would be to revert this patch.

I briefly thought something like this but decided it was under my
threshold for worrying about.

> However: If we're doing teh `initialise everything; single goto for
> error paths; dispose of everything' error handling pattern, dom ought
> to be initialised to NULL.
> 
> Ian C, what do you think ?
> 
> Thanks,
> Ian.

Re: [PATCH 5/7] libxc: Removing dead code from xc_dom_allocate()

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH 5/7] libxc: Removing dead code from xc_dom_allocate()"):
> On Fri, 2015-07-03 at 16:24 +0100, Ian Jackson wrote:
> > Jennifer Herbert writes ("[Xen-devel] [PATCH 5/7] libxc: Removing dead code from xc_dom_allocate()"):
> > > The only place that jumps to 'err:' does so because !dom, which is
> > > rechecked in 'err:'.  This patch simplifies, giving the same result.
> > 
> > I'm not particularly convinced by this change, but maybe Ian Campbell
> > disagrees.
> > 
> > I presume that your Coverity instance is complaining about the fact
> > that the if (dom) clause's test is always false.  This is true with
> > the current code, but if this function were to gain any other code it
> > might stop being true and the first thing to do to get a good error
> > handling pattern would be to revert this patch.
> 
> I briefly thought something like this but decided it was under my
> threshold for worrying about.

I guess my argument is that while this patch makes Coverity happy, it
makes the code slightly worse.  A future programmer will be more
likely to be tempted to simply `return' on error, even when it isn't
right (from this, or other, functions).

Ian.

Re: [PATCH 5/7] libxc: Removing dead code from xc_dom_allocate()

[Ian Campbell]

On Fri, 2015-07-03 at 16:33 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH 5/7] libxc: Removing dead code from xc_dom_allocate()"):
> > On Fri, 2015-07-03 at 16:24 +0100, Ian Jackson wrote:
> > > Jennifer Herbert writes ("[Xen-devel] [PATCH 5/7] libxc: Removing dead code from xc_dom_allocate()"):
> > > > The only place that jumps to 'err:' does so because !dom, which is
> > > > rechecked in 'err:'.  This patch simplifies, giving the same result.
> > > 
> > > I'm not particularly convinced by this change, but maybe Ian Campbell
> > > disagrees.
> > > 
> > > I presume that your Coverity instance is complaining about the fact
> > > that the if (dom) clause's test is always false.  This is true with
> > > the current code, but if this function were to gain any other code it
> > > might stop being true and the first thing to do to get a good error
> > > handling pattern would be to revert this patch.
> > 
> > I briefly thought something like this but decided it was under my
> > threshold for worrying about.
> 
> I guess my argument is that while this patch makes Coverity happy, it
> makes the code slightly worse.  A future programmer will be more
> likely to be tempted to simply `return' on error, even when it isn't
> right (from this, or other, functions).

Yes, true, but we'd probably catch that in review.

I'd be ok with either leaving this as is (and tagging CID 1055188 as
intentioanl) or adding the null initialiser as you suggested in your
"However: "

Ian.

Re: [PATCH 5/7] libxc: Removing dead code from xc_dom_allocate()

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH 5/7] libxc: Removing dead code from xc_dom_allocate()"):
> I'd be ok with either leaving this as is (and tagging CID 1055188 as
> intentioanl) or adding the null initialiser as you suggested in your
> "However: "

I would be happy with either of those.

(I don't think the null initialiser would placate Coverity, but of
course our objective is to make the code right and Coverity is a tool
to help us do that - placating Coverity is not a goal in its own
right.)

Ian.

[PATCH 6/7] libxc: Fix misleading use of strncpy code in build_hvm_info()

[Jennifer Herbert]

hvm_info->signature is not a string, but an 64 bit int, and is not
NULL terminated.  The use of strncpy to populate it is inappropriate and
potentially misleading.  A cursory glance might have you thinking someone
had miscounted the length of the string literal - not realising it was
intentionally cropping of the null termination.
Also, since we wish to initialise all of hvm_info->signature, and
certainly no more, the use of sizeof is safer.

Signed-off-by: Jennifer Herbert <jennifer.herbert@citrix.com>
---
 tools/libxc/xc_hvm_build_x86.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxc/xc_hvm_build_x86.c b/tools/libxc/xc_hvm_build_x86.c
index 003ea06..ec5ef4d 100644
--- a/tools/libxc/xc_hvm_build_x86.c
+++ b/tools/libxc/xc_hvm_build_x86.c
@@ -99,7 +99,7 @@ static void build_hvm_info(void *hvm_info_page,
     memset(hvm_info_page, 0, PAGE_SIZE);
 
     /* Fill in the header. */
-    strncpy(hvm_info->signature, "HVM INFO", 8);
+    memcpy(hvm_info->signature, "HVM INFO", sizeof(hvm_info->signature));
     hvm_info->length = sizeof(struct hvm_info_table);
 
     /* Sensible defaults: these can be overridden by the caller. */
-- 
1.7.10.4

Re: [PATCH 6/7] libxc: Fix misleading use of strncpy code in build_hvm_info()

[Andrew Cooper]

On 01/07/15 18:37, Jennifer Herbert wrote:
> hvm_info->signature is not a string, but an 64 bit int, and is not
> NULL terminated.  The use of strncpy to populate it is inappropriate and
> potentially misleading.  A cursory glance might have you thinking someone
> had miscounted the length of the string literal - not realising it was
> intentionally cropping of the null termination.
> Also, since we wish to initialise all of hvm_info->signature, and
> certainly no more, the use of sizeof is safer.
>
> Signed-off-by: Jennifer Herbert <jennifer.herbert@citrix.com>

Coverity-ID: 1198710

~Andrew

> ---
>  tools/libxc/xc_hvm_build_x86.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/libxc/xc_hvm_build_x86.c b/tools/libxc/xc_hvm_build_x86.c
> index 003ea06..ec5ef4d 100644
> --- a/tools/libxc/xc_hvm_build_x86.c
> +++ b/tools/libxc/xc_hvm_build_x86.c
> @@ -99,7 +99,7 @@ static void build_hvm_info(void *hvm_info_page,
>      memset(hvm_info_page, 0, PAGE_SIZE);
>  
>      /* Fill in the header. */
> -    strncpy(hvm_info->signature, "HVM INFO", 8);
> +    memcpy(hvm_info->signature, "HVM INFO", sizeof(hvm_info->signature));
>      hvm_info->length = sizeof(struct hvm_info_table);
>  
>      /* Sensible defaults: these can be overridden by the caller. */

Re: [PATCH 6/7] libxc: Fix misleading use of strncpy code in build_hvm_info()

[Ian Jackson]

Jennifer Herbert writes ("[Xen-devel] [PATCH 6/7] libxc: Fix misleading use of strncpy code in build_hvm_info()"):
> hvm_info->signature is not a string, but an 64 bit int, and is not
> NULL terminated.  The use of strncpy to populate it is inappropriate and
> potentially misleading.  A cursory glance might have you thinking someone
> had miscounted the length of the string literal - not realising it was
> intentionally cropping of the null termination.
> Also, since we wish to initialise all of hvm_info->signature, and
> certainly no more, the use of sizeof is safer.

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

Re: [PATCH 6/7] libxc: Fix misleading use of strncpy code in build_hvm_info()

[Ian Campbell]

On Fri, 2015-07-03 at 16:25 +0100, Ian Jackson wrote:
> Jennifer Herbert writes ("[Xen-devel] [PATCH 6/7] libxc: Fix misleading use of strncpy code in build_hvm_info()"):
> > hvm_info->signature is not a string, but an 64 bit int, and is not
> > NULL terminated.  The use of strncpy to populate it is inappropriate and
> > potentially misleading.  A cursory glance might have you thinking someone
> > had miscounted the length of the string literal - not realising it was
> > intentionally cropping of the null termination.
> > Also, since we wish to initialise all of hvm_info->signature, and
> > certainly no more, the use of sizeof is safer.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Applied.

[PATCH 7/7] libxc: Prevent NULL pointer dereference in stdiostream_vmessage()

[Jennifer Herbert]

Unlikely that it may seem localtime_r could fail, which would result in a
null pointer dereference.  In this case, one can simply just skip logging the
date/time, and logging anything is more useful then nothing.

Signed-off-by: Jennifer Herbert <jennifer.herbert@citrix.com>
---
 tools/libxc/xtl_logger_stdio.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/tools/libxc/xtl_logger_stdio.c b/tools/libxc/xtl_logger_stdio.c
index d8646e0..74d66a5 100644
--- a/tools/libxc/xtl_logger_stdio.c
+++ b/tools/libxc/xtl_logger_stdio.c
@@ -61,10 +61,11 @@ static void stdiostream_vmessage(xentoollog_logger *logger_in,
         struct tm lt_buf;
         time_t now = time(0);
         struct tm *lt= localtime_r(&now, &lt_buf);
-        fprintf(lg->f, "%04d-%02d-%02d %02d:%02d:%02d %s ",
-                lt->tm_year+1900, lt->tm_mon+1, lt->tm_mday,
-                lt->tm_hour, lt->tm_min, lt->tm_sec,
-                tzname[!!lt->tm_isdst]);
+        if (lt != NULL)
+            fprintf(lg->f, "%04d-%02d-%02d %02d:%02d:%02d %s ",
+                    lt->tm_year+1900, lt->tm_mon+1, lt->tm_mday,
+                    lt->tm_hour, lt->tm_min, lt->tm_sec,
+                    tzname[!!lt->tm_isdst]);
     }
     if (lg->flags & XTL_STDIOSTREAM_SHOW_PID)
         fprintf(lg->f, "[%lu] ", (unsigned long)getpid());
-- 
1.7.10.4

Re: [PATCH 7/7] libxc: Prevent NULL pointer dereference in stdiostream_vmessage()

[Ian Jackson]

Jennifer Herbert writes ("[Xen-devel] [PATCH 7/7] libxc: Prevent NULL pointer dereference in stdiostream_vmessage()"):
> Unlikely that it may seem localtime_r could fail, which would result in a
> null pointer dereference.  In this case, one can simply just skip logging the
> date/time, and logging anything is more useful then nothing.

I think it would be best, in this error case, to explicitly note that
date/time conversion failed, by printing (say) "[localtime_r failed:
<errno>]".  Sadly that will involve calling strerror and doing
something sensible if strerror fails, too.

thanks,
Ian.

Re: [PATCH 7/7] libxc: Prevent NULL pointer dereference in stdiostream_vmessage()

[Ian Campbell]

On Fri, 2015-07-03 at 16:29 +0100, Ian Jackson wrote:
> Jennifer Herbert writes ("[Xen-devel] [PATCH 7/7] libxc: Prevent NULL pointer dereference in stdiostream_vmessage()"):
> > Unlikely that it may seem localtime_r could fail, which would result in a
> > null pointer dereference.  In this case, one can simply just skip logging the
> > date/time, and logging anything is more useful then nothing.
> 
> I think it would be best, in this error case, to explicitly note that
> date/time conversion failed, by printing (say) "[localtime_r failed:
> <errno>]".  Sadly that will involve calling strerror and doing
> something sensible if strerror fails, too.

Perhaps a smaller yakk would be to just include the numeric errno? For
such a rare error case having to worry about two layers of nested error
handling seems annoying...

Ian

Re: [PATCH 7/7] libxc: Prevent NULL pointer dereference in stdiostream_vmessage()

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH 7/7] libxc: Prevent NULL pointer dereference in stdiostream_vmessage()"):
> On Fri, 2015-07-03 at 16:29 +0100, Ian Jackson wrote:
> > I think it would be best, in this error case, to explicitly note that
> > date/time conversion failed, by printing (say) "[localtime_r failed:
> > <errno>]".  Sadly that will involve calling strerror and doing
> > something sensible if strerror fails, too.
> 
> Perhaps a smaller yakk would be to just include the numeric errno? For
> such a rare error case having to worry about two layers of nested error
> handling seems annoying...

That would satisfy me.

Ian.

Re: [PATCH 0/7] libxc: Fix a number of coverity issues.

[Andrew Cooper]

On 01/07/15 18:37, Jennifer Herbert wrote:
> Fix a number of coverity issues in libxc.

Patches 2 through 7:
  Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH 0/7] libxc: Fix a number of coverity issues.

[Ian Campbell]

On Thu, 2015-07-02 at 14:23 +0100, Andrew Cooper wrote:
> On 01/07/15 18:37, Jennifer Herbert wrote:
> > Fix a number of coverity issues in libxc.
> 
> Patches 2 through 7:
>   Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Likewise: Acked-by: Ian Campbell <ian.campbell@citrix.com>, just
awaiting any CID numbers if there are any otherewise I'll commit those
and await a new #1. 

Ian.

Re: [PATCH 0/7] libxc: Fix a number of coverity issues.

[Ian Campbell]

On Wed, 2015-07-01 at 17:37 +0000, Jennifer Herbert wrote:
> Fix a number of coverity issues in libxc.

Do you have CID numbers which should be associated with each of these?

See e.g. d17547414199f4434289f75ab2b1364cc961982e for the style.

Ian.

Re: [PATCH 0/7] libxc: Fix a number of coverity issues.

[Andrew Cooper]

On 03/07/15 15:20, Ian Campbell wrote:
> On Wed, 2015-07-01 at 17:37 +0000, Jennifer Herbert wrote:
>> Fix a number of coverity issues in libxc.
> Do you have CID numbers which should be associated with each of these?
>
> See e.g. d17547414199f4434289f75ab2b1364cc961982e for the style.

These were from the XenServer Coverity instance, so our CIDs are in a
different numberspace to upstream SCAN CIDs.

I will have a look through and see if there are any relevant CIDs to
match up.

~Andrew

Re: [PATCH 0/7] libxc: Fix a number of coverity issues.

[Ian Campbell]

On Fri, 2015-07-03 at 15:22 +0100, Andrew Cooper wrote:
> On 03/07/15 15:20, Ian Campbell wrote:
> > On Wed, 2015-07-01 at 17:37 +0000, Jennifer Herbert wrote:
> >> Fix a number of coverity issues in libxc.
> > Do you have CID numbers which should be associated with each of these?
> >
> > See e.g. d17547414199f4434289f75ab2b1364cc961982e for the style.
> 
> These were from the XenServer Coverity instance, so our CIDs are in a
> different numberspace to upstream SCAN CIDs.

Given this...

> I will have a look through and see if there are any relevant CIDs to
> match up.

... then if you can be bothered please do, but also feel free to decide
not to bother...

Ian.

Re: [PATCH 0/7] libxc: Fix a number of coverity issues.

[Andrew Cooper]

On 03/07/15 15:42, Ian Campbell wrote:
> On Fri, 2015-07-03 at 15:22 +0100, Andrew Cooper wrote:
>> On 03/07/15 15:20, Ian Campbell wrote:
>>> On Wed, 2015-07-01 at 17:37 +0000, Jennifer Herbert wrote:
>>>> Fix a number of coverity issues in libxc.
>>> Do you have CID numbers which should be associated with each of these?
>>>
>>> See e.g. d17547414199f4434289f75ab2b1364cc961982e for the style.
>> These were from the XenServer Coverity instance, so our CIDs are in a
>> different numberspace to upstream SCAN CIDs.
> Given this...
>
>> I will have a look through and see if there are any relevant CIDs to
>> match up.
> ... then if you can be bothered please do, but also feel free to decide
> not to bother...

Done.  Only 3 which match up.  The rest are from having all our knobs
turned up to 11.

~Andrew

Re: [PATCH 0/7] libxc: Fix a number of coverity issues.

[Ian Jackson]

Jennifer Herbert writes ("[PATCH 0/7] libxc: Fix a number of coverity issues."):
> Fix a number of coverity issues in libxc.

Thanks.  Error handling is a pet topic of mine so I will review this
series.

Ian.