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.
Created attachment 11766 [details] wip
Created attachment 11767 [details] patch Done except for tests and evaluation on real code: test4.cc:4:39: warning: size argument in 'memcmp' call is a comparison if (memcmp(argv, argv, argc + 23452 > 2300)) ~~~~~~~~~~~~~^~~~~~ test4.cc:4:7: warning: did you mean to compare the result of 'memcmp' instead? if (memcmp(argv, argv, argc + 23452 > 2300)) ^ ~ ) test4.cc:4:26: warning: explicitly cast the argument to size_t to silence this warning if (memcmp(argv, argv, argc + 23452 > 2300)) ^ (size_t)( )
On Chromium, this found another bug in addition to the one we already knew about, and had 0 false positives. Likely worth it.
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
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?
Created attachment 11837 [details] generalized patch Here's a prototype of doing this for all calls where the last parameter is an unsigned. I haven't really evaluated this yet (and the patch needs lots of cleanups too). (Without the char check there lots of false positives in mesa, due to GLboolean being an unsigned char)
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
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);
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.
Created attachment 11838 [details] generalized patch, v2 This is a slightly better version of the previous generalized patch.
Hm, the xz false positive could be fixed by only triggering the warning on functions that don't return void.
Created attachment 11839 [details] generalized patch, v3 With the void check. The patch still has one false positive I think: bool MarkAsCompleted(uint64 result); bool AllSamplesPassedQuery::Process() { // ... return MarkAsCompleted(result != 0); } and relying on the void check feels brittle (if someone adds a status return code to that xz function, the warning starts firing again). So the warning is probably better as-is? It does find one thing that looks like a bug though.
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.)
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.