Warn if comparison result is passed as last argument to memcmp and friends (or when it's passed as size_t more generally?)

[Quuxplusone]

Bugzilla Link	PR18297
Status	NEW
Importance	P normal
Reported by	Nico Weber (nicolasweber@gmx.de)
Reported on	2013-12-20 18:25:56 -0800
Last modified on	2014-01-08 13:39:50 -0800
Version	unspecified
Hardware	PC All
CC	alp@nuanti.com, llvm-bugs@lists.llvm.org
Fixed by commit(s)
Attachments	clang-wip.patch (2243 bytes, application/octet-stream) clang-wip.patch (5297 bytes, text/plain) clang-memsize-reloaded.diff (1399 bytes, text/plain) clang-memsize-reloaded.diff (1853 bytes, application/octet-stream) clang-memsize-reloaded.diff (2369 bytes, text/plain)
Blocks
Blocked by
See also

The motivation is to find bugs like
https://codereview.chromium.org/99423002/diff/1/net/third_party/nss/ssl/ssl3con.c
at compile time:

-   memcmp(spki->data, P256_SPKI_PREFIX, sizeof(P256_SPKI_PREFIX) != 0)) {
+   memcmp(spki->data, P256_SPKI_PREFIX, sizeof(P256_SPKI_PREFIX)) != 0) {

rnk suggested: " warn when passing comparison results to functions that expect
sizes (memset et al)?". Give that a try.

[Quuxplusone]

Attached clang-wip.patch (2243 bytes, application/octet-stream): wip

[Quuxplusone]

Attached clang-wip.patch (5297 bytes, text/plain): patch

[Quuxplusone]

On Chromium, this found another bug in addition to the one we already knew about, and had 0 false positives. Likely worth it.

[Quuxplusone]

Evaluated this on chromium (found another bug, and 0 false positives), wrote tests, and sent the patch out: http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20131216/096073.html

[Quuxplusone]

Initial version checked in at r198063.

Might want to try to extend this to all function calls that have a size_t (or unsigned?) in the last parameter?

[Quuxplusone]

yasm defines TRUE as (0==0) and FALSE as (0!=0) leading to all kinds of funny
errors:

../../third_party/yasm/source/patched-yasm/libyasm/bitvect.c:1688:38: error:
size argument in 'BitVector_Create' call is a comparison [-Werror,-Wmemsize-
comparison]
        rest = BitVector_Create(bits,FALSE);
                                     ^~~~~
../../third_party/yasm/source/patched-yasm/libyasm/bitvect.h:73:23: note:
expanded from macro 'FALSE'
#define FALSE       (0!=0)
                     ~^~~
../../third_party/yasm/source/patched-yasm/libyasm/bitvect.c:1688:43: note: did
you mean to compare the result of 'BitVector_Create' instead?
        rest = BitVector_Create(bits,FALSE);
               ~~~~~~~~~~~~~~~~           ^

We should probably not emit this warning if the comparison is from a macro
expansion (probably for the currently-checked in version too) :-D

[Quuxplusone]

False positive in xz on:

static inline void
rc_bit(lzma_range_encoder *rc, probability *prob, uint32_t bit) { ... }

        rc_bit(&coder->rc,
                &coder->is_rep0_long[coder->state][pos_state],
                len != 1);

[Quuxplusone]

False positive in yasm (many):

        typedef enum boolean { false = FALSE, true = TRUE } boolean;

YASM_LIB_DECL
void    BitVector_LSB                (/*@out@*/ wordptr addr, boolean bit);

            BitVector_LSB(result, !BitVector_is_empty(op1) ||
                          !BitVector_is_empty(op2));

…I suppose this shouldn't fire on enums, or at least not on enums that have
exactly two values? Going with the former for now.

[Quuxplusone]

Attached clang-memsize-reloaded.diff (1399 bytes, text/plain): generalized patch

[Quuxplusone]

Hm, the xz false positive could be fixed by only triggering the warning on functions that don't return void.

[Quuxplusone]

Attached clang-memsize-reloaded.diff (1853 bytes, application/octet-stream): generalized patch, v2

[Quuxplusone]

Attached clang-memsize-reloaded.diff (2369 bytes, text/plain): generalized patch, v3

[Quuxplusone]

The one thing it found was this bit in nss (which I've reported to nss authors):

In nss/lib/softoken/pkcs11.c around line 1110:

    case CKK_NSS_JPAKE_ROUND1:
        if (!sftk_hasAttribute(object, CKA_PRIME ||
            !sftk_hasAttribute(object, CKA_SUBPRIME) ||
            !sftk_hasAttribute(object, CKA_BASE))) {
            return CKR_TEMPLATE_INCOMPLETE;
        }

(Note the missing ')' in the first sftk_hasAttribute call – but it's a harmless
bug, I'm told.)

[Quuxplusone]

nss bug: https://bugzilla.mozilla.org/show_bug.cgi?id=957727

The MarkAsCompleted() thing is apparently more a bug than a false positive too. Maybe worth running this over more code bases to get a better feel for how it does.