libxl: fix handling of returns in libxl_get_version_info()

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 <[email protected]>

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

tools/libxl/libxl.c +1 -1

index 2d18b8d..771cc40 100644
@@ -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;

tools/libxl/libxl.c +11 -6

     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,

Signature · 1 line

2.5.0

Quoted 4 lines · Harmandeep Kaur

+    info->xen_version_major = r >> 16;
+    info->xen_version_minor = r & 0xFF;

Quoted 8 lines

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

Quoted 1 line

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.

Quoted 14 lines · Wei Liu

tools/libxl/libxl.c const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx) · line 5286

Quoted 12 lines · Harmandeep Kaur

tools/libxl/libxl.c +6 -2

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

Quoted 8 lines

At the beginning of this function it checks if info-

Quoted 1 line

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...

Quoted 4 lines

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. :-)

Quoted 4 lines

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)