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

[Wei Liu <wei.liu2@citrix.com>]

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
>