Re: [Xen-devel] [PATCH v4 08/34] vmap: Make the while loop less fishy.

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Jan Beulich writes ("Re: [Xen-devel] [PATCH v4 08/34] vmap: Make the while loop 
less fishy."):
> On 17.03.16 at 15:37, <andrew.cooper3@xxxxxxxxxx> wrote:
> > 213 error:
> >         CID 63648: Overflowed constant (INTEGER_OVERFLOW)
> >         7. overflow_const: Decrement (--) operation overflows on operand
> > i, whose value is an unsigned constant, 0.
> > 214    while ( i-- )
> > 215        free_domheap_page(mfn_to_page(mfn_x(mfn[i])));
> > 216    xfree(mfn);
> > 217    return NULL;
> > 
> > By flipping the location of the postfix decrement, the problematic case
> > of getting to error: with i as 0 will not enter the loop, and won't
> > decrement i to UINT32_MAX.
> 
> But (as alluded to before) this is a pretty common cleanup pattern,
> and I really don't see us (a) fix all instances just because Coverity
> complains and (b) avoid introducing any new instances.

I'm inclined to agree.

> > It is arguable as to whether this is a Coverity bug or not.  Unsigned
> > integer overflow is defined under the C spec.  On the other hand, I
> > really don't blame Coverity for raising an issue here saying "did you
> > really mean for this underflow to happen".
> 
> Since this is defined behavior, I personally view it as a Coverity issue.
> Which is not to say that such a warning may not help some people in
> certain cases.

I think this should be marked as a false positive in Coverity.

Coverity ought to be re-educated so that it can see that:
  * The decrement result is defined as ~(unsigned)0
  * So there is no UB at this stage
  * ~0 will be written to i, which is potentially hazardous
  * But this is a dead store so in fact it is harmless

The problem is that Coverity is failing to distinguish this from cases
where an unsigned value is decreased and wraps, and then _the
resulting value with huge magnitude is used_.

These latter situations are often serious security bugs.  But if the
huge value is never used there is clearly no bug.

Ian.

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