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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 17/03/16 11:49, Jan Beulich wrote:

On 15.03.16 at 20:33, <andrew.cooper3@xxxxxxxxxx> wrote:

On 15/03/16 17:56, Konrad Rzeszutek Wilk wrote:

It looks like it could underflow at first glance. That is
if i is zero and you get in the while loop with the
i--. However the postfix _expression_ is evaluated after the
conditional so the loop is fine and won't execute (with i==0).

However in spirit of defense programming lets clarify
the loop conditional.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>

Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

This looks as if it will quieten Coverity, even though it is no
functional change.

Quieten Coverity? In what way? And why would it complain in
the first place? As just in reply to Konrad, this is well defined
behavior.

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.

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".

~Andrew

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