[StaticAnalyzer] Fix crash in CastToStructChecker

[danielmarjamaki]

Summary

This fix the crash reported in https://llvm.org/bugs/show_bug.cgi?id=31173

[NoQ]

Looks good. I assume the crash is in getTypeInfo(); do you have any idea what are exact prerequisites for using this method? So that there were no more crashes here.

[zaks.anna]

Please, subscribe cfe-commits list on the patches as described in http://llvm.org/docs/Phabricator.html.

Thanks!
Anna

[danielmarjamaki]

In D28297#642523, @NoQ wrote:

Looks good. I assume the crash is in getTypeInfo(); do you have any idea what are exact prerequisites for using this method? So that there were no more crashes here.

Yes. The crash happens during the getTypeInfo() call. I don't know what prerequisites are interesting to check.

The Type pointer returned by getTypePtr() must be nonnull and valid. The method clang::Type::getTypeClass() is called using that type pointer. If that returns Type::Record then the Type pointer is casted to a RecordType. And RecordType::getDecl() is called. The RecordDecl that is returned by that call is passed to getASTRecordLayout() shown below.

The crash occurs on the first assert in this code:

const ASTRecordLayout &
ASTContext::getASTRecordLayout(const RecordDecl *D) const {
  // These asserts test different things.  A record has a definition
  // as soon as we begin to parse the definition.  That definition is
  // not a complete definition (which is what isDefinition() tests)
  // until we *finish* parsing the definition.

  if (D->hasExternalLexicalStorage() && !D->getDefinition())
    getExternalSource()->CompleteType(const_cast<RecordDecl*>(D));
    
  D = D->getDefinition();
  assert(D && "Cannot get layout of forward declarations!");
  assert(!D->isInvalidDecl() && "Cannot get layout of invalid decl!");
  assert(D->isCompleteDefinition() && "Cannot layout type before complete!");
....

I am not sure I can write testcases that prevent regressions but do you think I should add isInvalidDecl() and isCompleteDefinition() also?

[NoQ]

I sometimes wish ASTContext methods just didn't crash :)
Oh well, let's just see if more problems show up.

lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp

93 ↗

(On Diff #83062)

I think we should move the check here then. That'd avoid double-checking if ToPointeeTy is a record type (we could cast<> directly on this branch).

[zaks.anna]

Please, make sure future reviews go through cfg-dev list. See http://llvm.org/docs/Phabricator.html.

[danielmarjamaki]

I reverted the change because there were buildbot failures.

[danielmarjamaki]

It was reported in the bugzilla report that my first fix did not fix all crashes. A new example code was provided that triggered a new crash. I have updated the patch so both crashes are fixed.

[danielmarjamaki]

I have updated the patch and want a new review.

[a.sidorin]

Looks like a right fix.

lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp

93 ↗

(On Diff #83062)

Or just hoist it out of condition?

[zaks.anna]

Thanks. looks good.

cfe/trunk/lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp

if (!OrigPointeeTy->isRecordType()) {
     const ValueDecl *VD = nullptr;
     if (const auto *SE = dyn_cast<DeclRefExpr>(U->getSubExpr()))
       VD = dyn_cast<ValueDecl>(SE->getDecl());
     else if (const auto *SE = dyn_cast<MemberExpr>(U->getSubExpr()))
       VD = SE->getMemberDecl();
     if (!VD || VD->getType()->isReferenceType())
       return true;
 
+    if (ToPointeeTy->isIncompleteType() ||
+        OrigPointeeTy->isIncompleteType())
+      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;
 
     PathDiagnosticLocation Loc(CE, BR.getSourceManager(), AC);
     BR.EmitBasicReport(AC->getDecl(), Checker, "Widening cast to struct type",

cfe/trunk/test/Analysis/cast-to-struct.cpp

 void intToStruct(int *P) {
   struct ABC *Abc;
   Abc = (struct ABC *)P; // expected-warning {{Casting a non-structure type to a structure type and accessing a field can lead to memory access errors or data corruption}}
 
   // Cast from void *.
   void *VP = P;
   Abc = (struct ABC *)VP;
 }
+
+// https://llvm.org/bugs/show_bug.cgi?id=31173
+void dontCrash1(struct AB X) {
+  struct UndefS *S = (struct UndefS *)&X;
+}
+
+struct S;
+struct T {
+  struct S *P;
+};
+extern struct S Var1, Var2;
+void dontCrash2() {
+  ((struct T *) &Var1)->P = &Var2;
+}