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 ^

75

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

85

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.

75

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.

85

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.

85

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

85

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

85

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

85

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

64–97

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/AST/RecursiveASTVisitor.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 CastToStructVisitor : public RecursiveASTVisitor<CastToStructVisitor> {
+  BugReporter &BR;
+  const CheckerBase *Checker;
+  AnalysisDeclContext *AC;
 
 public:
-  void checkPreStmt(const CastExpr *CE, CheckerContext &C) const;
+  explicit CastToStructVisitor(BugReporter &B, const CheckerBase *Checker,
+                               AnalysisDeclContext *A)
+      : BR(B), Checker(Checker), AC(A) {}
+  bool VisitCastExpr(const CastExpr *CE);
 };
 }
 
-void CastToStructChecker::checkPreStmt(const CastExpr *CE,
-                                       CheckerContext &C) const {
+bool CastToStructVisitor::VisitCastExpr(const CastExpr *CE) {
   const Expr *E = CE->getSubExpr();
-  ASTContext &Ctx = C.getASTContext();
+  ASTContext &Ctx = AC->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 true;
 
   QualType OrigPointeeTy = OrigPTy->getPointeeType();
   QualType ToPointeeTy = ToPTy->getPointeeType();
 
   if (!ToPointeeTy->isStructureOrClassType())
-    return;
+    return true;
 
   // We allow cast from void*.
   if (OrigPointeeTy->isVoidType())
-    return;
+    return true;
 
   // 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));
-    }
-  }
+    SourceRange Sr[1] = {CE->getSourceRange()};
+    PathDiagnosticLocation Loc(CE, BR.getSourceManager(), AC);
+    BR.EmitBasicReport(
+        AC->getDecl(), Checker, "Cast from non-struct type to struct type",
+        categories::LogicError, "Casting a non-structure type to a structure "
+                                "type and accessing a field can lead to memory "
+                                "access errors or data corruption.",
+        Loc, Sr);
+  } else {
+    // Don't warn when size of data is unknown.
+    const auto *U = dyn_cast<UnaryOperator>(E);
+    if (!U || U->getOpcode() != UO_AddrOf)
+      return true;
+
+    // Warn when there is widening cast.
+    unsigned ToWidth = Ctx.getTypeInfo(ToPointeeTy).Width;
+    unsigned OrigWidth = Ctx.getTypeInfo(OrigPointeeTy).Width;
+    if (ToWidth <= OrigWidth)
+      return true;
+
+    // Don't warn when resulting struct type has base class.
+    const RecordDecl *RD = ToPointeeTy->getAsStructureType()->getDecl();
+    if (const auto *D = dyn_cast<CXXRecordDecl>(RD))
+      if (D->bases_begin() != D->bases_end())
+        return true;
+
+    SourceRange Sr[1] = {CE->getSourceRange()};
+    PathDiagnosticLocation Loc(CE, BR.getSourceManager(), AC);
+    BR.EmitBasicReport(AC->getDecl(), Checker, "Widening cast to struct type",
+                       categories::LogicError,
+                       "Casting data to a larger structure type and accessing "
+                       "a field can lead to memory access errors or data "
+                       "corruption.",
+                       Loc, Sr);
+
+  return true;
+
+namespace {
+class CastToStructChecker : public Checker<check::ASTCodeBody> {
+public:
+  void checkASTCodeBody(const Decl *D, AnalysisManager &Mgr,
+                        BugReporter &BR) const {
+    CastToStructVisitor Visitor(BR, this, Mgr.getAnalysisDeclContext(D));
+    Visitor.TraverseDecl(const_cast<Decl *>(D));
   }
+};
+} // end anonymous namespace
 
 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:
     ;
   }
 }