Warn on mismatching types to sizeof for memset and friends where length is of the form sizeof(Type) * factor.

[ochang]

Summary

Slightly cleaned up patch stolen from thakis@.

[thakis]

The code looks reasonable to me, but others on cfe-commits know Sema better than I.

clang generally has a high bar for adding warnings. To evaluate a warning, we usually run it on some large code base and measure true and false positives and then compare the severity of the true positives with the noise from the false positives. Do you have any data on what the warning warns on, how serious the bugs it finds are, and if there are any common patterns this warns on but shouldn't?

include/clang/Basic/DiagnosticSemaKinds.td

435

I wonder if this should have its own flag instead of going into the existing group – else this might be annoying for people who have the current version of this warning enabled. I'm not sure though. (Do others have opinions?)

lib/Sema/SemaChecking.cpp

4741

It feels like we should have a function for this already…I couldn't find one either though.

4758

clang uses a 2 space indent. Please run clang-format on your patch.

4915

Should this also emit a "cast to (void*) to suppress warning" fixit like other warnings here do?

If the dest is an array with a known size, and the literal is the size of the array, should we suggest sizeof(variablename) instead?

test/SemaCXX/warn-memset-bad-sizeof.cpp

4

Tests should generally be standalone and not include any headers. What do you need this for? size_t? If so, do typedef __SIZE_TYPE__ size_t; instead.

[ochang]

clang-format and remove header file dependency from test.

[ochang]

Here are some general results after some quick testing (some of which is stolen from thakis's testing on chromium). Unfortunately some of these warnings were difficult to triage and were a little confusing (which I think makes a point for this warning).

Discounting duplicates in the same file, both Firefox and Chromium give around 8-10 warnings.

Some patterns observed:

• Most seem to just be based on size assumptions between types, e.g. 2 * sizeof(uint16_t) == sizeof(uint32_t).

Firefox had about 2 instances where 2 different classes were expected to have the same size (e.g. when 1 is a wrapper around another).

• In Chromium there are some issues with structs and unions that solely contained members of a single type.

e.g.

struct M {
  int A[4];
};

M a;
memset(&a, 0, sizeof(int) * 4);

thakis@ suggested decaying the struct type to the member type in this case for the comparison.

• Firefox also had about 2 warnings related to multidimensional arrays, e.g.

typedef int Foo[10];
Foo m;
memset(&m, 0, sizeof(Foo));

Perhaps we can just compare the type 'Foo' in this case before trying to break it down further in this case.

• There are some other warnings such as differences complex_t vs float[2], w_char vs short.

[thakis]

Did the warning find any legitimate bugs?

[ochang]

Unfortunately, apart from the one mentioned in the chromium tracker, this does not seem to uncover any new legitimate bugs. Apart from Chromium and Firefox, I've tried compiling ruby and python.

include/clang/Basic/DiagnosticSemaKinds.td

   "argument to 'sizeof' in %0 call is the same pointer type %1 as the "
   "%select{destination|source}2; expected %3 or an explicit length">,
   InGroup<SizeofPointerMemaccess>;
+def warn_sizeof_pointer_product_memaccess : Warning<
+  "argument to 'sizeof' in %0 call has a different type %1 as the "
+  "%select{destination|source}2; expected %3">,
+  InGroup<SizeofPointerMemaccess>;
 def warn_strlcpycat_wrong_size : Warning<
   "size argument in %0 call appears to be size of the source; "
   "expected the size of the destination">,

lib/Sema/SemaChecking.cpp

 }
 
 /// \brief If E is a sizeof expression, returns its argument type.
-static QualType getSizeOfArgType(const Expr *E) {
+static QualType getSizeOfArgType(const Expr *E, bool RequireType = false) {
   if (const UnaryExprOrTypeTraitExpr *SizeOf =
           dyn_cast<UnaryExprOrTypeTraitExpr>(E))
-    if (SizeOf->getKind() == clang::UETT_SizeOf)
+    if (SizeOf->getKind() == clang::UETT_SizeOf &&
+        (!RequireType || SizeOf->isArgumentType()))
       return SizeOf->getTypeOfArgument();
 
   return QualType();
 }
 
+/// \brief Tries to deconstruct E into the product of sizeof(type) and Sub.
+static bool getSizeOfArgTypeProduct(const Expr *E, QualType &SizeOf,
+                                    const Expr *&Factor) {
+  const auto *Product = dyn_cast<BinaryOperator>(E);
+  if (!Product || Product->getOpcode() != BO_Mul)
+    return false;
+
+  QualType SizeOfType;
+  SizeOfType = getSizeOfArgType(Product->getLHS(), true);
+  if (SizeOfType != QualType()) {
+    SizeOf = SizeOfType;
+    Factor = Product->getRHS();
+    return true;
+  }
+  SizeOfType = getSizeOfArgType(Product->getRHS(), true);
+  if (SizeOfType != QualType()) {
+    SizeOf = SizeOfType;
+    Factor = Product->getLHS();
+    return true;
+  }
+  return false;
+}
+
+static bool typesAreCompatibleIgnoreQualifiers(ASTContext &Context,
+                                               QualType &LHSType,
+                                               QualType &RHSType) {
+  if (Context.typesAreCompatible(LHSType, RHSType))
+    return true;
+
+  if (LHSType->isAnyPointerType() && RHSType->isAnyPointerType()) {
+    QualType LHSPointee = LHSType->getPointeeType().getUnqualifiedType();
+    QualType RHSPointee = RHSType->getPointeeType().getUnqualifiedType();
+    return typesAreCompatibleIgnoreQualifiers(Context, LHSPointee, RHSPointee);
+  }
+  return false;
+}
+
+static bool isSizeOfArgFactorTypeCompatible(ASTContext &Context,
+                                            QualType &SizeOfArgFactorTy,
+                                            QualType &EltType) {
+  QualType LHSType =
+      Context.getCanonicalType(SizeOfArgFactorTy).getUnqualifiedType();
+  QualType RHSType = Context.getCanonicalType(EltType).getUnqualifiedType();
+
+  if (LHSType->hasSignedIntegerRepresentation() &&
+      LHSType != Context.getWCharType())
+    LHSType = Context.getCorrespondingUnsignedType(LHSType);
+
+  if (RHSType->hasSignedIntegerRepresentation() &&
+      RHSType != Context.getWCharType())
+    RHSType = Context.getCorrespondingUnsignedType(RHSType);
+
+  return typesAreCompatibleIgnoreQualifiers(Context, LHSType, RHSType);
+}
+
 /// \brief Check for dangerous or invalid arguments to memset().
 ///
 /// This issues warnings on known problematic, dangerous or unspecified

                                      Call->getLocStart(), Call->getRParenLoc()))
     return;
 
-  // We have special checking when the length is a sizeof expression.
+  // We have special checking when the length is a sizeof expression, or
+  // a product where one factor is a sizeof expression.
   QualType SizeOfArgTy = getSizeOfArgType(LenExpr);
   const Expr *SizeOfArg = getSizeOfExprArg(LenExpr);
+
+  QualType SizeOfArgFactorTy;
+  const Expr *OtherSizeOfFactor;
+  bool SizeOfIsFactor =
+      getSizeOfArgTypeProduct(LenExpr, SizeOfArgFactorTy, OtherSizeOfFactor);
+
   llvm::FoldingSetNodeID SizeOfArgID;
 
   for (unsigned ArgIdx = 0; ArgIdx != LastArg; ++ArgIdx) {

     if (PointeeTy == QualType())
       continue;
 
+    if (SizeOfIsFactor) {
+      QualType EltType(QualType(PointeeTy->getBaseElementTypeUnsafe(), 0));
+
+      if (!EltType->isCharType() &&
+          !isSizeOfArgFactorTypeCompatible(Context, SizeOfArgFactorTy, EltType))
+        DiagRuntimeBehavior(LenExpr->getExprLoc(), Dest,
+                            PDiag(diag::warn_sizeof_pointer_product_memaccess)
+                                << FnName << SizeOfArgFactorTy << ArgIdx
+                                << EltType << Dest->getSourceRange()
+                                << LenExpr->getSourceRange());
+    }
+
     // Always complain about dynamic classes.
     bool IsContained;
     if (const CXXRecordDecl *ContainedRD =

test/Analysis/taint-generic.c

   int *buf1 = (int*)malloc(ts*sizeof(int)); // expected-warning {{Untrusted data is used to specify the buffer size}}
   char *dst = (char*)calloc(ts, sizeof(char)); //expected-warning {{Untrusted data is used to specify the buffer size}}
   bcopy(buf1, dst, ts); // expected-warning {{Untrusted data is used to specify the buffer size}}
-  __builtin_memcpy(dst, buf1, (ts + 4)*sizeof(char)); // expected-warning {{Untrusted data is used to specify the buffer size}}
+  __builtin_memcpy(dst, (char*)buf1, (ts + 4)*sizeof(char)); // expected-warning {{Untrusted data is used to specify the buffer size}}
 
   // If both buffers are trusted, do not issue a warning.
   char *dst2 = (char*)malloc(ts*sizeof(char)); // expected-warning {{Untrusted data is used to specify the buffer size}}

test/SemaCXX/warn-memset-bad-sizeof.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify -Wno-sizeof-array-argument %s
 //
+
+typedef __SIZE_TYPE__ size_t;
+
 extern "C" void *memset(void *, int, unsigned);
 extern "C" void *memmove(void *s1, const void *s2, unsigned n);
 extern "C" void *memcpy(void *s1, const void *s2, unsigned n);

 
 typedef double Mat[4][4];
 
+typedef int IntType;
+typedef unsigned int UIntType;
+
 template <class Dest, class Source>
 inline Dest bit_cast(const Source& source) {
   Dest dest;

   }), 0, sizeof(s));
 }
 
+void f1() {
+  size_t arr[8];
+  size_t arr2[8];
+  int** a = new int*[4];
+  Mat m;
+
+  /* Should warn */
+  memset(arr, 0, 8 * sizeof(int));  // expected-warning {{argument to 'sizeof' in 'memset' call has a different type 'int' as the destination; expected 'size_t'}}
+  memset(a, 0, 4 * sizeof(short*));  // expected-warning {{argument to 'sizeof' in 'memset' call has a different type 'short *' as the destination; expected 'int *'}}
+  memcpy(arr, arr2, 4 * sizeof(int));  // expected-warning {{argument to 'sizeof' in 'memcpy' call has a different type 'int' as the source; expected 'size_t'}}  expected-warning{{argument to 'sizeof' in 'memcpy' call has a different type 'int' as the destination; expected 'size_t'}}
+  memcpy(arr, a, 1 * sizeof(size_t));  // expected-warning {{argument to 'sizeof' in 'memcpy' call has a different type 'size_t' (aka 'unsigned long') as the source; expected 'int *'}}
+
+  /* Shouldn't warn */
+  memcpy(arr, arr2, 4 * sizeof(size_t));
+  memset(arr, 0, 8 * sizeof(size_t));
+  memset(arr, 0, 8 * sizeof(const size_t));
+  memset(&arr, 0, sizeof(size_t) * 3);
+  memset(a, 0, 4 * sizeof(int*));
+  memset(m, 0, sizeof(double) * 16);
+
+  char buf[4 * sizeof(int)];
+  memset(buf, 0, 4 * sizeof(int));
+
+  unsigned int i[4];
+  memset(&i, 0, 4 * sizeof(int));
+
+  IntType i2[4];
+  memset(i2, 0, 4 * sizeof(UIntType));
+
+  wchar_t arr3[8];
+  memset(arr3, 0, 2 * sizeof(wchar_t));
+
+  float arr4[8];
+  memset(arr4, 0, 1 * sizeof(float));
+
+  const int** ca = new const int*[4];
+  memset(ca, 0, 1 * sizeof(int*));
+
+  int const* const** ca2 = new int const* const*[4];
+  memset(ca2, 0, 1 * sizeof(int**));
+  memset(ca2, 0, 1 * sizeof(volatile int **));
+}
+
 namespace ns {
 void memset(void* s, char c, int n);
 void f(int* i) {