erik.pilkington created this revision.
erik.pilkington added reviewers: rsmith, aaron.ballman, arphaman.
Herald added a subscriber: dexonsmith.
This warning tries to catch programs that incorrectly call memset with the
second and third arguments transposed, ie `memset(ary, sizeof(ary), 0)` instead
of `memset(ary, 0, sizeof(ary))`. This is done by looking at two factors: 1) if
the last argument is `0`, then this is likely a bug (looks this is what GCC's
implementation does) and 2) if the last argument isn't `0`, but the second
argument is a `sizeof`expression. This catches cases like `memset(ary,
sizeof(ary), 0xff)`. I also grouped a couple of related diagnostics that deal
with these c functions into a new group, -Wsuspicious-memaccess.
llvm.org/PR36242
Thanks for taking a look!
Erik
Repository:
rC Clang
https://reviews.llvm.org/D49112
Files:
clang/include/clang/Basic/DiagnosticGroups.td
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Sema/SemaChecking.cpp
clang/test/Sema/transpose-memset.c
Index: clang/test/Sema/transpose-memset.c
===================================================================
--- /dev/null
+++ clang/test/Sema/transpose-memset.c
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -Wmemset-transposed-args -verify %s
+// RUN: %clang_cc1 -xc++ -Wmemset-transposed-args -verify %s
+
+#define memset(...) __builtin_memset(__VA_ARGS__)
+
+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(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
+}
Index: clang/lib/Sema/SemaChecking.cpp
===================================================================
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -7839,24 +7839,26 @@
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();
}
@@ -7952,6 +7954,57 @@
}
+/// Detect if \c E is likely to calculate the sizeof an object.
+static bool doesExprLikelyComputeSize(const Expr *SizeofExpr) {
+ SizeofExpr = SizeofExpr->IgnoreParenImpCasts();
+
+ if (auto *BO = dyn_cast<BinaryOperator>(SizeofExpr)) {
+ if (BO->getOpcode() != BO_Mul)
+ return false;
+
+ const Expr *LHSSizeof = getAsSizeOfExpr(BO->getLHS()),
+ *RHSSizeof = getAsSizeOfExpr(BO->getRHS());
+ return static_cast<bool>(LHSSizeof) != static_cast<bool>(RHSSizeof);
+ }
+
+ return getAsSizeOfExpr(SizeofExpr) != nullptr;
+}
+
+/// Diagnose cases like 'memset(buf, sizeof(buf), 0)', which should have the
+/// last two arguments transposed.
+static void CheckMemsetSizeof(Sema &S, unsigned BId, const CallExpr *Call) {
+ if (BId != Builtin::BImemset)
+ return;
+
+ int WarningKind;
+ SourceLocation DiagLoc;
+
+ // If we're memsetting 0 bytes, then this is likely a programmer error.
+ const Expr *ThirdArg = Call->getArg(2)->IgnoreImpCasts();
+ if (isa<IntegerLiteral>(ThirdArg) &&
+ cast<IntegerLiteral>(ThirdArg)->getValue() == 0) {
+ WarningKind = 0;
+ DiagLoc = ThirdArg->getExprLoc();
+ }
+ // If the second argument is a sizeof expression and the third isn't, this is
+ // also likely an error. This should catch 'memset(buf, sizeof(buf), 0xff)'.
+ else if (doesExprLikelyComputeSize(Call->getArg(1)) &&
+ !doesExprLikelyComputeSize(Call->getArg(2))) {
+ WarningKind = 1;
+ DiagLoc = Call->getArg(1)->getExprLoc();
+ } else
+ return;
+
+
+ // If the function is defined as a builtin macro, do not show macro expansion.
+ if (S.getSourceManager().isMacroArgExpansion(DiagLoc))
+ DiagLoc = S.getSourceManager().getSpellingLoc(DiagLoc);
+
+ // FIXME: A fixit would be nice here.
+ S.Diag(DiagLoc, diag::warn_suspicious_sizeof_memset) << WarningKind;
+ S.Diag(DiagLoc, diag::note_suspicious_sizeof_memset_silence) << WarningKind;
+}
+
/// Check for dangerous or invalid arguments to memset().
///
/// This issues warnings on known problematic, dangerous or unspecified
@@ -7981,6 +8034,9 @@
Call->getLocStart(), Call->getRParenLoc()))
return;
+ // Catch cases like 'memset(buf, sizeof(buf), 0)'.
+ CheckMemsetSizeof(*this, BId, Call);
+
// We have special checking when the length is a sizeof expression.
QualType SizeOfArgTy = getSizeOfArgType(LenExpr);
const Expr *SizeOfArg = getSizeOfExprArg(LenExpr);
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/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<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,12 @@
"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_strncat_large_size : Warning<
"the value of the size argument in 'strncat' is too large, might lead to a "
"buffer overflow">, InGroup<StrncatSize>;
Index: clang/include/clang/Basic/DiagnosticGroups.td
===================================================================
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ clang/include/clang/Basic/DiagnosticGroups.td
@@ -442,6 +442,12 @@
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 SuspiciousMemaccess : DiagGroup<"suspicious-memaccess",
+ [SizeofPointerMemaccess, DynamicClassMemaccess,
+ NonTrivialMemaccess, MemsetTransposedArgs]>;
def StaticInInline : DiagGroup<"static-in-inline">;
def StaticLocalInInline : DiagGroup<"static-local-in-inline">;
def GNUStaticFloatInit : DiagGroup<"gnu-static-float-init">;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
