| Message ID | 1458064616-23101-9-git-send-email-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
>>> On 15.03.16 at 18:56, <konrad.wilk@oracle.com> 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). I don't think this is the only place we do such, and with this being well defined behavior I also don't see why we need to change any of such uses. (And btw., what does this have to do with the already large xSplice series?) Jan
>>> 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. Jan
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
>>> On 17.03.16 at 15:37, <andrew.cooper3@citrix.com> wrote: > 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. 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
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@citrix.com> 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.
Konrad Rzeszutek Wilk writes ("[PATCH v4 08/34] vmap: Make the while loop less fishy."):
> error:
> - while ( i-- )
> - free_domheap_page(mfn_to_page(mfn_x(mfn[i])));
> + while ( i )
> + free_domheap_page(mfn_to_page(mfn_x(mfn[--i])));
I quite strongly dislike this. It is good practice to keep the loop
control code together where this is reasonably convenient.
I wouldn't quibble on such a stylistic matter (particularly outside my
bailiwick) but (a) I would like to reinforce Jan's position and
(b) it seems worth writing an email as there will be many occurrences.
Ian.
On Thu, Mar 17, 2016 at 4:08 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote: > Konrad Rzeszutek Wilk writes ("[PATCH v4 08/34] vmap: Make the while loop less fishy."): >> error: >> - while ( i-- ) >> - free_domheap_page(mfn_to_page(mfn_x(mfn[i]))); >> + while ( i ) >> + free_domheap_page(mfn_to_page(mfn_x(mfn[--i]))); > > I quite strongly dislike this. It is good practice to keep the loop > control code together where this is reasonably convenient. > > I wouldn't quibble on such a stylistic matter (particularly outside my > bailiwick) but (a) I would like to reinforce Jan's position and > (b) it seems worth writing an email as there will be many occurrences. Since we're taking about general principle (and I've been referred to here from a similar discussion elsewhere [1]), let me weigh in as well. I can see the point of not wanting the decrement to be in the middle of the expression here. But I also entirely agree with Konrad's assessment that this code is likely to be confusing; and the fact that a computer program following a list of rules *developed by professional bug-finders* is confused by this kind of semantics I think supports this assessment. At very least it has the potential to waste a lot of mental energy figuring out why code that looks wrong isn't wrong; and at worst there's a risk that at some point someone will "fix" it incorrectly. The fact that there are already many instances of this pattern in the source tree would be relevant if we expect nobody but people currently familiar with the code to every try to read or modify it. But since on the contrary we hope that others will contribute to the codebase, and even that they may eventually become maintainers, I think there is sense in addressing them, at least as they come up. In my case I've suggested adding a comment to clue people into the fact that the postfix semantics are in operation; I think that balances "reducing cognitive load" with "avoids unnecessarily verbose code". Other options would be things like this: do { i--; [cleanup] } while ( i > 0 ); or while ( i > 0 ) { i--; [cleanup] } The first one I think is the clearest, but neither one are very concise. -George [1] marc.info/?i=<56EBC5E102000078000DE376@prv-mh.provo.novell.com>
>>> On 21.03.16 at 13:04, <George.Dunlap@eu.citrix.com> wrote: > On Thu, Mar 17, 2016 at 4:08 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote: >> Konrad Rzeszutek Wilk writes ("[PATCH v4 08/34] vmap: Make the while loop less fishy."): >>> error: >>> - while ( i-- ) >>> - free_domheap_page(mfn_to_page(mfn_x(mfn[i]))); >>> + while ( i ) >>> + free_domheap_page(mfn_to_page(mfn_x(mfn[--i]))); >> >> I quite strongly dislike this. It is good practice to keep the loop >> control code together where this is reasonably convenient. >> >> I wouldn't quibble on such a stylistic matter (particularly outside my >> bailiwick) but (a) I would like to reinforce Jan's position and >> (b) it seems worth writing an email as there will be many occurrences. > > Since we're taking about general principle (and I've been referred to > here from a similar discussion elsewhere [1]), let me weigh in as > well. > > I can see the point of not wanting the decrement to be in the middle > of the expression here. But I also entirely agree with Konrad's > assessment that this code is likely to be confusing; and the fact that > a computer program following a list of rules *developed by > professional bug-finders* is confused by this kind of semantics I > think supports this assessment. At very least it has the potential to > waste a lot of mental energy figuring out why code that looks wrong > isn't wrong; and at worst there's a risk that at some point someone > will "fix" it incorrectly. > > The fact that there are already many instances of this pattern in the > source tree would be relevant if we expect nobody but people currently > familiar with the code to every try to read or modify it. But since > on the contrary we hope that others will contribute to the codebase, > and even that they may eventually become maintainers, I think there is > sense in addressing them, at least as they come up. Well, if talk was about something really complex here, I might agree. But unary prefix and postfix operators are an integral part of the C language, and while code reading indeed shouldn't require overly much mental energy, I think we should be permitted to make full knowledge of the base programming language a prereq to reading our code. Otherwise - where do you want to draw the boundary of what is permitted and what is not? (Yes, I know I'm guilty in occasionally writing rather complex expressions, with at times not immediately obvious side effects, and I'm trying to do better irrespective of all such also falling in the above "basic language" features category. That's because I can see doing so being past the boundary of reasonably understandable code.) > In my case I've suggested adding a comment to clue people into the > fact that the postfix semantics are in operation; I think that > balances "reducing cognitive load" with "avoids unnecessarily verbose > code". > > Other options would be things like this: > > do { > i--; > [cleanup] > } while ( i > 0 ); > > or > > while ( i > 0 ) { > i--; > [cleanup] > } > > The first one I think is the clearest, but neither one are very concise. But you realize that the first (but not the second) one is wrong for the i == 0 case? Jan
On Mon, Mar 21, 2016 at 1:26 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 21.03.16 at 13:04, <George.Dunlap@eu.citrix.com> wrote: >> On Thu, Mar 17, 2016 at 4:08 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote: >>> Konrad Rzeszutek Wilk writes ("[PATCH v4 08/34] vmap: Make the while loop less fishy."): >>>> error: >>>> - while ( i-- ) >>>> - free_domheap_page(mfn_to_page(mfn_x(mfn[i]))); >>>> + while ( i ) >>>> + free_domheap_page(mfn_to_page(mfn_x(mfn[--i]))); >>> >>> I quite strongly dislike this. It is good practice to keep the loop >>> control code together where this is reasonably convenient. >>> >>> I wouldn't quibble on such a stylistic matter (particularly outside my >>> bailiwick) but (a) I would like to reinforce Jan's position and >>> (b) it seems worth writing an email as there will be many occurrences. >> >> Since we're taking about general principle (and I've been referred to >> here from a similar discussion elsewhere [1]), let me weigh in as >> well. >> >> I can see the point of not wanting the decrement to be in the middle >> of the expression here. But I also entirely agree with Konrad's >> assessment that this code is likely to be confusing; and the fact that >> a computer program following a list of rules *developed by >> professional bug-finders* is confused by this kind of semantics I >> think supports this assessment. At very least it has the potential to >> waste a lot of mental energy figuring out why code that looks wrong >> isn't wrong; and at worst there's a risk that at some point someone >> will "fix" it incorrectly. >> >> The fact that there are already many instances of this pattern in the >> source tree would be relevant if we expect nobody but people currently >> familiar with the code to every try to read or modify it. But since >> on the contrary we hope that others will contribute to the codebase, >> and even that they may eventually become maintainers, I think there is >> sense in addressing them, at least as they come up. > > Well, if talk was about something really complex here, I might > agree. But unary prefix and postfix operators are an integral > part of the C language, and while code reading indeed shouldn't > require overly much mental energy, I think we should be > permitted to make full knowledge of the base programming > language a prereq to reading our code. Otherwise - where do > you want to draw the boundary of what is permitted and what > is not? (Yes, I know I'm guilty in occasionally writing rather > complex expressions, with at times not immediately obvious side > effects, and I'm trying to do better irrespective of all such also > falling in the above "basic language" features category. That's > because I can see doing so being past the boundary of > reasonably understandable code.) And I'm not saying that we can't take advantage of postfix operators in this case; I'm just saying that if we are going to, it would be better to point it out in a comment. There are lots of "features" of C that standard practice still demands that we add a comment for; the default fall-through for switch statements comes to mind. Of course programmers should be required to know that switch statements default to fallthrough in C; but it's still a common mistake to forget that, and so we point them in the right direction by adding comments. Similarly, of course programmers should know the difference between prefix and postfix operators. But this is a case where there's a risk of tripping over something, so it makes sense to point them in the right direction by adding comments. Or to take a different tack: I understand that you don't think there's no particular benefit to adding a comment in cases like this; could you explain to me why you think it would have a significant cost? >> do { >> i--; >> [cleanup] >> } while ( i > 0 ); >> > > But you realize that the first (but not the second) one is wrong for > the i == 0 case? So it is. I think I was thinking of a case where it was known that at least one iteration had succeeded; but obviously the second is more safe in general. -George
>>> On 21.03.16 at 15:22, <George.Dunlap@eu.citrix.com> wrote: > Or to take a different tack: I understand that you don't think there's > no particular benefit to adding a comment in cases like this; could > you explain to me why you think it would have a significant cost? There's no significant cost here. Yet I do think that commenting the obvious is not really helpful (or else we end up with more comments than there is actual code; some may consider this a good thing, but I'm of the opinion that this would only serve obfuscating the code). Plus - as said in various other contexts - I'm in favor of consistency, i.e. I would think that if these constructs warrant a comment, all of them should get one. The fall through example you gave for switch statements is actually a good one: There the comments silence Coverity. If comments here had the same effect, I think I'd have no reservations against adding such, yet I don't think they do. In the end it boils down to: I'm not going to ack any such change myself, but I'm also not going to stand in the way of other maintainers ack-ing such comments getting added. What I would object to though is calling such constructs "fishy" in commit titles, commit messages, or the comments being added. Jan
diff --git a/xen/common/vmap.c b/xen/common/vmap.c index c57239f..be01285 100644 --- a/xen/common/vmap.c +++ b/xen/common/vmap.c @@ -246,8 +246,8 @@ void *vmalloc(size_t size) return va; error: - while ( i-- ) - free_domheap_page(mfn_to_page(mfn_x(mfn[i]))); + while ( i ) + free_domheap_page(mfn_to_page(mfn_x(mfn[--i]))); xfree(mfn); return NULL; }
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> --- Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Keir Fraser <keir@xen.org> Cc: Tim Deegan <tim@xen.org> --- --- xen/common/vmap.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)