[PATCH v2] libxl: fix handling of returns in libxl_get_version_info()

[PATCH v2] libxl: fix handling of returns in libxl_get_version_info()

[Harmandeep Kaur]

Check the return value of xc_version() and return NULL if it
fails. libxl_get_version_info() can also return NULL now.
Callers of the function libxl_get_version_info() are already
prepared to deal with returning NULL on failure of xc_version().

Coverity ID 1351217

Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com>
---
v2: Change local variable rc to r. Remove xen_version.
    Better readiblity of blocks of code.
---
 tools/libxl/libxl.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 2d18b8d..771cc40 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5267,42 +5267,50 @@ const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx)
         xen_platform_parameters_t p_parms;
         xen_commandline_t xen_commandline;
     } u;
-    long xen_version;
+    long r = 0;
     libxl_version_info *info = &ctx->version_info;
 
     if (info->xen_version_extra != NULL)
         goto out;
 
-    xen_version = xc_version(ctx->xch, XENVER_version, NULL);
-    info->xen_version_major = xen_version >> 16;
-    info->xen_version_minor = xen_version & 0xFF;
+    r = xc_version(ctx->xch, XENVER_version, NULL);
+    if ( r < 0 ) goto out;
+    info->xen_version_major = r >> 16;
+    info->xen_version_minor = r & 0xFF;
 
-    xc_version(ctx->xch, XENVER_extraversion, &u.xen_extra);
+    r = xc_version(ctx->xch, XENVER_extraversion, &u.xen_extra);
+    if ( r < 0 ) goto out;
     info->xen_version_extra = libxl__strdup(NOGC, u.xen_extra);
 
-    xc_version(ctx->xch, XENVER_compile_info, &u.xen_cc);
+    r = xc_version(ctx->xch, XENVER_compile_info, &u.xen_cc);
+    if ( r < 0 ) goto out;
     info->compiler = libxl__strdup(NOGC, u.xen_cc.compiler);
     info->compile_by = libxl__strdup(NOGC, u.xen_cc.compile_by);
     info->compile_domain = libxl__strdup(NOGC, u.xen_cc.compile_domain);
     info->compile_date = libxl__strdup(NOGC, u.xen_cc.compile_date);
 
-    xc_version(ctx->xch, XENVER_capabilities, &u.xen_caps);
+    r = xc_version(ctx->xch, XENVER_capabilities, &u.xen_caps);
+    if ( r < 0 ) goto out;
     info->capabilities = libxl__strdup(NOGC, u.xen_caps);
 
-    xc_version(ctx->xch, XENVER_changeset, &u.xen_chgset);
+    r = xc_version(ctx->xch, XENVER_changeset, &u.xen_chgset);
+    if ( r < 0 ) goto out;
     info->changeset = libxl__strdup(NOGC, u.xen_chgset);
 
-    xc_version(ctx->xch, XENVER_platform_parameters, &u.p_parms);
+    r = xc_version(ctx->xch, XENVER_platform_parameters, &u.p_parms);
+    if ( r < 0 ) goto out;
     info->virt_start = u.p_parms.virt_start;
 
-    info->pagesize = xc_version(ctx->xch, XENVER_pagesize, NULL);
+    r = info->pagesize = xc_version(ctx->xch, XENVER_pagesize, NULL);
+    if ( r < 0 ) goto out;
 
-    xc_version(ctx->xch, XENVER_commandline, &u.xen_commandline);
+    r = xc_version(ctx->xch, XENVER_commandline, &u.xen_commandline);
+    if ( r < 0 ) goto out;
     info->commandline = libxl__strdup(NOGC, u.xen_commandline);
 
  out:
     GC_FREE;
-    return info;
+    return r < 0 ? NULL:info;
 }
 
 libxl_vcpuinfo *libxl_list_vcpu(libxl_ctx *ctx, uint32_t domid,
-- 
2.5.0

Re: [PATCH v2] libxl: fix handling of returns in libxl_get_version_info()

[Wei Liu]

On Fri, Feb 12, 2016 at 05:00:40PM +0530, Harmandeep Kaur wrote:
> Check the return value of xc_version() and return NULL if it
> fails. libxl_get_version_info() can also return NULL now.
> Callers of the function libxl_get_version_info() are already
> prepared to deal with returning NULL on failure of xc_version().
> 
> Coverity ID 1351217
> 
> Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com>
> ---
> v2: Change local variable rc to r. Remove xen_version.
>     Better readiblity of blocks of code.
> ---
>  tools/libxl/libxl.c | 32 ++++++++++++++++++++------------
>  1 file changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 2d18b8d..771cc40 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -5267,42 +5267,50 @@ const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx)
>          xen_platform_parameters_t p_parms;
>          xen_commandline_t xen_commandline;
>      } u;
> -    long xen_version;
> +    long r = 0;
>      libxl_version_info *info = &ctx->version_info;
>  
>      if (info->xen_version_extra != NULL)
>          goto out;
>  
> -    xen_version = xc_version(ctx->xch, XENVER_version, NULL);
> -    info->xen_version_major = xen_version >> 16;
> -    info->xen_version_minor = xen_version & 0xFF;
> +    r = xc_version(ctx->xch, XENVER_version, NULL);
> +    if ( r < 0 ) goto out;

I know you're following Ian's suggestion, but examples in CODING_STYLE
don't have space after "(" and before ")".


> +    info->xen_version_major = r >> 16;
> +    info->xen_version_minor = r & 0xFF;
>  
> -    xc_version(ctx->xch, XENVER_extraversion, &u.xen_extra);
> +    r = xc_version(ctx->xch, XENVER_extraversion, &u.xen_extra);
> +    if ( r < 0 ) goto out;
>      info->xen_version_extra = libxl__strdup(NOGC, u.xen_extra);
>  
> -    xc_version(ctx->xch, XENVER_compile_info, &u.xen_cc);
> +    r = xc_version(ctx->xch, XENVER_compile_info, &u.xen_cc);
> +    if ( r < 0 ) goto out;

At the beginning of this function it checks if info->xen_version_extra
is not NULL.

You can now get into a state where partial information is cached. This
is buggy.

Not that the original implementation is any better, but if you're going
to fix it, try not to introduce new bug with your fix. :-)

I think you can rollback the caching by freeing up any resources before
returning.

Wei.

>      info->compiler = libxl__strdup(NOGC, u.xen_cc.compiler);
>      info->compile_by = libxl__strdup(NOGC, u.xen_cc.compile_by);
>      info->compile_domain = libxl__strdup(NOGC, u.xen_cc.compile_domain);
>      info->compile_date = libxl__strdup(NOGC, u.xen_cc.compile_date);
>  
> -    xc_version(ctx->xch, XENVER_capabilities, &u.xen_caps);
> +    r = xc_version(ctx->xch, XENVER_capabilities, &u.xen_caps);
> +    if ( r < 0 ) goto out;
>      info->capabilities = libxl__strdup(NOGC, u.xen_caps);
>  
> -    xc_version(ctx->xch, XENVER_changeset, &u.xen_chgset);
> +    r = xc_version(ctx->xch, XENVER_changeset, &u.xen_chgset);
> +    if ( r < 0 ) goto out;
>      info->changeset = libxl__strdup(NOGC, u.xen_chgset);
>  
> -    xc_version(ctx->xch, XENVER_platform_parameters, &u.p_parms);
> +    r = xc_version(ctx->xch, XENVER_platform_parameters, &u.p_parms);
> +    if ( r < 0 ) goto out;
>      info->virt_start = u.p_parms.virt_start;
>  
> -    info->pagesize = xc_version(ctx->xch, XENVER_pagesize, NULL);
> +    r = info->pagesize = xc_version(ctx->xch, XENVER_pagesize, NULL);
> +    if ( r < 0 ) goto out;
>  
> -    xc_version(ctx->xch, XENVER_commandline, &u.xen_commandline);
> +    r = xc_version(ctx->xch, XENVER_commandline, &u.xen_commandline);
> +    if ( r < 0 ) goto out;
>      info->commandline = libxl__strdup(NOGC, u.xen_commandline);
>  
>   out:
>      GC_FREE;
> -    return info;
> +    return r < 0 ? NULL:info;
>  }
>  
>  libxl_vcpuinfo *libxl_list_vcpu(libxl_ctx *ctx, uint32_t domid,
> -- 
> 2.5.0
> 

Re: [PATCH v2] libxl: fix handling of returns in libxl_get_version_info()

[Dario Faggioli]

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

On Fri, 2016-02-12 at 12:31 +0000, Wei Liu wrote:
> On Fri, Feb 12, 2016 at 05:00:40PM +0530, Harmandeep Kaur wrote:
> > 
> > +    info->xen_version_major = r >> 16;
> > +    info->xen_version_minor = r & 0xFF;
> >  
> > -    xc_version(ctx->xch, XENVER_extraversion, &u.xen_extra);
> > +    r = xc_version(ctx->xch, XENVER_extraversion, &u.xen_extra);
> > +    if ( r < 0 ) goto out;
> >      info->xen_version_extra = libxl__strdup(NOGC, u.xen_extra);
> >  
> > -    xc_version(ctx->xch, XENVER_compile_info, &u.xen_cc);
> > +    r = xc_version(ctx->xch, XENVER_compile_info, &u.xen_cc);
> > +    if ( r < 0 ) goto out;
> 
> At the beginning of this function it checks if info-
> >xen_version_extra
> is not NULL.
> 
> You can now get into a state where partial information is cached.
> This
> is buggy.
> 
Yep, I saw this, and figured out it is not ideal. I thought that, as
you say, original code was bad in this respect already, and that we
should fix that independently. However...

> Not that the original implementation is any better, but if you're
> going
> to fix it, try not to introduce new bug with your fix. :-)
> 
> I think you can rollback the caching by freeing up any resources
> before
> returning.
> 
...you're right, it's probably simple enough to fix both issues, that
we should just take the chance.

So, Harmandeep, can you take care of this issue Wei is rising as well?

Thanks and Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


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

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

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

Re: [PATCH v2] libxl: fix handling of returns in libxl_get_version_info()

[Harmandeep Kaur]

On Fri, Feb 12, 2016 at 6:39 PM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> On Fri, 2016-02-12 at 12:31 +0000, Wei Liu wrote:
>> On Fri, Feb 12, 2016 at 05:00:40PM +0530, Harmandeep Kaur wrote:
>> >
>> > +    info->xen_version_major = r >> 16;
>> > +    info->xen_version_minor = r & 0xFF;
>> >
>> > -    xc_version(ctx->xch, XENVER_extraversion, &u.xen_extra);
>> > +    r = xc_version(ctx->xch, XENVER_extraversion, &u.xen_extra);
>> > +    if ( r < 0 ) goto out;
>> >      info->xen_version_extra = libxl__strdup(NOGC, u.xen_extra);
>> >
>> > -    xc_version(ctx->xch, XENVER_compile_info, &u.xen_cc);
>> > +    r = xc_version(ctx->xch, XENVER_compile_info, &u.xen_cc);
>> > +    if ( r < 0 ) goto out;
>>
>> At the beginning of this function it checks if info-
>> >xen_version_extra
>> is not NULL.
>>
>> You can now get into a state where partial information is cached.
>> This
>> is buggy.
>>
> Yep, I saw this, and figured out it is not ideal. I thought that, as
> you say, original code was bad in this respect already, and that we
> should fix that independently. However...
>
>> Not that the original implementation is any better, but if you're
>> going
>> to fix it, try not to introduce new bug with your fix. :-)
>>
>> I think you can rollback the caching by freeing up any resources
>> before
>> returning.
>>
> ...you're right, it's probably simple enough to fix both issues, that
> we should just take the chance.
>
> So, Harmandeep, can you take care of this issue Wei is rising as well?

Posted v3: http://lists.xenproject.org/archives/html/xen-devel/2016-02/msg01891.html

Thanks and Regards.

> Thanks and Regards,
> Dario
> --
> <<This happens because I choose it to happen!>> (Raistlin Majere)
> -----------------------------------------------------------------
> Dario Faggioli, Ph.D, http://about.me/dario.faggioli
> Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
>