From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v4 08/34] vmap: Make the while loop less fishy. Date: Thu, 17 Mar 2016 14:37:10 +0000 Message-ID: <56EAC116.7040907@citrix.com> References: <1458064616-23101-1-git-send-email-konrad.wilk@oracle.com> <1458064616-23101-9-git-send-email-konrad.wilk@oracle.com> <56E863A7.1020201@citrix.com> <56EAA7E402000078000DDCC5@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============5161910578662587208==" Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1agZ36-0001yW-0f for xen-devel@lists.xenproject.org; Thu, 17 Mar 2016 14:37:16 +0000 In-Reply-To: <56EAA7E402000078000DDCC5@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Jan Beulich Cc: Keir Fraser , Ian Jackson , Tim Deegan , mpohlack@amazon.de, ross.lagerwall@citrix.com, xen-devel@lists.xenproject.org, sasha.levin@oracle.com List-Id: xen-devel@lists.xenproject.org --===============5161910578662587208== Content-Type: multipart/alternative; boundary="------------030203070605050205030408" --------------030203070605050205030408 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit On 17/03/16 11:49, Jan Beulich wrote: >>>> On 15.03.16 at 20:33, 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 >> Reviewed-by: Andrew Cooper >> >> 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 --------------030203070605050205030408 Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: 8bit

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

On 15.03.16 at 20:33, <andrew.cooper3@citrix.com> 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@oracle.com>

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

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
--------------030203070605050205030408-- --===============5161910578662587208== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwOi8vbGlzdHMueGVuLm9y Zy94ZW4tZGV2ZWwK --===============5161910578662587208==--