[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 ↗

(On Diff #154727)

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 ↗

(On Diff #154727)

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 ↗

(On Diff #154727)

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 ↗

(On Diff #154727)

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 ↗

(On Diff #154833)

nit: stay below 80 columns

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

662 ↗

(On Diff #154727)

My preference is for int as well.

clang/lib/Sema/SemaChecking.cpp

7975–7976 ↗

(On Diff #154833)

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 ↗

(On Diff #154833)

Spurious whitespace.

[• Quuxplusone]

clang/include/clang/Basic/DiagnosticSemaKinds.td

662 ↗

(On Diff #154727)

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 ↗

(On Diff #154833)

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 ↗

(On Diff #154833)

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 ↗

(On Diff #154833)

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 ↗

(On Diff #154833)

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

8751 ↗

(On Diff #156178)

const auto *

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 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">;
 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">;

include/clang/Basic/DiagnosticSemaKinds.td

: Error<"always_inline function %1 requires target feature '%2', but would "
             "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 "
def note_strlcpycat_wrong_size : Note<
   "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_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>;
 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<

lib/Sema/SemaChecking.cpp

if (const CXXRecordDecl *ContainedRD =
       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 QualType();
 }
 
 namespace {
 
 struct SearchNonTrivialToInitializeField
     : DefaultInitializedTypeVisitor<SearchNonTrivialToInitializeField> {
   using Super =
struct SearchNonTrivialToCopyField
   ASTContext &getContext() { return S.getASTContext(); }
 
   const Expr *E;
   Sema &S;
 };
 
 }
 
+/// 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
 /// arguments to the standard 'memset', 'memcpy', 'memmove', and 'memcmp'
 /// function calls.
 ///
 /// \param Call The call expression to diagnose.
 void Sema::CheckMemaccessArguments(const CallExpr *Call,
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)'.
+  CheckMemaccessSize(*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(...)).

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__)
+#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}}
+}