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

[Nico Weber]

Thanks for taking a look!

On Sat, Dec 21, 2013 at 2:26 PM, Alp Toker <alp at nuanti.com> wrote:

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


I think it's not obvious which direction the generalization should go: You
suggest implicit bool-to-size_t – there's a much higher chance for false
positives there. If experiments show that this is in fact viable, should
this be done for bool-to-unsigned? What about bool-to-int – we already know
that this has a noise level that's too high to be bearable. Would you do it
everywhere, or only in calls?

I think a more promising generalization is to do this for the last
parameter of calls whose last parameter type is unsigned.

But since it's not clear, I figured this is a generally agreeable and
useful subset, so I wanted to get this in first; this is what I meant with
"this can possibly be extended later on, but I feel this is a good start".


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

This is mostly a style thing, no? Is there any technical reason to prefer
your suggestion? I went with a cast since several other warnings have a
"explicitly cast to suppress" note, and I liked the consistency in
diagnostics. (?1:0 will suppress the warning too, so people can use that if
they prefer this.)


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

Note that it's a fixit on a note, not on the diagnostic itself, so it's
never applied automatically. We have several diagnostics with fixits on the
"to suppress" note. (See also the bit on fixits on
http://clang.llvm.org/docs/InternalsManual.html#fix-it-hints).

Nico


>
> 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131221/4f193c3f/attachment.html>