[patch] Warn on memcmp(a, b, sizeof(a) != 0) & co

[Alp Toker]

On 21/12/2013 07:31, Nico Weber wrote:
> Hi,
>
> the attached patch adds a new warning that makes memcmp & co warn on 
> misplaced parentheses such as
>
>   if (memcmp(a, b, sizeof(a) != 0))
>
> like so:
>
> test4.cc:5:30: warning: size argument in 'memcmp' call is a comparison 
> [-Wmemsize-comparison]
>   if (memcmp(a, b, sizeof(a) != 0))
>                    ~~~~~~~~~~^~~~


Hi Nico,

Agree we'd do well to diagnose here, but it seems can get a lot more 
mileage by simplifying and generalizing the check.

This doesn't appear to be a memsize-specific problem, nor is it 
necessarily limited to binop argument expressions.

Surely it's rather the case that any implicit bool-to-size_t call 
argument conversion is at best dubious, and most likely a bug?


> test4.cc:5:7: note: did you mean to compare the result of 'memcmp' 
> instead?
>   if (memcmp(a, b, sizeof(a) != 0))
>       ^                          ~
>                             )
> test4.cc:5:20: note: explicitly cast the argument to size_t to silence 
> this warning
>   if (memcmp(a, b, sizeof(a) != 0))
>                    ^
>                    (size_t)(     )


A cast is the wrong FixIt to suggest. "sizeof(a) ? 1 : 0" is closer to 
the natural way to write this construct if it was indeed their true 
intention.

However it's best not to generate a FixIt in this context given that we 
have a high degree of confidence that the user's code is bogus, 
especially since the FixIt hides away the bug into a form that's harder 
to detect.

Alp.



> 1 warning generated.
>
>
> This would have found one bug in NSS that we recently fixed [1] and 
> found one more bug in chromium we didn't know about before [2]. It had 
> 0 false positives on all of chromium.
>
> The idea of triggering this warning on a binop in the size argument is 
> due to rnk.
>
> This warning can possibly be extended later on, but I feel this is a 
> good start.
>
> Thoughts?
>
> Nico
>
> [1]: 
> https://codereview.chromium.org/99423002/diff/1/net/third_party/nss/ssl/ssl3con.c
> [2]: https://codereview.chromium.org/8431007/#msg12
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

-- 
http://www.nuanti.com
the browser experts