[patch] Warn on memcmp(a, b, sizeof(a) != 0) & co
Nico Weber
thakis at chromium.org
Fri Dec 20 23:31:10 PST 2013
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))
~~~~~~~~~~^~~~
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)( )
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131220/c8752d48/attachment.html>
-------------- next part --------------
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td (revision 197300)
+++ include/clang/Basic/DiagnosticSemaKinds.td (working copy)
@@ -388,11 +388,18 @@
"%select{destination|source}2; expected %3 or an explicit length">,
InGroup<SizeofPointerMemaccess>;
def warn_strlcpycat_wrong_size : Warning<
- "size argument in %0 call appears to be size of the source; expected the size of "
- "the destination">,
+ "size argument in %0 call appears to be size of the source; "
+ "expected the size of the destination">,
InGroup<DiagGroup<"strlcpy-strlcat-size">>;
def note_strlcpycat_wrong_size : Note<
"change size argument to be the size of the destination">;
+def warn_memsize_comparison : Warning<
+ "size argument in %0 call is a comparison">,
+ InGroup<DiagGroup<"memsize-comparison">>;
+def warn_memsize_comparison_paren_note : Note<
+ "did you mean to compare the result of %0 instead?">;
+def warn_memsize_comparison_cast_note : Note<
+ "explicitly cast the argument to size_t to silence this warning">;
def warn_strncat_large_size : Warning<
"the value of the size argument in 'strncat' is too large, might lead to a "
Index: lib/Sema/SemaChecking.cpp
===================================================================
--- lib/Sema/SemaChecking.cpp (revision 197300)
+++ lib/Sema/SemaChecking.cpp (working copy)
@@ -622,7 +622,7 @@
RHS.get(), AA_Assigning))
return true;
}
-
+
// For NEON intrinsics which take an immediate value as part of the
// instruction, range check them here.
unsigned i = 0, l = 0, u = 0;
@@ -3560,6 +3560,40 @@
//===--- CHECK: Standard memory functions ---------------------------------===//
+/// \brief Takes the expression passed to the size_t parameter of functions
+/// such as memcmp, strncat, etc and warns if it's a comparison.
+///
+/// This is to catch typos like `if (memcmp(&a, &b, sizeof(a) > 0))`.
+static bool CheckMemorySizeofForComparison(Sema &S, const Expr *E,
+ IdentifierInfo *FnName,
+ SourceLocation FnLoc,
+ SourceLocation RParenLoc) {
+ const BinaryOperator *Size = dyn_cast<BinaryOperator>(E);
+ if (!Size)
+ return false;
+
+ // if E is binop and op is >, <, >=, <=, ==:
+ if (!Size->isComparisonOp() && !Size->isEqualityOp() && !Size->isLogicalOp())
+ return false;
+
+ Preprocessor &PP = S.getPreprocessor();
+ SourceRange SizeRange = Size->getSourceRange();
+ S.Diag(Size->getOperatorLoc(), diag::warn_memsize_comparison)
+ << SizeRange << FnName;
+ S.Diag(FnLoc, diag::warn_memsize_comparison_paren_note)
+ << FnName
+ << FixItHint::CreateInsertion(
+ PP.getLocForEndOfToken(Size->getLHS()->getLocEnd()),
+ ")")
+ << FixItHint::CreateRemoval(RParenLoc);
+ S.Diag(SizeRange.getBegin(), diag::warn_memsize_comparison_cast_note)
+ << FixItHint::CreateInsertion(SizeRange.getBegin(), "(size_t)(")
+ << FixItHint::CreateInsertion(
+ PP.getLocForEndOfToken(SizeRange.getEnd()), ")");
+
+ return true;
+}
+
/// \brief Determine whether the given type is a dynamic class type (e.g.,
/// whether it has a vtable).
static bool isDynamicClassType(QualType T) {
@@ -3615,6 +3649,10 @@
unsigned LenArg = (BId == Builtin::BIstrndup ? 1 : 2);
const Expr *LenExpr = Call->getArg(LenArg)->IgnoreParenImpCasts();
+ if (CheckMemorySizeofForComparison(*this, LenExpr, FnName,
+ Call->getLocStart(), Call->getRParenLoc()))
+ return;
+
// We have special checking when the length is a sizeof expression.
QualType SizeOfArgTy = getSizeOfArgType(LenExpr);
const Expr *SizeOfArg = getSizeOfExprArg(LenExpr);
@@ -3798,6 +3836,10 @@
const Expr *SrcArg = ignoreLiteralAdditions(Call->getArg(1), Context);
const Expr *SizeArg = ignoreLiteralAdditions(Call->getArg(2), Context);
const Expr *CompareWithSrc = NULL;
+
+ if (CheckMemorySizeofForComparison(*this, SizeArg, FnName,
+ Call->getLocStart(), Call->getRParenLoc()))
+ return;
// Look for 'strlcpy(dst, x, sizeof(x))'
if (const Expr *Ex = getSizeOfExprArg(SizeArg))
@@ -3880,6 +3922,10 @@
const Expr *SrcArg = CE->getArg(1)->IgnoreParenCasts();
const Expr *LenArg = CE->getArg(2)->IgnoreParenCasts();
+ if (CheckMemorySizeofForComparison(*this, LenArg, FnName, CE->getLocStart(),
+ CE->getRParenLoc()))
+ return;
+
// Identify common expressions, which are wrongly used as the size argument
// to strncat and may lead to buffer overflows.
unsigned PatternType = 0;
@@ -5235,7 +5281,7 @@
if (Target->isSpecificBuiltinType(BuiltinType::Bool)) {
if (isa<StringLiteral>(E))
// Warn on string literal to bool. Checks for string literals in logical
- // expressions, for instances, assert(0 && "error here"), is prevented
+ // expressions, for instances, assert(0 && "error here"), are prevented
// by a check in AnalyzeImplicitConversions().
return DiagnoseImpCast(S, E, T, CC,
diag::warn_impcast_string_literal_to_bool);
Index: test/SemaCXX/warn-memsize-comparison.cpp
===================================================================
--- test/SemaCXX/warn-memsize-comparison.cpp (revision 0)
+++ test/SemaCXX/warn-memsize-comparison.cpp (working copy)
@@ -0,0 +1,91 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+//
+
+typedef __SIZE_TYPE__ size_t;
+extern "C" void *memset(void *, int, size_t);
+extern "C" void *memmove(void *s1, const void *s2, size_t n);
+extern "C" void *memcpy(void *s1, const void *s2, size_t n);
+extern "C" void *memcmp(void *s1, const void *s2, size_t n);
+extern "C" int strncmp(const char *s1, const char *s2, size_t n);
+extern "C" int strncasecmp(const char *s1, const char *s2, size_t n);
+extern "C" char *strncpy(char *dst, const char *src, size_t n);
+extern "C" char *strncat(char *dst, const char *src, size_t n);
+extern "C" char *strndup(const char *src, size_t n);
+extern "C" size_t strlcpy(char *dst, const char *src, size_t size);
+extern "C" size_t strlcat(char *dst, const char *src, size_t size);
+
+void f() {
+ char b1[80], b2[80];
+ if (memset(b1, 0, sizeof(b1) != 0)) {} // \
+ expected-warning{{size argument in 'memset' call is a comparison}} \
+ expected-note {{did you mean to compare}} \
+ expected-note {{explicitly cast the argument}}
+ if (memset(b1, 0, sizeof(b1)) != 0) {}
+
+ if (memmove(b1, b2, sizeof(b1) == 0)) {} // \
+ expected-warning{{size argument in 'memmove' call is a comparison}} \
+ expected-note {{did you mean to compare}} \
+ expected-note {{explicitly cast the argument}}
+ if (memmove(b1, b2, sizeof(b1)) == 0) {}
+
+ if (memcpy(b1, b2, sizeof(b1) < 0)) {} // \
+ expected-warning{{size argument in 'memcpy' call is a comparison}} \
+ expected-note {{did you mean to compare}} \
+ expected-note {{explicitly cast the argument}}
+ if (memcpy(b1, b2, sizeof(b1)) < 0) {}
+
+ if (memcmp(b1, b2, sizeof(b1) <= 0)) {} // \
+ expected-warning{{size argument in 'memcmp' call is a comparison}} \
+ expected-note {{did you mean to compare}} \
+ expected-note {{explicitly cast the argument}}
+ if (memcmp(b1, b2, sizeof(b1)) <= 0) {}
+
+ if (strncmp(b1, b2, sizeof(b1) > 0)) {} // \
+ expected-warning{{size argument in 'strncmp' call is a comparison}} \
+ expected-note {{did you mean to compare}} \
+ expected-note {{explicitly cast the argument}}
+ if (strncmp(b1, b2, sizeof(b1)) > 0) {}
+
+ if (strncasecmp(b1, b2, sizeof(b1) >= 0)) {} // \
+ expected-warning{{size argument in 'strncasecmp' call is a comparison}} \
+ expected-note {{did you mean to compare}} \
+ expected-note {{explicitly cast the argument}}
+ if (strncasecmp(b1, b2, sizeof(b1)) >= 0) {}
+
+ if (strncpy(b1, b2, sizeof(b1) == 0 || true)) {} // \
+ expected-warning{{size argument in 'strncpy' call is a comparison}} \
+ expected-note {{did you mean to compare}} \
+ expected-note {{explicitly cast the argument}}
+ if (strncpy(b1, b2, sizeof(b1)) == 0 || true) {}
+
+ if (strncat(b1, b2, sizeof(b1) - 1 >= 0 && true)) {} // \
+ expected-warning{{size argument in 'strncat' call is a comparison}} \
+ expected-note {{did you mean to compare}} \
+ expected-note {{explicitly cast the argument}}
+ if (strncat(b1, b2, sizeof(b1) - 1) >= 0 && true) {}
+
+ if (strndup(b1, sizeof(b1) != 0)) {} // \
+ expected-warning{{size argument in 'strndup' call is a comparison}} \
+ expected-note {{did you mean to compare}} \
+ expected-note {{explicitly cast the argument}}
+ if (strndup(b1, sizeof(b1)) != 0) {}
+
+ if (strlcpy(b1, b2, sizeof(b1) != 0)) {} // \
+ expected-warning{{size argument in 'strlcpy' call is a comparison}} \
+ expected-note {{did you mean to compare}} \
+ expected-note {{explicitly cast the argument}}
+ if (strlcpy(b1, b2, sizeof(b1)) != 0) {}
+
+ if (strlcat(b1, b2, sizeof(b1) != 0)) {} // \
+ expected-warning{{size argument in 'strlcat' call is a comparison}} \
+ expected-note {{did you mean to compare}} \
+ expected-note {{explicitly cast the argument}}
+ if (strlcat(b1, b2, sizeof(b1)) != 0) {}
+
+ if (memset(b1, 0, sizeof(b1) / 2)) {}
+ if (memset(b1, 0, sizeof(b1) >> 2)) {}
+ if (memset(b1, 0, 4 << 2)) {}
+ if (memset(b1, 0, 4 + 2)) {}
+ if (memset(b1, 0, 4 - 2)) {}
+ if (memset(b1, 0, 4 * 2)) {}
+}
More information about the cfe-commits
mailing list