| Message ID | 1455276640-3735-1-git-send-email-write.harmandeep@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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
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) >
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,
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(-)