[Xen-devel] Coverity tidying

[Joshua Otto <jtotto@xxxxxxxxxxxx>]

On Mon, Dec 14, 2015 at 11:08:43AM +0000, Ian Campbell wrote:
> On Sat, 2015-12-12 at 17:07 -0500, Joshua Otto wrote:
> > On Fri, Dec 11, 2015 at 01:52:41PM +0000, Ian Campbell wrote:
> > > Cool! Just to be clear, you are looking for one project for the 3 of
> > > you to
> > > work on as a group (vs 3 individual projects), is that right?
> > 
> > Yes, that's right.
> > 
> > > It's been a while since there has been a scan run, I did one yesterday but
> > > it is taking an unusually long time to get the results back. Hopefully
> > > we'll have an up to date set of defects early next week and I can have a
> > > scrobble around for some interesting ones for you guys to take a look at.
> > 
> > That would be perfect, thanks!
> 
> Results are in. I've cherry-picked a few of the new issues below. I've not
> checked carefully for false +ves.
> 
> Not a great deal of massive thrills in there, but some one liners etc to
> dip your toes in I guess.

These patches address the Coverity scan issues identified below that appear to
be actual problems.  For issues that we believe to be false positives, we
briefly explain why.

We've attempted to CC maintainers according to get_maintainer.pl.

> ________________________________________________________________________________________________________
> *** CID 1343310:ÂÂCode maintainability issuesÂÂ(UNUSED_VALUE)
> /xen/arch/x86/hvm/svm/intr.c: 95 in svm_enable_intr_window()
> 89ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂstruct vmcb_struct *gvmcb = nv->nv_vvmcx;
> 90ÂÂÂÂÂ
> 91ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ/* check if l1 guest injects interrupt into l2 guest via 
> vintr.
> 92ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ* return here or l2 guest looses interrupts, otherwise.
> 93ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ*/
> 94ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂASSERT(gvmcb != NULL);
> >>>ÂÂÂÂÂCID 1343310:ÂÂCode maintainability issuesÂÂ(UNUSED_VALUE)
> >>>ÂÂÂÂÂAssigning value from "vmcb_get_vintr(gvmcb)" to "intr" here, but that 
> >>>stored value is overwritten before it can be used.
> 95ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂintr = vmcb_get_vintr(gvmcb);
> 96ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif ( intr.fields.irq )
> 97ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn;
> 98ÂÂÂÂÂÂÂÂÂÂÂÂÂ}
> 99ÂÂÂÂÂÂÂÂÂ}
> 100 Â ÂÂ

intr is used on the next line, so this appears to be a false positive without an
obvious rephrasing that Coverity would accept.

> ________________________________________________________________________________________________________
> *** CID 1343309:ÂÂControl flow issuesÂÂ(UNREACHABLE)
> /tools/libxl/libxl.c: 5575 in libxl_get_scheduler()
> 5569ÂÂÂÂÂ{
> 5570ÂÂÂÂÂÂÂÂÂlibxl_scheduler sched, ret;
> 5571ÂÂÂÂÂÂÂÂÂGC_INIT(ctx);
> 5572ÂÂÂÂÂÂÂÂÂif ((ret = xc_sched_id(ctx->xch, (int *)&sched)) != 0) {
> 5573ÂÂÂÂÂÂÂÂÂÂÂÂÂLOGE(ERROR, "getting domain info list");
> 5574ÂÂÂÂÂÂÂÂÂÂÂÂÂreturn ERROR_FAIL;
> >>>ÂÂÂÂÂCID 1343309:ÂÂControl flow issuesÂÂ(UNREACHABLE)
> >>>ÂÂÂÂÂThis code cannot be reached: "libxl__free_all(gc);".
> 5575ÂÂÂÂÂÂÂÂÂÂÂÂÂGC_FREE;
> 5576ÂÂÂÂÂÂÂÂÂ}
> 5577ÂÂÂÂÂÂÂÂÂGC_FREE;
> 5578ÂÂÂÂÂÂÂÂÂreturn sched;
> 5579ÂÂÂÂÂ}
> 5580 Â ÂÂ
>
> As well as putting GC_FREE in the right place this function could be
> reworked to follow the recommendations in tools/libxl/CODING_STYLE.

This issue is addressed by patches 1 and 2.

> ** CID 1343307:ÂÂÂÂ(RESOURCE_LEAK)
> /tools/libxl/libxl_dm.c: 746 in libxl__dm_runas_helper()
> /tools/libxl/libxl_dm.c: 748 in libxl__dm_runas_helper()
> /tools/libxl/libxl_dm.c: 749 in libxl__dm_runas_helper()
>
>
> ________________________________________________________________________________________________________
> *** CID 1343307:ÂÂÂÂ(RESOURCE_LEAK)
> /tools/libxl/libxl_dm.c: 746 in libxl__dm_runas_helper()
> 740ÂÂÂÂÂÂÂÂÂÂÂÂÂret = getpwnam_r(username, &pwd, buf, buf_size, &user);
> 741ÂÂÂÂÂÂÂÂÂÂÂÂÂif (ret == ERANGE) {
> 742ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbuf_size += 128;
> 743ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcontinue;
> 744ÂÂÂÂÂÂÂÂÂÂÂÂÂ}
> 745ÂÂÂÂÂÂÂÂÂÂÂÂÂif (ret != 0)
> >>>ÂÂÂÂÂCID 1343307:ÂÂÂÂ(RESOURCE_LEAK)
> >>>ÂÂÂÂÂVariable "buf" going out of scope leaks the storage it points to.
> 746ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn ERROR_FAIL;
> 747ÂÂÂÂÂÂÂÂÂÂÂÂÂif (user != NULL)
> 748ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn 1;
> 749ÂÂÂÂÂÂÂÂÂÂÂÂÂreturn 0;
> 750ÂÂÂÂÂÂÂÂÂ}
> 751ÂÂÂÂÂ}
> /tools/libxl/libxl_dm.c: 748 in libxl__dm_runas_helper()
> 742ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbuf_size += 128;
> 743ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcontinue;
> 744ÂÂÂÂÂÂÂÂÂÂÂÂÂ}
> 745ÂÂÂÂÂÂÂÂÂÂÂÂÂif (ret != 0)
> 746ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn ERROR_FAIL;
> 747ÂÂÂÂÂÂÂÂÂÂÂÂÂif (user != NULL)
> >>>ÂÂÂÂÂCID 1343307:ÂÂÂÂ(RESOURCE_LEAK)
> >>>ÂÂÂÂÂVariable "buf" going out of scope leaks the storage it points to.
> 748ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn 1;
> 749ÂÂÂÂÂÂÂÂÂÂÂÂÂreturn 0;
> 750ÂÂÂÂÂÂÂÂÂ}
> 751ÂÂÂÂÂ}
> 752ÂÂÂÂÂ
> 753ÂÂÂÂÂstatic int libxl__build_device_model_args_new(libxl__gc *gc,
> /tools/libxl/libxl_dm.c: 749 in libxl__dm_runas_helper()
> 743ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcontinue;
> 744ÂÂÂÂÂÂÂÂÂÂÂÂÂ}
> 745ÂÂÂÂÂÂÂÂÂÂÂÂÂif (ret != 0)
> 746ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn ERROR_FAIL;
> 747ÂÂÂÂÂÂÂÂÂÂÂÂÂif (user != NULL)
> 748ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn 1;
> >>>ÂÂÂÂÂCID 1343307:ÂÂÂÂ(RESOURCE_LEAK)
> >>>ÂÂÂÂÂVariable "buf" going out of scope leaks the storage it points to.
> 749ÂÂÂÂÂÂÂÂÂÂÂÂÂreturn 0;
> 750ÂÂÂÂÂÂÂÂÂ}
> 751ÂÂÂÂÂ}
> 752ÂÂÂÂÂ
> 753ÂÂÂÂÂstatic int libxl__build_device_model_args_new(libxl__gc *gc,
> 754ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂconst char *dm, int 
> guest_domid,

This appears to be a false positive - libxl__realloc() ensures that any
new allocations are added to the gc, and that subsequent reallocations
bring the gc up to date, so exiting the function at any time should be
safe.

> *** CID 1343302:ÂÂInteger handling issuesÂÂ(OVERFLOW_BEFORE_WIDEN)
> /xen/drivers/char/ns16550.c: 916 in pci_uart_config()
> 910ÂÂÂÂÂ
> 911ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂp = uart_config[i].param;
> 912ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ/*
> 913ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ* Force length of mmio region to be at least
> 914ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ* 8 bytes times (1 << reg_shift)
> 915ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ*/
> >>>ÂÂÂÂÂCID 1343302:ÂÂInteger handling issuesÂÂ(OVERFLOW_BEFORE_WIDEN)
> >>>ÂÂÂÂÂPotentially overflowing expression "1 << uart_param[p].reg_shift" 
> >>>with type "int" (32 bits, signed) is evaluated using 32-bit arithmetic, 
> >>>and then used in a context that expects an expression of type "u64" (64 
> >>>bits, unsigned).
> 916ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif ( size < (0x8 * (1 << 
> uart_param[p].reg_shift)) )
> 917ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcontinue;
> 918ÂÂÂÂÂ
> 919ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif ( bar_idx >= uart_param[p].max_bars )
> 920ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcontinue;
> 921ÂÂÂÂÂ

This should be fine - reg_shift never exceeds 2, so there's no need to
worry about overflow.  However, if it's possible that reg_shift might in the
future have a value that causes overflow it could be addressed by applying our
third patch.

> ________________________________________________________________________________________________________
> *** CID 1343301:ÂÂIncorrect expressionÂÂ(NO_EFFECT)
> /xen/common/sched_credit.c: 1795 in csched_dump_pcpu()
> 1789ÂÂÂÂÂcsched_dump_pcpu(const struct scheduler *ops, int cpu)
> 1790ÂÂÂÂÂ{
> 1791ÂÂÂÂÂÂÂÂÂstruct list_head *runq, *iter;
> 1792ÂÂÂÂÂÂÂÂÂstruct csched_private *prv = CSCHED_PRIV(ops);
> 1793ÂÂÂÂÂÂÂÂÂstruct csched_pcpu *spc;
> 1794ÂÂÂÂÂÂÂÂÂstruct csched_vcpu *svc;
> >>>ÂÂÂÂÂCID 1343301:ÂÂIncorrect expressionÂÂ(NO_EFFECT)
> >>>ÂÂÂÂÂAssignment operation "lock = lock" has no effect.
> 1795ÂÂÂÂÂÂÂÂÂspinlock_t *lock = lock;
> 1796ÂÂÂÂÂÂÂÂÂunsigned long flags;
> 1797ÂÂÂÂÂÂÂÂÂint loop;
> 1798ÂÂÂÂÂ#define cpustr keyhandler_scratch
> 1799ÂÂÂÂÂ
> 1800ÂÂÂÂÂÂÂÂÂ/*

This is addressed by the fourth patch.

> *** CID 1343299:ÂÂIncorrect expressionÂÂ(MIXED_ENUMS)
> /tools/libxl/libxl_psr.c: 313 in libxl_psr_cat_set_cbm()
> 307ÂÂÂÂÂÂÂÂÂÂÂÂÂgoto out;
> 308ÂÂÂÂÂÂÂÂÂ}
> 309ÂÂÂÂÂ
> 310ÂÂÂÂÂÂÂÂÂlibxl_for_each_set_bit(socketid, *target_map) {
> 311ÂÂÂÂÂÂÂÂÂÂÂÂÂif (socketid >= nr_sockets)
> 312ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbreak;
> >>>ÂÂÂÂÂCID 1343299:ÂÂIncorrect expressionÂÂ(MIXED_ENUMS)
> >>>ÂÂÂÂÂMixing enum types "enum libxl_psr_cbm_type" and "enum 
> >>>xc_psr_cat_type" for "type".
> 313ÂÂÂÂÂÂÂÂÂÂÂÂÂif (xc_psr_cat_set_domain_data(ctx->xch, domid, type, 
> socketid, cbm)) {
> 314ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂlibxl__psr_cat_log_err_msg(gc, errno);
> 315ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂrc = ERROR_FAIL;
> 316ÂÂÂÂÂÂÂÂÂÂÂÂÂ}
> 317ÂÂÂÂÂÂÂÂÂ}
> 318ÂÂÂÂÂ

The enum types being mixed are identical - one lives in the libxl types
IDL and the other in libxc.  Our fifth patch introduces an explicit conversion,
in case this style is preferable.

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