Coverity tidying

[Joshua Otto <jtotto@uwaterloo.ca>]

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