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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.03.16 at 15:37, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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.

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.

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

Jan


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