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@lists.xen.org
http://lists.xen.org/xen-devel
