This has a false positive on ffmpeg: ../../third_party/ffmpeg/libavcodec/options.c:273:64: error: 'size' argument to memset is '0'; did you mean to transpose the last two arguments? [-Werror,-Wmemset-transposed-args] alloc_and_copy_or_fail(intra_matrix, 64 * sizeof(int16_t), 0);
With: #define alloc_and_copy_or_fail(obj, size, pad) \ if (src->obj && size > 0) { \ dest->obj = av_malloc(size + pad); \ if (!dest->obj) \ goto fail; \ memcpy(dest->obj, src->obj, size); \ if (pad) \ memset(((uint8_t *) dest->obj) + size, 0, pad); \ } ( https://cs.chromium.org/chromium/src/third_party/ffmpeg/libavcodec/options.c?q=alloc_and_copy_or_fail&sq=package:chromium&g=0&l=261 ; https://bugs.chromium.org/p/chromium/issues/detail?id=866202) Maybe the warning shouldn't fire if the memset is from a macro? On Thu, Jul 19, 2018 at 12:51 PM Erik Pilkington via cfe-commits < cfe-commits@lists.llvm.org> wrote: > Author: epilk > Date: Thu Jul 19 09:46:15 2018 > New Revision: 337470 > > URL: http://llvm.org/viewvc/llvm-project?rev=337470&view=rev > Log: > [Sema] Add a new warning, -Wmemset-transposed-args > > This diagnoses calls to memset that have the second and third arguments > transposed, for example: > > memset(buf, sizeof(buf), 0); > > This is done by checking if the third argument is a literal 0, or if the > second > is a sizeof expression (and the third isn't). The first check is also done > for > calls to bzero. > > Differential revision: https://reviews.llvm.org/D49112 > > Added: > cfe/trunk/test/Sema/transpose-memset.c > Modified: > cfe/trunk/include/clang/Basic/DiagnosticGroups.td > cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td > cfe/trunk/lib/Sema/SemaChecking.cpp > > Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=337470&r1=337469&r2=337470&view=diff > > ============================================================================== > --- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original) > +++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Thu Jul 19 09:46:15 > 2018 > @@ -443,6 +443,13 @@ def : DiagGroup<"synth">; > 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">; > > Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=337470&r1=337469&r2=337470&view=diff > > ============================================================================== > --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original) > +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Thu Jul 19 > 09:46:15 2018 > @@ -619,14 +619,14 @@ def warn_cstruct_memaccess : Warning< > "%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<DiagGroup<"nontrivial-memaccess">>; > + InGroup<NonTrivialMemaccess>; > 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<DiagGroup<"dynamic-class-memaccess">>; > + InGroup<DynamicClassMemaccess>; > 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 @@ def note_memsize_comparison_paren : Note > "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<MemsetTransposedArgs>; > +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<SuspiciousBzero>; > +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<StrncatSize>; > > Modified: cfe/trunk/lib/Sema/SemaChecking.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=337470&r1=337469&r2=337470&view=diff > > ============================================================================== > --- cfe/trunk/lib/Sema/SemaChecking.cpp (original) > +++ cfe/trunk/lib/Sema/SemaChecking.cpp Thu Jul 19 09:46:15 2018 > @@ -8629,24 +8629,26 @@ static const CXXRecordDecl *getContained > return nullptr; > } > > +static const UnaryExprOrTypeTraitExpr *getAsSizeOfExpr(const Expr *E) { > + if (const auto *Unary = dyn_cast<UnaryExprOrTypeTraitExpr>(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<UnaryExprOrTypeTraitExpr>(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<UnaryExprOrTypeTraitExpr>(E)) > - if (SizeOf->getKind() == UETT_SizeOf) > - return SizeOf->getTypeOfArgument(); > - > + if (const UnaryExprOrTypeTraitExpr *SizeOf = getAsSizeOfExpr(E)) > + return SizeOf->getTypeOfArgument(); > return QualType(); > } > > @@ -8742,6 +8744,86 @@ struct SearchNonTrivialToCopyField > > } > > +/// 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<BinaryOperator>(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<IntegerLiteral>(SizeArg) && > + cast<IntegerLiteral>(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 @@ void Sema::CheckMemaccessArguments(const > 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); > > Added: cfe/trunk/test/Sema/transpose-memset.c > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/transpose-memset.c?rev=337470&view=auto > > ============================================================================== > --- cfe/trunk/test/Sema/transpose-memset.c (added) > +++ cfe/trunk/test/Sema/transpose-memset.c Thu Jul 19 09:46:15 2018 > @@ -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}} > +} > > > _______________________________________________ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits