From 0e6daefe8f11c7a0e82c5b1455a0ff1d89a986bd Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Thu, 26 Dec 2013 23:38:39 +0000 Subject: [PATCH] Warn on mismatched parentheses in memcmp and friends. Thisadds a new warning that warns on code like this: if (memcmp(a, b, sizeof(a) != 0)) The warning looks like: 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 found 2 bugs in chromium and has 0 false positives on both chromium and llvm. The idea of triggering this warning on a binop in the size argument is due to rnk. llvm-svn: 198063 --- .../clang/Basic/DiagnosticSemaKinds.td | 11 ++- clang/lib/Sema/SemaChecking.cpp | 50 +++++++++- .../test/SemaCXX/warn-memsize-comparison.cpp | 93 +++++++++++++++++++ 3 files changed, 150 insertions(+), 4 deletions(-) create mode 100644 clang/test/SemaCXX/warn-memsize-comparison.cpp diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index c3ac6fbd78e3..461d46736eaa 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -390,11 +390,18 @@ def warn_sizeof_pointer_type_memaccess : Warning< "%select{destination|source}2; expected %3 or an explicit length">, InGroup; 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>; 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>; +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 " diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp index 97c041c9013b..feee61ea5ecc 100644 --- a/clang/lib/Sema/SemaChecking.cpp +++ b/clang/lib/Sema/SemaChecking.cpp @@ -622,7 +622,7 @@ bool Sema::CheckARMBuiltinFunctionCall(unsigned BuiltinID, CallExpr *TheCall) { 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 @@ void Sema::CheckFormatString(const StringLiteral *FExpr, //===--- 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(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 @@ void Sema::CheckMemaccessArguments(const CallExpr *Call, 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 @@ void Sema::CheckStrlcpycatArguments(const CallExpr *Call, 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 @@ void Sema::CheckStrncatArguments(const CallExpr *CE, 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 @@ void CheckImplicitConversion(Sema &S, Expr *E, QualType T, if (Target->isSpecificBuiltinType(BuiltinType::Bool)) { if (isa(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); diff --git a/clang/test/SemaCXX/warn-memsize-comparison.cpp b/clang/test/SemaCXX/warn-memsize-comparison.cpp new file mode 100644 index 000000000000..54c410e3dc0b --- /dev/null +++ b/clang/test/SemaCXX/warn-memsize-comparison.cpp @@ -0,0 +1,93 @@ +// 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)) {} + + if (memset(b1, 0, (size_t)(sizeof(b1) != 0))) {} +}