[Sema] Implement -Wmemset-transposed-args

[erik.pilkington]

Summary

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 sizeofexpression. 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

[• Quuxplusone]

clang/include/clang/Basic/DiagnosticSemaKinds.td

662

If it were my codebase, I'd rather see a cast to (unsigned char) than a cast to (int). (The second argument to memset is supposed to be a single byte.) Why did you pick (int) specifically?

clang/lib/Sema/SemaChecking.cpp

7962

Honestly, if the second argument to memset involves sizeof *at all*, it's probably a bug, isn't it?

memset(&buf, sizeof foo + 10, 0xff);
memset(&buf, sizeof foo * sizeof bar, 0xff);

Should Clang really go out of its way to silence the warning in these cases?

[erik.pilkington]

Address @Quuxplusone comments.

clang/include/clang/Basic/DiagnosticSemaKinds.td

662

I chose int because that's the actual type of the second parameter to memset, it just gets casted down to unsigned char internally. FWIW, either type will suppress the warning. I'm fine with recommending unsigned char if you have a strong preference for it.

clang/lib/Sema/SemaChecking.cpp

7962

Sure, that seems reasonable. The new patch makes this check less conservative.

[thakis]

lgtm assuming you ran this on some large code base to make sure it doesn't have false positives.

clang/include/clang/Basic/DiagnosticSemaKinds.td

659

nit: stay below 80 columns

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

662

My preference is for int as well.

clang/lib/Sema/SemaChecking.cpp

7975–7976

This functionality should apply equally to wmemset() as well, should it not? The only difference I can think of would be that the type should be cast to wchar_t instead of int to silence the warning.

7997

Spurious whitespace.

[• Quuxplusone]

clang/include/clang/Basic/DiagnosticSemaKinds.td

662

Personally I have a strong preference for "any 8-bit type" — the important thing is that it needs to indicate to the reader that it's intended to be the single-byte value that memset expects. I even want my compiler to diagnose memset(x, 0x1234, z) as a bug!
But I'm not inclined to fight hard, because this is all about the mechanism of *suppressing* of the warning, and I don't imagine we'll see too many people trying to suppress it.

[erik.pilkington]

In D49112#1158793, @thakis wrote:

lgtm assuming you ran this on some large code base to make sure it doesn't have false positives.

Sorry for the delay here, I was running this on some very large internal code bases. This is overwhelmingly true-positive, but there are 2 improvements I want to make here before landing this:

• Suppress the diagnostic in cases like memset(ptr, 0xff, PADDING), when PADDING is a macro defined to be 0.
• Some platforms #define bzero to __builtin_memset, so we should recognize when we're in a macro expansion to bzero and emit a more accurate diagnostic for bzero(ary, 0);. We might as well add support for __builtin_bzero too.

clang/lib/Sema/SemaChecking.cpp

7975–7976

Looks like clang doesn't have a builtin for wmemset, its just defined as a normal function. I suppose that we could still try to diagnose calls to top-level functions named wmemset, but thats unprecedented and doesn't really seem worth the trouble.

[• Quuxplusone]

clang/lib/Sema/SemaChecking.cpp

7984

Suppress the diagnostic in cases like memset(ptr, 0xff, PADDING), when PADDING is a macro defined to be 0.

Would it suffice to treat a literal 0xFF in the exact same way you treat a literal 0? That is,

if (SecondArg is integer literal 0 or 255 || ThirdArg involves sizeof) {
    do nothing
} else if (ThirdArg is integer literal 0 or 255 || SecondArg involves sizeof) {
    diagnose it
}

You should add a test case proving that memset(x, 0, 0) doesn't get diagnosed (assuming the two 0s come from two different macro expansions, for example).
My sketch above would fail to diagnose memset(x, 0, 255). I don't know if this is a feature or a bug. :) You probably ought to have a test case for that, either way, just to demonstrate the expected behavior.

[aaron.ballman]

clang/lib/Sema/SemaChecking.cpp

7975–7976

I'm fine leaving it off then; I agree it's not worth the trouble.

[erik.pilkington]

In this new patch:

• Add support for builtin_bzero (-Wsuspicious-bzero), improve diagnostics for platforms that define bzero to builtin_memset
• Suppress the diagnostic when the 0 in memset(ptr, something, 0) came from a macro expansion.
• Address review comments

Thanks!

[erik.pilkington]

clang/lib/Sema/SemaChecking.cpp

7984

I definitely don't think that ThirdArg being 255 should lead to a diagnostic, regardless of what SecondArg is. I'm fine with omitting a diagnostic in memset(ptr, 255, 0), but if the user explicitly spelled out 0 then I would probably prefer to diagnose. FWIW, for the case I found where we emitted a false-positive the second argument was 0xd9 or something.

[aaron.ballman]

LGTM aside from a minor nit.

clang/lib/Sema/SemaChecking.cpp

8805

const auto *

clang/include/clang/Basic/DiagnosticGroups.td

 def : DiagGroup<"sign-promo">;
 def SignCompare : DiagGroup<"sign-compare">;
 def : DiagGroup<"stack-protector">;
 def : DiagGroup<"switch-default">;
 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 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">;
 def StaticFloatInit : DiagGroup<"static-float-init", [GNUStaticFloatInit]>;
 def GNUStatementExpression : DiagGroup<"gnu-statement-expression">;
 def StringCompare : DiagGroup<"string-compare">;
 def StringPlusInt : DiagGroup<"string-plus-int">;
 def StringPlusChar : DiagGroup<"string-plus-char">;

clang/include/clang/Basic/DiagnosticSemaKinds.td

             "be inlined into function %0 that is compiled without support for "
             "'%2'">;
 def warn_builtin_unknown : Warning<"use of unknown builtin %0">,
   InGroup<ImplicitFunctionDeclare>, DefaultError;
 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<
   "'%0' call operates on objects of type %1 while the size is based on a " 
   "different type %2">, 
   InGroup<SizeofPointerMemaccess>;
 def warn_sizeof_pointer_expr_memaccess_note : Note<
   "did you mean to %select{dereference the argument to 'sizeof' (and multiply "

   "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 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_strncat_large_size : Warning<
   "the value of the size argument in 'strncat' is too large, might lead to a " 
   "buffer overflow">, InGroup<StrncatSize>;
 def warn_strncat_src_size : Warning<"size argument in 'strncat' call appears " 
   "to be size of the source">, InGroup<StrncatSize>;
 def warn_strncat_wrong_size : Warning<
   "the value of the size argument to 'strncat' is wrong">, InGroup<StrncatSize>;
 def note_strncat_wrong_size : Note<

clang/lib/Sema/SemaChecking.cpp

       IsContained = true;
       return ContainedRD;
     }
   }
 
   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();
 }
 
 namespace {
 
 struct SearchNonTrivialToInitializeField
     : DefaultInitializedTypeVisitor<SearchNonTrivialToInitializeField> {
   using Super =

   ASTContext &getContext() { return S.getASTContext(); }
 
   const Expr *E;
   Sema &S;
 };
 
 }
 
+/// 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
 /// arguments to the standard 'memset', 'memcpy', 'memmove', and 'memcmp'
 /// function calls.
 ///
 /// \param Call The call expression to diagnose.
 void Sema::CheckMemaccessArguments(const CallExpr *Call,

   unsigned LenArg =
       (BId == Builtin::BIbzero || BId == Builtin::BIstrndup ? 1 : 2);
   const Expr *LenExpr = Call->getArg(LenArg)->IgnoreParenImpCasts();
 
   if (CheckMemorySizeofForComparison(*this, LenExpr, FnName,
                                      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);
   llvm::FoldingSetNodeID SizeOfArgID;
 
   // Although widely used, 'bzero' is not a standard function. Be more strict
   // with the argument types before allowing diagnostics and only allow the
   // form bzero(ptr, sizeof(...)).

           E->getType()->isObjCQualifiedIdType()) &&
          "EvalAddr only works on pointers");
 
   E = E->IgnoreParens();
 
   // Our "symbolic interpreter" is just a dispatch off the currently
   // viewed AST node.  We then recursively traverse the AST by calling
   // EvalAddr and EvalVal appropriately.
   switch (E->getStmtClass()) {

clang/test/Sema/transpose-memset.c

+// 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
+}