Index: include/clang/Basic/DiagnosticGroups.td =================================================================== --- include/clang/Basic/DiagnosticGroups.td +++ include/clang/Basic/DiagnosticGroups.td @@ -443,6 +443,13 @@ def SizeofArrayArgument : DiagGroup<"sizeof-array-argument">; def SizeofArrayDecay : DiagGroup<"sizeof-array-decay">; def SizeofPointerMemaccess : DiagGroup<"sizeof-pointer-memaccess">; +def MemsetTransposedArgs : DiagGroup<"memset-transposed-args">; +def DynamicClassMemaccess : DiagGroup<"dynamic-class-memaccess">; +def NonTrivialMemaccess : DiagGroup<"nontrivial-memaccess">; +def SuspiciousBzero : DiagGroup<"suspicious-bzero">; +def SuspiciousMemaccess : DiagGroup<"suspicious-memaccess", + [SizeofPointerMemaccess, DynamicClassMemaccess, + NonTrivialMemaccess, MemsetTransposedArgs, SuspiciousBzero]>; def StaticInInline : DiagGroup<"static-in-inline">; def StaticLocalInInline : DiagGroup<"static-local-in-inline">; def GNUStaticFloatInit : DiagGroup<"gnu-static-float-init">; Index: include/clang/Basic/DiagnosticSemaKinds.td =================================================================== --- include/clang/Basic/DiagnosticSemaKinds.td +++ include/clang/Basic/DiagnosticSemaKinds.td @@ -619,14 +619,14 @@ "%select{destination for|source of|first operand of|second operand of}0 this " "%1 call is a pointer to record %2 that is not trivial to " "%select{primitive-default-initialize|primitive-copy}3">, - InGroup>; + InGroup; def note_nontrivial_field : Note< "field is non-trivial to %select{copy|default-initialize}0">; def warn_dyn_class_memaccess : Warning< "%select{destination for|source of|first operand of|second operand of}0 this " "%1 call is a pointer to %select{|class containing a }2dynamic class %3; " "vtable pointer will be %select{overwritten|copied|moved|compared}4">, - InGroup>; + InGroup; def note_bad_memaccess_silence : Note< "explicitly cast the pointer to silence this warning">; def warn_sizeof_pointer_expr_memaccess : Warning< @@ -655,7 +655,19 @@ "did you mean to compare the result of %0 instead?">; def note_memsize_comparison_cast_silence : Note< "explicitly cast the argument to size_t to silence this warning">; - +def warn_suspicious_sizeof_memset : Warning< + "%select{'size' argument to memset is '0'|" + "setting buffer to a 'sizeof' expression}0" + "; did you mean to transpose the last two arguments?">, + InGroup; +def note_suspicious_sizeof_memset_silence : Note< + "%select{parenthesize the third argument|" + "cast the second argument to 'int'}0 to silence">; +def warn_suspicious_bzero_size : Warning<"'size' argument to bzero is '0'">, + InGroup; +def note_suspicious_bzero_size_silence : Note< + "parenthesize the second argument to silence">; + def warn_strncat_large_size : Warning< "the value of the size argument in 'strncat' is too large, might lead to a " "buffer overflow">, InGroup; Index: lib/Sema/SemaChecking.cpp =================================================================== --- lib/Sema/SemaChecking.cpp +++ lib/Sema/SemaChecking.cpp @@ -8629,24 +8629,26 @@ return nullptr; } +static const UnaryExprOrTypeTraitExpr *getAsSizeOfExpr(const Expr *E) { + if (const auto *Unary = dyn_cast(E)) + if (Unary->getKind() == UETT_SizeOf) + return Unary; + return nullptr; +} + /// If E is a sizeof expression, returns its argument expression, /// otherwise returns NULL. static const Expr *getSizeOfExprArg(const Expr *E) { - if (const UnaryExprOrTypeTraitExpr *SizeOf = - dyn_cast(E)) - if (SizeOf->getKind() == UETT_SizeOf && !SizeOf->isArgumentType()) + if (const UnaryExprOrTypeTraitExpr *SizeOf = getAsSizeOfExpr(E)) + if (!SizeOf->isArgumentType()) return SizeOf->getArgumentExpr()->IgnoreParenImpCasts(); - return nullptr; } /// If E is a sizeof expression, returns its argument type. static QualType getSizeOfArgType(const Expr *E) { - if (const UnaryExprOrTypeTraitExpr *SizeOf = - dyn_cast(E)) - if (SizeOf->getKind() == UETT_SizeOf) - return SizeOf->getTypeOfArgument(); - + if (const UnaryExprOrTypeTraitExpr *SizeOf = getAsSizeOfExpr(E)) + return SizeOf->getTypeOfArgument(); return QualType(); } @@ -8742,6 +8744,86 @@ } +/// Detect if \c SizeofExpr is likely to calculate the sizeof an object. +static bool doesExprLikelyComputeSize(const Expr *SizeofExpr) { + SizeofExpr = SizeofExpr->IgnoreParenImpCasts(); + + if (const auto *BO = dyn_cast(SizeofExpr)) { + if (BO->getOpcode() != BO_Mul && BO->getOpcode() != BO_Add) + return false; + + return doesExprLikelyComputeSize(BO->getLHS()) || + doesExprLikelyComputeSize(BO->getRHS()); + } + + return getAsSizeOfExpr(SizeofExpr) != nullptr; +} + +/// Check if the ArgLoc originated from a macro passed to the call at CallLoc. +/// +/// \code +/// #define MACRO 0 +/// foo(MACRO); +/// foo(0); +/// \endcode +/// +/// This should return true for the first call to foo, but not for the second +/// (regardless of whether foo is a macro or function). +static bool isArgumentExpandedFromMacro(SourceManager &SM, + SourceLocation CallLoc, + SourceLocation ArgLoc) { + if (!CallLoc.isMacroID()) + return SM.getFileID(CallLoc) != SM.getFileID(ArgLoc); + + return SM.getFileID(SM.getImmediateMacroCallerLoc(CallLoc)) != + SM.getFileID(SM.getImmediateMacroCallerLoc(ArgLoc)); +} + +/// Diagnose cases like 'memset(buf, sizeof(buf), 0)', which should have the +/// last two arguments transposed. +static void CheckMemaccessSize(Sema &S, unsigned BId, const CallExpr *Call) { + if (BId != Builtin::BImemset && BId != Builtin::BIbzero) + return; + + const Expr *SizeArg = + Call->getArg(BId == Builtin::BImemset ? 2 : 1)->IgnoreImpCasts(); + + // If we're memsetting or bzeroing 0 bytes, then this is likely an error. + SourceLocation CallLoc = Call->getRParenLoc(); + SourceManager &SM = S.getSourceManager(); + if (isa(SizeArg) && + cast(SizeArg)->getValue() == 0 && + !isArgumentExpandedFromMacro(SM, CallLoc, SizeArg->getExprLoc())) { + + SourceLocation DiagLoc = SizeArg->getExprLoc(); + + // Some platforms #define bzero to __builtin_memset. See if this is the + // case, and if so, emit a better diagnostic. + if (BId == Builtin::BIbzero || + (CallLoc.isMacroID() && Lexer::getImmediateMacroName( + CallLoc, SM, S.getLangOpts()) == "bzero")) { + S.Diag(DiagLoc, diag::warn_suspicious_bzero_size); + S.Diag(DiagLoc, diag::note_suspicious_bzero_size_silence); + } else { + S.Diag(DiagLoc, diag::warn_suspicious_sizeof_memset) << 0; + S.Diag(DiagLoc, diag::note_suspicious_sizeof_memset_silence) << 0; + } + return; + } + + // If the second argument to a memset is a sizeof expression and the third + // isn't, this is also likely an error. This should catch + // 'memset(buf, sizeof(buf), 0xff)'. + if (BId == Builtin::BImemset && + doesExprLikelyComputeSize(Call->getArg(1)) && + !doesExprLikelyComputeSize(Call->getArg(2))) { + SourceLocation DiagLoc = Call->getArg(1)->getExprLoc(); + S.Diag(DiagLoc, diag::warn_suspicious_sizeof_memset) << 1; + S.Diag(DiagLoc, diag::note_suspicious_sizeof_memset_silence) << 1; + return; + } +} + /// Check for dangerous or invalid arguments to memset(). /// /// This issues warnings on known problematic, dangerous or unspecified @@ -8771,6 +8853,9 @@ Call->getLocStart(), Call->getRParenLoc())) return; + // Catch cases like 'memset(buf, sizeof(buf), 0)'. + CheckMemaccessSize(*this, BId, Call); + // We have special checking when the length is a sizeof expression. QualType SizeOfArgTy = getSizeOfArgType(LenExpr); const Expr *SizeOfArg = getSizeOfExprArg(LenExpr); Index: test/Sema/transpose-memset.c =================================================================== --- test/Sema/transpose-memset.c +++ test/Sema/transpose-memset.c @@ -0,0 +1,60 @@ +// RUN: %clang_cc1 -Wmemset-transposed-args -verify %s +// RUN: %clang_cc1 -xc++ -Wmemset-transposed-args -verify %s + +#define memset(...) __builtin_memset(__VA_ARGS__) +#define bzero(x,y) __builtin_memset(x, 0, y) +#define real_bzero(x,y) __builtin_bzero(x,y) + +int array[10]; +int *ptr; + +int main() { + memset(array, sizeof(array), 0); // expected-warning{{'size' argument to memset is '0'; did you mean to transpose the last two arguments?}} expected-note{{parenthesize the third argument to silence}} + memset(array, sizeof(array), 0xff); // expected-warning{{setting buffer to a 'sizeof' expression; did you mean to transpose the last two arguments?}} expected-note{{cast the second argument to 'int' to silence}} + memset(ptr, sizeof(ptr), 0); // expected-warning{{'size' argument to memset is '0'; did you mean to transpose the last two arguments?}} expected-note{{parenthesize the third argument to silence}} + memset(ptr, sizeof(*ptr) * 10, 1); // expected-warning{{setting buffer to a 'sizeof' expression; did you mean to transpose the last two arguments?}} expected-note{{cast the second argument to 'int' to silence}} + memset(ptr, 10 * sizeof(int *), 1); // expected-warning{{setting buffer to a 'sizeof' expression; did you mean to transpose the last two arguments?}} expected-note{{cast the second argument to 'int' to silence}} + memset(ptr, 10 * sizeof(int *) + 10, 0xff); // expected-warning{{setting buffer to a 'sizeof' expression; did you mean to transpose the last two arguments?}} expected-note{{cast the second argument to 'int' to silence}} + memset(ptr, sizeof(char) * sizeof(int *), 0xff); // expected-warning{{setting buffer to a 'sizeof' expression; did you mean to transpose the last two arguments?}} expected-note{{cast the second argument to 'int' to silence}} + memset(array, sizeof(array), sizeof(array)); // Uh... fine I guess. + memset(array, 0, sizeof(array)); + memset(ptr, 0, sizeof(int *) * 10); + memset(array, (int)sizeof(array), (0)); // no warning + memset(array, (int)sizeof(array), 32); // no warning + memset(array, 32, (0)); // no warning + + bzero(ptr, 0); // expected-warning{{'size' argument to bzero is '0'}} expected-note{{parenthesize the second argument to silence}} + real_bzero(ptr, 0); // expected-warning{{'size' argument to bzero is '0'}} expected-note{{parenthesize the second argument to silence}} +} + +void macros() { +#define ZERO 0 + int array[10]; + memset(array, 0xff, ZERO); // no warning + // Still emit a diagnostic for memsetting a sizeof expression: + memset(array, sizeof(array), ZERO); // expected-warning{{'sizeof'}} expected-note{{cast}} + bzero(array, ZERO); // no warning + real_bzero(array, ZERO); // no warning +#define NESTED_DONT_DIAG \ + memset(array, 0xff, ZERO); \ + real_bzero(array, ZERO); + + NESTED_DONT_DIAG; + +#define NESTED_DO_DIAG \ + memset(array, 0xff, 0); \ + real_bzero(array, 0) + + NESTED_DO_DIAG; // expected-warning{{'size' argument to memset}} expected-warning{{'size' argument to bzero}} expected-note2{{parenthesize}} + +#define FN_MACRO(p) \ + memset(array, 0xff, p) + + FN_MACRO(ZERO); + FN_MACRO(0); // FIXME: should we diagnose this? + + __builtin_memset(array, 0, ZERO); // no warning + __builtin_bzero(array, ZERO); + __builtin_memset(array, 0, 0); // expected-warning{{'size' argument to memset}} // expected-note{{parenthesize}} + __builtin_bzero(array, 0); // expected-warning{{'size' argument to bzero}} // expected-note{{parenthesize}} +}