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

[Nico Weber]

On Sat, Dec 21, 2013 at 8:55 PM, Nico Weber <thakis at chromium.org> wrote:

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

I did experiment with this extension a bit. It finds another 1.5 bugs in
chromium (a real bug in NSS, and somewhat questionable code in chromium
itself), with 0.5 false positives (it's actually 0, but in 1 case it's
fairly easy to accidentally enable the warning) – see
http://llvm.org/bugs/show_bug.cgi?id=18297 , comments 6 and later.

Does someone feel like running this expanded warning on another large
codebase, to get a better idea how useful it is? A prototype patch that's
good enough for evaluating the warning is attached to the bug:
http://llvm.org/bugs/attachment.cgi?id=11839


>
> 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/20140108/ecf0ab13/attachment.html>