Improve alpha.core.CastToStruct warn about widening casts of structs also

[danielmarjamaki]

Summary

The alpha.core.CastToStruct warns when for instance casting a int pointer to a struct pointer. As accessing a field can lead to memory access errors.

This patch improves the checker so it also warns about widening casts of structs.

[NoQ]

Could we re-use some code between the two checks, like obtaining the type of the operands, and throwing the bug report?

P.S. A thing to notice: this checker shouldn't be path-sensitive, it doesn't use any path-sensitive information.

[danielmarjamaki]

P.S. A thing to notice: this checker shouldn't be path-sensitive, it doesn't use any path-sensitive information.

Yes I do wonder about that.

As the checker is written right now it seems to me it could be moved to clang-tidy.

But I am thinking we could use path-sensitive analysis in this check and see if the cast leads to memory access problems.

[NoQ]

But I am thinking we could use path-sensitive analysis in this check and see if the cast leads to memory access problems.

Yeah, in particular, we could also see if the suspicious cast is actually a revert of a previous cast. No way it's blocking this review though.

[danielmarjamaki]

Reuse code, and use RecursiveASTVisitor.

[danielmarjamaki]

ping

[NoQ]

Thanks!! It looks a lot better now.

lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp

1

Ouch, just noticed ^

80

Could you add a test case for other branches of this code (cast without unary operator, cast with non-& unary operator)?

90

Could you add a test case for this? It might force you to add a C++ test file. Adding a dedicated file of tests for this checker is anyway a good idea, i think.

You may also want to make this more accurate with CXXRecordDecl::isDerivedFrom() etc. if you like.

[danielmarjamaki]

lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp

1

I am not sure what problem you see with this line. It looks ok to me.

80

Ok. Maybe the getOpCode is redundant. The ~, !, - operators does not make much sense on pointers and I believe I get compile errors. However imho it's nice for clarity to show that I only look for &var.

90

I started writing a testcase. But I am now thinking about removing this code. I could not write a sensible test case.

If there is code like:

struct Derived : public Base { .. };
struct Base B = {1,2};
struct Derived *D = (struct Derived *)&B;

Then writing the warning seems ok. It seems to me this is just as dangerous as if there was no inheritance.

[danielmarjamaki]

Added testfile. Remove code for derived records.

[NoQ]

lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp

1

The description is from another checker.

90

Then you should exclude dynamic_cast and probably static_cast as well, because they were created for this particular purpose. Even then, i'm afraid many projects still use C-style casts with combination of kind-of custom RTTI (even in C) and have large switches by kind followed by casts to kind everywhere. You should be able to see this if you run the checker on a large codebase - even though the code you posted is clearly invalid, you cannot prove this without proper path-sensitive analysis, and the cast itself isn't necessarily bad.

[danielmarjamaki]

lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp

90

I think you're missing that I am not writing warnings about arbitrary struct pointers.

The dynamic_cast is normally used to cast some kind of pointer variable:

Base *B;
...
Derived *D = dynamic_cast<Derived *>(B);

I do not warn about casting such pointers. Because the check does not know what B points at.

I require that '&' is used, like so:

Base B;
...
Derived *D = dynamic_cast<Derived *>(&Base);

Then the check knows what the pointer points at. It's just a Base. I guess you are telling me that we should not warn here. Well you are right, nothing dangerous happens the cast result will just be 0.

[NoQ]

Thanks for the tests!

lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp

90

Uhm, but the operator-&-heuristic sounds like something extremely restrictive, that should be eventually removed (not even a pair of parentheses is allowed around operator-&).

Some false positives may still appear though, i suspect:

Derived D;
Base &B = D;
Derived *Dptr = (D*)&B; // no-warning

^You can find an example of such code in MallocChecker::checkPreCall, the way they cast CallEvent references. Right now i'm not sure how popular correct and incorrect code patterns of this kind are.

Why i think the heuristic is too restrictive:

Base B;
Base *Bptr = &B;
Derived *Dptr = (D*)B; // expected-warning{{}}

[danielmarjamaki]

Handle case between Base/Derived types better.

[danielmarjamaki]

Ping.

lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp

90

thanks!

yes I agree it's extremely restricted. of course a pair of parentheses should not cause false negatives though.

imho , path-sensitive analysis is needed to warn about "pointers" and "references" as the check must know what it points at / references to avoid false positives.

[NoQ]

This patch seems to be an improvement for the original checker, and i think it should be ok to commit if your evaluation of the new check shows reasonable true positive rate.

However, if i was to choose, based on this discussion and other experiences, i would have wished this checker go in a different direction: make a single checker for violations of the strict aliasing rule. This should be do-able as a classic path-sensitive checker which would warn if the same object (RegionOffset) is accessed assuming incompatible value types. For symbolic void* or char* pointers, such checker would need to catch two accesses (which forces us to register stuff with program state), while for other pointer symbols and for concrete regions perhaps a single access should be enough to detect the problem. Such checker would probably be improved by finding branches that introduce aliases between pointer symbols, eg.

void foo(int *x, struct S *y) {
  if (x == y) {
    ...
  }
}

which we don't handle very well in the path-sensitive engine, but it's a great place to start looking into this. I wish somebody looked into this, because it sounds like a really good task for the analyzer.

lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp

69–132

You can use a single Sr here, the convenient ArrayRef constructor from a single element allows passing a single element instead of a one-element array into EmitBasicReport.

[danielmarjamaki]

However, if i was to choose, based on this discussion and other experiences, i would have wished this checker go in a different direction: make a single checker for violations of the strict aliasing rule.

That is reasonable. Ideally everybody would fix all UB.

As far as I know it's common to break the strict aliasing rule. There are users that would rather use -fno-strict-aliasing than fix their code. I want to have a checker that is useful when -fno-strict-aliasing is used and detects access out of bounds.

lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp

 //=== CastToStructChecker.cpp - Fixed address usage checker ----*- C++ -*--===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This files defines CastToStructChecker, a builtin checker that checks for
 // cast from non-struct pointer to struct pointer.
 // This check corresponds to CWE-588.
 //
 //===----------------------------------------------------------------------===//
 
 #include "ClangSACheckers.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 
 using namespace clang;
 using namespace ento;
 
 namespace {
-class CastToStructChecker : public Checker< check::PreStmt<CastExpr> > {
-  mutable std::unique_ptr<BuiltinBug> BT;
+class CastToStructChecker : public Checker<check::PreStmt<CastExpr>> {
+  mutable std::unique_ptr<BuiltinBug> BTNonStruct;
+  mutable std::unique_ptr<BuiltinBug> BTWidening;
+
+  bool nonStructPointerToStructPointerCast(const CastExpr *CE,
+                                           CheckerContext &C) const;
+  bool wideningStructPointerCast(const CastExpr *CE, CheckerContext &C) const;
 
 public:
   void checkPreStmt(const CastExpr *CE, CheckerContext &C) const;
 };
 }
 
 void CastToStructChecker::checkPreStmt(const CastExpr *CE,
                                        CheckerContext &C) const {
+  if (!nonStructPointerToStructPointerCast(CE, C))
+    wideningStructPointerCast(CE, C);
+}
+
+bool CastToStructChecker::nonStructPointerToStructPointerCast(
+    const CastExpr *CE, CheckerContext &C) const {
   const Expr *E = CE->getSubExpr();
   ASTContext &Ctx = C.getASTContext();
   QualType OrigTy = Ctx.getCanonicalType(E->getType());
   QualType ToTy = Ctx.getCanonicalType(CE->getType());
 
   const PointerType *OrigPTy = dyn_cast<PointerType>(OrigTy.getTypePtr());
   const PointerType *ToPTy = dyn_cast<PointerType>(ToTy.getTypePtr());
 
   if (!ToPTy || !OrigPTy)
-    return;
+    return false;
 
   QualType OrigPointeeTy = OrigPTy->getPointeeType();
   QualType ToPointeeTy = ToPTy->getPointeeType();
 
   if (!ToPointeeTy->isStructureOrClassType())
-    return;
+    return false;
 
   // We allow cast from void*.
   if (OrigPointeeTy->isVoidType())
-    return;
+    return false;
 
   // Now the cast-to-type is struct pointer, the original type is not void*.
-  if (!OrigPointeeTy->isRecordType()) {
-    if (ExplodedNode *N = C.generateNonFatalErrorNode()) {
-      if (!BT)
-        BT.reset(
-            new BuiltinBug(this, "Cast from non-struct type to struct type",
-                           "Casting a non-structure type to a structure type "
-                           "and accessing a field can lead to memory access "
-                           "errors or data corruption."));
-      auto R = llvm::make_unique<BugReport>(*BT, BT->getDescription(), N);
-      R->addRange(CE->getSourceRange());
-      C.emitReport(std::move(R));
-    }
+  if (OrigPointeeTy->isRecordType())
+    return false;
+
+    if (!BTNonStruct)
+      BTNonStruct.reset(
+    auto R = llvm::make_unique<BugReport>(*BTNonStruct,
+                                          BTNonStruct->getDescription(), N);
+  return true;
+}
+
+bool CastToStructChecker::wideningStructPointerCast(const CastExpr *Cast,
+                                                    CheckerContext &C) const {
+  const UnaryOperator *SourceExpr =
+      dyn_cast<UnaryOperator>(Cast->IgnoreParenCasts());
+  if (!SourceExpr || SourceExpr->getOpcode() != UO_AddrOf)
+    return false;
+
+  QualType CastType = Cast->getType();
+  if (!CastType->isPointerType() || !CastType->getPointeeType()->isRecordType())
+    return false;
+  QualType SourceExprType = Cast->IgnoreParenCasts()->getType();
+  if (!SourceExprType->isPointerType() ||
+      !SourceExprType->getPointeeType()->isRecordType())
+    return false;
+
+  ASTContext &Context = C.getASTContext();
+
+  unsigned CastTypeWidth =
+      Context.getTypeInfo(CastType->getPointeeType()).Width;
+  unsigned SubTypeWidth =
+      Context.getTypeInfo(SourceExprType->getPointeeType()).Width;
+
+  if (CastTypeWidth <= SubTypeWidth)
+    return false;
+
+  const RecordType *R = CastType->getPointeeType()->getAsStructureType();
+  if (!R)
+    return false;
+
+  // Don't warn when resulting struct type has base class.
+  if (const auto *D = dyn_cast<CXXRecordDecl>(R->getDecl())) {
+    if (D->bases_begin() != D->bases_end())
+      return false;
+  }
+
+  if (ExplodedNode *N = C.generateNonFatalErrorNode()) {
+    if (!BTWidening)
+      BTWidening.reset(
+          new BuiltinBug(this, "Cast to struct type",
+                         "Casting data to a larger structure type "
+                         "and accessing a field can lead to memory access "
+                         "errors or data corruption."));
+    auto R = llvm::make_unique<BugReport>(*BTWidening,
+                                          BTWidening->getDescription(), N);
+    R->addRange(Cast->getSourceRange());
+    C.emitReport(std::move(R));
   }
+  return true;
 }
 
 void ento::registerCastToStructChecker(CheckerManager &mgr) {
   mgr.registerChecker<CastToStructChecker>();
 }

test/Analysis/casts.c

 typedef __darwin_socklen_t socklen_t;
 struct sockaddr { sa_family_t sa_family; };
 struct sockaddr_storage {};
 
 void getsockname();
 
 void f(int sock) {
   struct sockaddr_storage storage;
-  struct sockaddr* sockaddr = (struct sockaddr*)&storage;
+  struct sockaddr* sockaddr = (struct sockaddr*)&storage; // expected-warning{{Casting data to a larger structure type and accessing a field can lead to memory access errors or data corruption}}
   socklen_t addrlen = sizeof(storage);
   getsockname(sock, sockaddr, &addrlen);
   switch (sockaddr->sa_family) { // no-warning
   default:
     ;
   }
 }
 

test/Analysis/struct-struct.c

+// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.core -verify %s
+
+struct A {
+  int a;
+};
+
+struct AB {
+  int a;
+  int b;
+};
+
+void f() {
+  struct A a;
+  struct AB *ab = (struct AB *)&a; // expected-warning{{Casting data to a larger structure type and accessing a field can lead to memory access errors or data corruption}}
+}
+
+// Do not warn about casting a pointer variable. The data it points at might
+// be large enough.
+void dontwarn(struct A *a) {
+  struct AB *ab = (struct AB *)a;
+}