[Sema] Add a new warning, -Wmemset-transposed-args

epilk · epilk · commit d1cf276621a7 · 2018-07-19T16:46:15.000Z
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 llvm-svn: 337470
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -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">;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -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>;
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
@@ -8629,24 +8629,26 @@ static const CXXRecordDecl *getContainedDynamicClass(QualType T,
   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 CallExpr *Call,
                                      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);
diff --git a/clang/test/Sema/transpose-memset.c b/clang/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}}
+}
