On Tue, Oct 18, 2016 at 9:31 AM, Nico Weber <tha...@chromium.org> wrote:
> Did you find real-world code triggering this, or did you happen to see the > implementation of the warning? I'm not sure if omitting just the note or > the whole warning is more useful in practice. Intuitively I'd say that > people "never" will compare the result of memcpy, so I'd guess that it > doesn't matter much what we pick and omitting the warning completely is > fine, but I'm not sure. > I found this when implementing core issue 1512, under which the suggested "fixed" code doesn't compile any more. > On Oct 17, 2016 1:44 PM, "Richard Smith" <rich...@metafoo.co.uk> wrote: > >> [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_lite >>> ral_to_bool); >>> >>> Added: cfe/trunk/test/SemaCXX/warn-memsize-comparison.cpp >>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/w >>> arn-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
