[Re-send to correct addresses.] On Thu, Dec 26, 2013 at 3:38 PM, Nico Weber <nicolaswe...@gmx.de> wrote:
> Author: nico > Date: Thu Dec 26 17:38:39 2013 > New Revision: 198063 > > URL: http://llvm.org/viewvc/llvm-project?rev=198063&view=rev > Log: > 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. > > > Added: > cfe/trunk/test/SemaCXX/warn-memsize-comparison.cpp > Modified: > cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td > cfe/trunk/lib/Sema/SemaChecking.cpp > > Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ > Basic/DiagnosticSemaKinds.td?rev=198063&r1=198062&r2=198063&view=diff > ============================================================ > ================== > --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original) > +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Thu Dec 26 > 17:38:39 2013 > @@ -390,11 +390,18 @@ def warn_sizeof_pointer_type_memaccess : > "%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 " > > Modified: cfe/trunk/lib/Sema/SemaChecking.cpp > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaC > hecking.cpp?rev=198063&r1=198062&r2=198063&view=diff > ============================================================ > ================== > --- cfe/trunk/lib/Sema/SemaChecking.cpp (original) > +++ cfe/trunk/lib/Sema/SemaChecking.cpp Thu Dec 26 17:38:39 2013 > @@ -622,7 +622,7 @@ bool Sema::CheckARMBuiltinFunctionCall(u > 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 Strin > > //===--- 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 @@ void Sema::CheckMemaccessArguments(const > 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(cons > 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 C > 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, Ex > 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); > > Added: cfe/trunk/test/SemaCXX/warn-memsize-comparison.cpp > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/ > warn-memsize-comparison.cpp?rev=198063&view=auto > ============================================================ > ================== > --- cfe/trunk/test/SemaCXX/warn-memsize-comparison.cpp (added) > +++ cfe/trunk/test/SemaCXX/warn-memsize-comparison.cpp Thu Dec 26 > 17:38:39 2013 > @@ -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}} \ > This note makes no sense in this case. memcpy returns a pointer; comparing it 'less than 0' is not meaningful... > + expected-note {{explicitly cast the argument}} > + if (memcpy(b1, b2, sizeof(b1)) < 0) {} > ... and this 'corrected' form is ill-formed under C++ DR1512. Do you think we should suppress the note in this case, or the entire warning? + > + 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}} \ > Likewise. > + 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))) {} > +} > > > _______________________________________________ > cfe-commits mailing list > cfe-comm...@cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
