[CLANG]Fix potential null pointer dereference bugs

[Manna]

Summary

This patch uses castAs instead of getAs which will assert if the type doesn't match and adds nullptr check if needed.

[Manna]

Fix build error

[Manna]

Update patch

[aaronpuchert]

clang/lib/Analysis/ThreadSafety.cpp

504–506

Can't we just take I.getData() instead of doing a lookup? After all we're iterating over the same map.

[Manna]

Address review comment and update patch

[Manna]

clang/lib/Analysis/ThreadSafety.cpp

504–506

Thank you @aaronpuchert for review and comment.

Can't we just take I.getData() instead of doing a lookup? After all we're iterating over the same map.

Makes sense to me! I have updated patch

[aaron.ballman]

clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp

3368 ↗

(On Diff #531976)

This is unnecessary, getCanonicalDecl() can never return a null pointer (it will return this, at the very least)

[Manna]

Address review comment.

[Manna]

clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp

3368 ↗

(On Diff #531976)

Thank you @aaron.ballman for reviews! I agree with you. My previous analysis was wrong. I have removed this change.

[Manna]

ping @aaron.ballman

[aaron.ballman]

LGTM

[Manna]

Thank you for reviews @aaron.ballman!

clang/lib/AST/ASTContext.cpp

static bool sameObjCTypeArgs(ASTContext &ctx,
                              const ObjCInterfaceDecl *iface,
                              ArrayRef<QualType> lhsArgs,
                              ArrayRef<QualType> rhsArgs,
                              bool stripKindOf) {
   if (lhsArgs.size() != rhsArgs.size())
     return false;
 
   ObjCTypeParamList *typeParams = iface->getTypeParamList();
+  if (!typeParams)
+    return false;
+
   for (unsigned i = 0, n = lhsArgs.size(); i != n; ++i) {
     if (ctx.hasSameType(lhsArgs[i], rhsArgs[i]))
       continue;
 
     switch (typeParams->begin()[i]->getVariance()) {
     case ObjCTypeParamVariance::Invariant:
       if (!stripKindOf ||
           !ctx.hasSameType(lhsArgs[i].stripObjCKindOfType(ctx),

clang/lib/AST/MicrosoftMangle.cpp

if (IsCtorClosure) {
 
       if (StructorType == Ctor_DefaultClosure) {
         // Default constructor closure always has no arguments.
         Out << 'X';
       } else if (StructorType == Ctor_CopyingClosure) {
         // Copy constructor closure always takes an unqualified reference.
         mangleFunctionArgumentType(getASTContext().getLValueReferenceType(
                                        Proto->getParamType(0)
-                                           ->getAs<LValueReferenceType>()
+                                           ->castAs<LValueReferenceType>()
                                            ->getPointeeType(),
                                        /*SpelledAsLValue=*/true),
                                    Range);
         Out << '@';
       } else {
         llvm_unreachable("unexpected constructor closure!");
       }
       Out << 'Z';

clang/lib/Analysis/ThreadSafety.cpp

for (unsigned i = 1, e = VarDefinitions.size(); i < e; ++i) {
       }
     }
   }
 
   /// Dumps an ASCII representation of a Context to llvm::errs()
   void dumpContext(Context C) {
     for (Context::iterator I = C.begin(), E = C.end(); I != E; ++I) {
       const NamedDecl *D = I.getKey();
       D->printName(llvm::errs());
-      const unsigned *i = C.lookup(D);
       llvm::errs() << " -> ";
-      dumpVarDefinitionName(*i);
+      dumpVarDefinitionName(I.getData());
       llvm::errs() << "\n";
     }
   }
 
   /// Builds the variable map.
   void traverseCFG(CFG *CFGraph, const PostOrderCFGView *SortedGraph,
                    std::vector<CFGBlockInfo> &BlockInfo);
 

clang/lib/Sema/SemaExpr.cpp

if (isa<BinaryOperator>(E) && cast<BinaryOperator>(E)->isCommaOp()) {
       Diag(E->getExprLoc(), diag::err_matrix_subscript_comma)
           << SourceRange(base->getBeginLoc(), rbLoc);
       return true;
     }
     return false;
   };
   // The matrix subscript operator ([][])is considered a single operator.
   // Separating the index expressions by parenthesis is not allowed.
-  if (base->hasPlaceholderType(BuiltinType::IncompleteMatrixIdx) &&
+  if (base && !base->getType().isNull() &&
+      base->hasPlaceholderType(BuiltinType::IncompleteMatrixIdx) &&
       !isa<MatrixSubscriptExpr>(base)) {
     Diag(base->getExprLoc(), diag::err_matrix_separate_incomplete_index)
         << SourceRange(base->getBeginLoc(), rbLoc);
     return ExprError();
   }
   // If the base is a MatrixSubscriptExpr, try to create a new
   // MatrixSubscriptExpr.
   auto *matSubscriptE = dyn_cast<MatrixSubscriptExpr>(base);

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

for (SymbolRef LeakSym : LeakedSyms) {
     //      return; // warning
     //  } // warning
     // While this isn't necessarily true (leaking the same stream could result
     // from a different kinds of errors), the reduction in redundant reports
     // makes this a worthwhile heuristic.
     // FIXME: Add a checker option to turn this uniqueing feature off.
     const ExplodedNode *StreamOpenNode = getAcquisitionSite(Err, LeakSym, C);
     assert(StreamOpenNode && "Could not find place of stream opening.");
-    PathDiagnosticLocation LocUsedForUniqueing =
-        PathDiagnosticLocation::createBegin(
-            StreamOpenNode->getStmtForDiagnostics(), C.getSourceManager(),
-            StreamOpenNode->getLocationContext());
+
+    PathDiagnosticLocation LocUsedForUniqueing;
+    if (const Stmt *StreamStmt = StreamOpenNode->getStmtForDiagnostics())
+       LocUsedForUniqueing = PathDiagnosticLocation::createBegin(
+          StreamStmt, C.getSourceManager(),
 
     std::unique_ptr<PathSensitiveBugReport> R =
         std::make_unique<PathSensitiveBugReport>(
             BT_ResourceLeak,
             "Opened stream never closed. Potential resource leak.", Err,
             LocUsedForUniqueing,
             StreamOpenNode->getLocationContext()->getDecl());
     R->markInteresting(LeakSym);