[analyzer] Fix analyzer warnings on analyzer.

Write tests for the actual crash that was found. Write comments and refactor
code around 17 style bugs and suppress 3 false positives.

Differential Revision: https://reviews.llvm.org/D66847

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@370246 91177308-0d34-0410-b5e6-96231b3b80d8

include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h

@@ -169,6 +169,7 @@ public:
   Kind getKind() const { return kind; }
 
   template<typename RegionTy> const RegionTy* getAs() const;
+  template<typename RegionTy> const RegionTy* castAs() const;
 
   virtual bool isBoundable() const { return false; }
 
@@ -1231,6 +1232,11 @@ const RegionTy* MemRegion::getAs() const {
   return nullptr;
 }
 
+template<typename RegionTy>
+const RegionTy* MemRegion::castAs() const {
+  return cast<RegionTy>(this);
+}
+
 //===----------------------------------------------------------------------===//
 // MemRegionManager - Factory object for creating regions.
 //===----------------------------------------------------------------------===//

lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp

@@ -156,14 +156,21 @@ bool WalkAST::containsBadStrlcpyStrlcatPattern(const CallExpr *CE) {
   const Expr *DstArg = CE->getArg(0);
   const Expr *LenArg = CE->getArg(2);
 
-  const auto *DstArgDecl = dyn_cast<DeclRefExpr>(DstArg->IgnoreParenImpCasts());
-  const auto *LenArgDecl = dyn_cast<DeclRefExpr>(LenArg->IgnoreParenLValueCasts());
+  const auto *DstArgDRE = dyn_cast<DeclRefExpr>(DstArg->IgnoreParenImpCasts());
+  const auto *LenArgDRE =
+      dyn_cast<DeclRefExpr>(LenArg->IgnoreParenLValueCasts());
   uint64_t DstOff = 0;
   if (isSizeof(LenArg, DstArg))
     return false;
+
   // - size_t dstlen = sizeof(dst)
-  if (LenArgDecl) {
-    const auto *LenArgVal = dyn_cast<VarDecl>(LenArgDecl->getDecl());
+  if (LenArgDRE) {
+    const auto *LenArgVal = dyn_cast<VarDecl>(LenArgDRE->getDecl());
+    // If it's an EnumConstantDecl instead, then we're missing out on something.
+    if (!LenArgVal) {
+      assert(isa<EnumConstantDecl>(LenArgDRE->getDecl()));
+      return false;
+    }
     if (LenArgVal->getInit())
       LenArg = LenArgVal->getInit();
   }
@@ -177,9 +184,10 @@ bool WalkAST::containsBadStrlcpyStrlcatPattern(const CallExpr *CE) {
     // Case when there is pointer arithmetic on the destination buffer
     // especially when we offset from the base decreasing the
     // buffer length accordingly.
-    if (!DstArgDecl) {
-      if (const auto *BE = dyn_cast<BinaryOperator>(DstArg->IgnoreParenImpCasts())) {
-        DstArgDecl = dyn_cast<DeclRefExpr>(BE->getLHS()->IgnoreParenImpCasts());
+    if (!DstArgDRE) {
+      if (const auto *BE =
+              dyn_cast<BinaryOperator>(DstArg->IgnoreParenImpCasts())) {
+        DstArgDRE = dyn_cast<DeclRefExpr>(BE->getLHS()->IgnoreParenImpCasts());
         if (BE->getOpcode() == BO_Add) {
           if ((IL = dyn_cast<IntegerLiteral>(BE->getRHS()->IgnoreParenImpCasts()))) {
             DstOff = IL->getValue().getZExtValue();
@@ -187,8 +195,9 @@ bool WalkAST::containsBadStrlcpyStrlcatPattern(const CallExpr *CE) {
         }
       }
     }
-    if (DstArgDecl) {
-      if (const auto *Buffer = dyn_cast<ConstantArrayType>(DstArgDecl->getType())) {
+    if (DstArgDRE) {
+      if (const auto *Buffer =
+              dyn_cast<ConstantArrayType>(DstArgDRE->getType())) {
         ASTContext &C = BR.getContext();
         uint64_t BufferLen = C.getTypeSize(Buffer) / 8;
         auto RemainingBufferLen = BufferLen - DstOff;

lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp

@@ -204,6 +204,8 @@ void WalkAST::VisitForStmt(ForStmt *FS) {
 // Implements: CERT security coding advisory FLP-30.
 //===----------------------------------------------------------------------===//
 
+// Returns either 'x' or 'y', depending on which one of them is incremented
+// in 'expr', or nullptr if none of them is incremented.
 static const DeclRefExpr*
 getIncrementedVar(const Expr *expr, const VarDecl *x, const VarDecl *y) {
   expr = expr->IgnoreParenCasts();
@@ -289,14 +291,15 @@ void WalkAST::checkLoopConditionForFloat(const ForStmt *FS) {
 
   // Does either variable appear in increment?
   const DeclRefExpr *drInc = getIncrementedVar(increment, vdLHS, vdRHS);
-
   if (!drInc)
     return;
 
+  const VarDecl *vdInc = cast<VarDecl>(drInc->getDecl());
+  assert(vdInc && (vdInc == vdLHS || vdInc == vdRHS));
+
   // Emit the error.  First figure out which DeclRefExpr in the condition
   // referenced the compared variable.
-  assert(drInc->getDecl());
-  const DeclRefExpr *drCond = vdLHS == drInc->getDecl() ? drLHS : drRHS;
+  const DeclRefExpr *drCond = vdLHS == vdInc ? drLHS : drRHS;
 
   SmallVector<SourceRange, 2> ranges;
   SmallString<256> sbuf;

lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp

@@ -394,11 +394,11 @@ static const ObjCObjectPointerType *getMostInformativeDerivedClassImpl(
   }
 
   const auto *SuperOfTo =
-      To->getObjectType()->getSuperClassType()->getAs<ObjCObjectType>();
+      To->getObjectType()->getSuperClassType()->castAs<ObjCObjectType>();
   assert(SuperOfTo);
   QualType SuperPtrOfToQual =
       C.getObjCObjectPointerType(QualType(SuperOfTo, 0));
-  const auto *SuperPtrOfTo = SuperPtrOfToQual->getAs<ObjCObjectPointerType>();
+  const auto *SuperPtrOfTo = SuperPtrOfToQual->castAs<ObjCObjectPointerType>();
   if (To->isUnspecialized())
     return getMostInformativeDerivedClassImpl(From, SuperPtrOfTo, SuperPtrOfTo,
                                               C);
@@ -827,16 +827,15 @@ void DynamicTypePropagation::checkPostObjCMessage(const ObjCMethodCall &M,
   if (MessageExpr->getReceiverKind() == ObjCMessageExpr::Class &&
       Sel.getAsString() == "class") {
     QualType ReceiverType = MessageExpr->getClassReceiver();
-    const auto *ReceiverClassType = ReceiverType->getAs<ObjCObjectType>();
+    const auto *ReceiverClassType = ReceiverType->castAs<ObjCObjectType>();
+    if (!ReceiverClassType->isSpecialized())
+      return;
+
     QualType ReceiverClassPointerType =
         C.getASTContext().getObjCObjectPointerType(
             QualType(ReceiverClassType, 0));
-
-    if (!ReceiverClassType->isSpecialized())
-      return;
     const auto *InferredType =
-        ReceiverClassPointerType->getAs<ObjCObjectPointerType>();
-    assert(InferredType);
+        ReceiverClassPointerType->castAs<ObjCObjectPointerType>();
 
     State = State->set<MostSpecializedTypeArgsMap>(RetSym, InferredType);
     C.addTransition(State);

lib/StaticAnalyzer/Checkers/IteratorChecker.cpp

@@ -567,7 +567,8 @@ void IteratorChecker::checkPostCall(const CallEvent &Call,
   if (Func->isOverloadedOperator()) {
     const auto Op = Func->getOverloadedOperator();
     if (isAssignmentOperator(Op)) {
-      const auto *InstCall = dyn_cast<CXXInstanceCall>(&Call);
+      // Overloaded 'operator=' must be a non-static member function.
+      const auto *InstCall = cast<CXXInstanceCall>(&Call);
       if (cast<CXXMethodDecl>(Func)->isMoveAssignmentOperator()) {
         handleAssign(C, InstCall->getCXXThisVal(), Call.getOriginExpr(),
                      Call.getArgSVal(0));

lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp

@@ -882,18 +882,17 @@ void NonLocalizedStringChecker::checkPreObjCMessage(const ObjCMethodCall &msg,
 
 void NonLocalizedStringChecker::checkPreCall(const CallEvent &Call,
                                              CheckerContext &C) const {
-  const Decl *D = Call.getDecl();
-  if (D && isa<FunctionDecl>(D)) {
-    const FunctionDecl *FD = dyn_cast<FunctionDecl>(D);
-    auto formals = FD->parameters();
-    for (unsigned i = 0,
-                  ei = std::min(unsigned(formals.size()), Call.getNumArgs());
-         i != ei; ++i) {
-      if (isAnnotatedAsTakingLocalized(formals[i])) {
-        auto actual = Call.getArgSVal(i);
-        if (hasNonLocalizedState(actual, C)) {
-          reportLocalizationError(actual, Call, C, i + 1);
-        }
+  const auto *FD = dyn_cast_or_null<FunctionDecl>(Call.getDecl());
+  if (!FD)
+    return;
+
+  auto formals = FD->parameters();
+  for (unsigned i = 0, ei = std::min(static_cast<unsigned>(formals.size()),
+                                     Call.getNumArgs()); i != ei; ++i) {
+    if (isAnnotatedAsTakingLocalized(formals[i])) {
+      auto actual = Call.getArgSVal(i);
+      if (hasNonLocalizedState(actual, C)) {
+        reportLocalizationError(actual, Call, C, i + 1);
       }
     }
   }

lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.cpp

@@ -91,11 +91,13 @@ PathDiagnosticPieceRef MPIBugReporter::RequestNodeVisitor::VisitNode(
     return nullptr;
 
   const Request *const Req = N->getState()->get<RequestMap>(RequestRegion);
+  assert(Req && "The region must be tracked and alive, given that we've "
+                "just emitted a report against it");
   const Request *const PrevReq =
       N->getFirstPred()->getState()->get<RequestMap>(RequestRegion);
 
   // Check if request was previously unused or in a different state.
-  if ((Req && !PrevReq) || (Req->CurrentState != PrevReq->CurrentState)) {
+  if (!PrevReq || (Req->CurrentState != PrevReq->CurrentState)) {
     IsNodeFound = true;
 
     ProgramPoint P = N->getFirstPred()->getLocation();

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

@@ -1132,14 +1132,13 @@ ProgramStateRef MallocChecker::addExtentSize(CheckerContext &C,
     // Store the extent size for the (symbolic)region
     // containing the elements.
     Region = Target.getAsRegion()
-                 ->getAs<SubRegion>()
+                 ->castAs<SubRegion>()
                  ->StripCasts()
-                 ->getAs<SubRegion>();
+                 ->castAs<SubRegion>();
   } else {
     ElementCount = svalBuilder.makeIntVal(1, true);
-    Region = Target.getAsRegion()->getAs<SubRegion>();
+    Region = Target.getAsRegion()->castAs<SubRegion>();
   }
-  assert(Region);
 
   // Set the region's extent equal to the Size in Bytes.
   QualType ElementType = NE->getAllocatedType();
@@ -3066,8 +3065,12 @@ PathDiagnosticPieceRef MallocChecker::MallocBugVisitor::VisitNode(
     }
   }
 
-  if (Msg.empty())
+  if (Msg.empty()) {
+    // Silence a memory leak warning by MallocChecker in MallocChecker.cpp :)
+    assert(!StackHint && "Memory leak!");
     return nullptr;
+  }
+
   assert(StackHint);
 
   // Generate the extra diagnostic.

lib/StaticAnalyzer/Checkers/MallocSizeofChecker.cpp

@@ -183,7 +183,7 @@ public:
       QualType CastedType = i->CastedExpr->getType();
       if (!CastedType->isPointerType())
         continue;
-      QualType PointeeType = CastedType->getAs<PointerType>()->getPointeeType();
+      QualType PointeeType = CastedType->getPointeeType();
       if (PointeeType->isVoidType())
         continue;
 

lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp

@@ -119,12 +119,12 @@ const MemRegion *PointerArithChecker::getArrayRegion(const MemRegion *Region,
                                                      AllocKind &AKind,
                                                      CheckerContext &C) const {
   assert(Region);
-  while (Region->getKind() == MemRegion::Kind::CXXBaseObjectRegionKind) {
-    Region = Region->getAs<CXXBaseObjectRegion>()->getSuperRegion();
+  while (const auto *BaseRegion = dyn_cast<CXXBaseObjectRegion>(Region)) {
+    Region = BaseRegion->getSuperRegion();
     Polymorphic = true;
   }
-  if (Region->getKind() == MemRegion::Kind::ElementRegionKind) {
-    Region = Region->getAs<ElementRegion>()->getSuperRegion();
+  if (const auto *ElemRegion = dyn_cast<ElementRegion>(Region)) {
+    Region = ElemRegion->getSuperRegion();
   }
 
   ProgramStateRef State = C.getState();
@@ -137,7 +137,7 @@ const MemRegion *PointerArithChecker::getArrayRegion(const MemRegion *Region,
   }
   // When the region is symbolic and we do not have any information about it,
   // assume that this is an array to avoid false positives.
-  if (Region->getKind() == MemRegion::Kind::SymbolicRegionKind)
+  if (isa<SymbolicRegion>(Region))
     return Region;
 
   // No AllocKind stored and not symbolic, assume that it points to a single

lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp

@@ -85,7 +85,7 @@ void UndefinedAssignmentChecker::checkBind(SVal location, SVal val,
     }
 
     if (const DeclStmt *DS = dyn_cast<DeclStmt>(StoreE)) {
-      const VarDecl *VD = dyn_cast<VarDecl>(DS->getSingleDecl());
+      const VarDecl *VD = cast<VarDecl>(DS->getSingleDecl());
       ex = VD->getInit();
     }
 

lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp

@@ -260,12 +260,13 @@ static llvm::Optional<DereferenceInfo> dereference(ProgramStateRef State,
       break;
   }
 
-  while (R->getAs<CXXBaseObjectRegion>()) {
+  while (isa<CXXBaseObjectRegion>(R)) {
     NeedsCastBack = true;
-
-    if (!isa<TypedValueRegion>(R->getSuperRegion()))
+    const auto *SuperR = dyn_cast<TypedValueRegion>(R->getSuperRegion());
+    if (!SuperR)
       break;
-    R = R->getSuperRegion()->getAs<TypedValueRegion>();
+
+    R = SuperR;
   }
 
   return DereferenceInfo{R, NeedsCastBack, /*IsCyclic*/ false};

lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp

@@ -104,7 +104,8 @@ void VirtualCallChecker::checkPreCall(const CallEvent &Call,
     return;
 
   ProgramStateRef State = C.getState();
-  const CallExpr *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
+  // Member calls are always represented by a call-expression.
+  const auto *CE = cast<CallExpr>(Call.getOriginExpr());
   if (!isVirtualCall(CE))
     return;
 

lib/StaticAnalyzer/Core/CheckerHelpers.cpp

@@ -91,7 +91,7 @@ parseAssignment(const Stmt *S) {
   } else if (auto PD = dyn_cast_or_null<DeclStmt>(S)) {
     // Initialization
     assert(PD->isSingleDecl() && "We process decls one by one");
-    VD = dyn_cast_or_null<VarDecl>(PD->getSingleDecl());
+    VD = cast<VarDecl>(PD->getSingleDecl());
     RHS = VD->getAnyInitializer();
   }
 

lib/StaticAnalyzer/Core/ExprEngineC.cpp

@@ -850,8 +850,7 @@ VisitOffsetOfExpr(const OffsetOfExpr *OOE,
   if (OOE->EvaluateAsInt(Result, getContext())) {
     APSInt IV = Result.Val.getInt();
     assert(IV.getBitWidth() == getContext().getTypeSize(OOE->getType()));
-    assert(OOE->getType()->isBuiltinType());
-    assert(OOE->getType()->getAs<BuiltinType>()->isInteger());
+    assert(OOE->getType()->castAs<BuiltinType>()->isInteger());
     assert(IV.isSigned() == OOE->getType()->isSignedIntegerType());
     SVal X = svalBuilder.makeIntVal(IV);
     B.generateNode(OOE, Pred,

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

@@ -803,9 +803,8 @@ void ExprEngine::VisitCXXNewExpr(const CXXNewExpr *CNE, ExplodedNode *Pred,
   if (CNE->isArray()) {
     // FIXME: allocating an array requires simulating the constructors.
     // For now, just return a symbolicated region.
-    if (const SubRegion *NewReg =
-            dyn_cast_or_null<SubRegion>(symVal.getAsRegion())) {
-      QualType ObjTy = CNE->getType()->getAs<PointerType>()->getPointeeType();
+    if (const auto *NewReg = cast_or_null<SubRegion>(symVal.getAsRegion())) {
+      QualType ObjTy = CNE->getType()->getPointeeType();
       const ElementRegion *EleReg =
           getStoreManager().GetElementZeroRegion(NewReg, ObjTy);
       Result = loc::MemRegionVal(EleReg);

lib/StaticAnalyzer/Core/MemRegion.cpp

@@ -1075,7 +1075,7 @@ MemRegionManager::getCXXBaseObjectRegion(const CXXRecordDecl *RD,
                                          const SubRegion *Super,
                                          bool IsVirtual) {
   if (isa<TypedValueRegion>(Super)) {
-    assert(isValidBaseClass(RD, dyn_cast<TypedValueRegion>(Super), IsVirtual));
+    assert(isValidBaseClass(RD, cast<TypedValueRegion>(Super), IsVirtual));
     (void)&isValidBaseClass;
 
     if (IsVirtual) {
@@ -1426,6 +1426,7 @@ static RegionOffset calculateOffset(const MemRegion *R) {
     case MemRegion::FieldRegionKind: {
       const auto *FR = cast<FieldRegion>(R);
       R = FR->getSuperRegion();
+      assert(R);
 
       const RecordDecl *RD = FR->getDecl()->getParent();
       if (RD->isUnion() || !RD->isCompleteDefinition()) {

lib/StaticAnalyzer/Core/RegionStore.cpp

@@ -2291,8 +2291,7 @@ RegionBindingsRef RegionStoreManager::bindVector(RegionBindingsConstRef B,
                                                  const TypedValueRegion* R,
                                                  SVal V) {
   QualType T = R->getValueType();
-  assert(T->isVectorType());
-  const VectorType *VT = T->getAs<VectorType>(); // Use getAs for typedefs.
+  const VectorType *VT = T->castAs<VectorType>(); // Use castAs for typedefs.
 
   // Handle lazy compound values and symbolic values.
   if (V.getAs<nonloc::LazyCompoundVal>() || V.getAs<nonloc::SymbolVal>())
@@ -2377,7 +2376,7 @@ RegionBindingsRef RegionStoreManager::bindStruct(RegionBindingsConstRef B,
   QualType T = R->getValueType();
   assert(T->isStructureOrClassType());
 
-  const RecordType* RT = T->getAs<RecordType>();
+  const RecordType* RT = T->castAs<RecordType>();
   const RecordDecl *RD = RT->getDecl();
 
   if (!RD->isCompleteDefinition())

test/Analysis/cstring-syntax-weird.c

@@ -0,0 +1,19 @@
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=unix.cstring.BadSizeArg \
+// RUN:                    -verify %s
+
+// expected-no-diagnostics
+
+typedef __SIZE_TYPE__ size_t;
+// The last parameter is normally size_t but the test is about the abnormal
+// situation when it's not a size_t.
+size_t strlcpy(char *, const char *, int);
+
+enum WeirdDecl {
+  AStrangeWayToSpecifyStringLengthCorrectly = 10UL,
+  AStrangeWayToSpecifyStringLengthIncorrectly = 5UL
+};
+void testWeirdDecls(const char *src) {
+  char dst[10];
+  strlcpy(dst, src, AStrangeWayToSpecifyStringLengthCorrectly); // no-crash
+  strlcpy(dst, src, AStrangeWayToSpecifyStringLengthIncorrectly); // no-crash // no-warning
+}

test/Analysis/cstring-syntax-weird2.c

@@ -0,0 +1,17 @@
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=unix.cstring.BadSizeArg \
+// RUN:                    -verify %s
+
+// expected-no-diagnostics
+
+typedef __SIZE_TYPE__ size_t;
+// The last parameter is normally size_t but the test is about the abnormal
+// situation when it's not a size_t.
+size_t strlcpy(char *, const char *, void (*)());
+
+void foo();
+
+void testWeirdDecls(const char *src) {
+  char dst[10];
+  strlcpy(dst, src, foo); // no-crash
+  strlcpy(dst, src, &foo); // no-crash
+}

test/Analysis/cstring-syntax.c

@@ -1,7 +1,18 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=unix.cstring.BadSizeArg -analyzer-store=region -Wno-strncat-size -Wno-strlcpy-strlcat-size -Wno-sizeof-array-argument -Wno-sizeof-pointer-memaccess -verify %s
-// RUN: %clang_analyze_cc1 -triple armv7-a15-linux -analyzer-checker=unix.cstring.BadSizeArg -analyzer-store=region -Wno-strncat-size -Wno-strlcpy-strlcat-size -Wno-sizeof-array-argument -Wno-sizeof-pointer-memaccess -verify %s
-// RUN: %clang_analyze_cc1 -triple aarch64_be-none-linux-gnu -analyzer-checker=unix.cstring.BadSizeArg -analyzer-store=region -Wno-strncat-size -Wno-strlcpy-strlcat-size -Wno-sizeof-array-argument -Wno-sizeof-pointer-memaccess -verify %s
-// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 -analyzer-checker=unix.cstring.BadSizeArg -analyzer-store=region -Wno-strncat-size -Wno-strlcpy-strlcat-size -Wno-sizeof-array-argument -Wno-sizeof-pointer-memaccess -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=unix.cstring.BadSizeArg -verify %s\
+// RUN:                    -Wno-strncat-size -Wno-sizeof-pointer-memaccess     \
+// RUN:                    -Wno-strlcpy-strlcat-size -Wno-sizeof-array-argument
+// RUN: %clang_analyze_cc1 -analyzer-checker=unix.cstring.BadSizeArg -verify %s\
+// RUN:                    -Wno-strncat-size -Wno-sizeof-pointer-memaccess     \
+// RUN:                    -Wno-strlcpy-strlcat-size -Wno-sizeof-array-argument\
+// RUN:                    -triple armv7-a15-linux
+// RUN: %clang_analyze_cc1 -analyzer-checker=unix.cstring.BadSizeArg -verify %s\
+// RUN:                    -Wno-strncat-size -Wno-sizeof-pointer-memaccess     \
+// RUN:                    -Wno-strlcpy-strlcat-size -Wno-sizeof-array-argument\
+// RUN:                    -triple aarch64_be-none-linux-gnu
+// RUN: %clang_analyze_cc1 -analyzer-checker=unix.cstring.BadSizeArg -verify %s\
+// RUN:                    -Wno-strncat-size -Wno-sizeof-pointer-memaccess     \
+// RUN:                    -Wno-strlcpy-strlcat-size -Wno-sizeof-array-argument\
+// RUN:                    -triple i386-apple-darwin10
 
 typedef __SIZE_TYPE__ size_t;
 char  *strncat(char *, const char *, size_t);