[Analysis] Fix null pointer dereference warnings [1/n]

[mgrang]

Summary

Running the PREfast static analysis tool on clang resulted in several null
pointer dereference warnings. This is the first of several patches to fix
these.

The full list of warnings reported is here: https://docs.google.com/spreadsheets/d/1h_3tHxsgBampxb7PXoB5lgwiBSpTty9RLe5maIQxnTk/edit#gid=2014408543

[efriedma]

Please don't add null checks for pointers that can't be null. It makes the code slower and harder to understand. And least one of the checks you added is actively breaking the code.

In some cases, the analysis is pointing to cases where the code could be made more clear for both humans and machines with some refactoring or assertions. Patches welcome, but please make sure any assertions properly explain the invariant. And please split the patches up a bit more; adding assertions for complex invariants in ten different unrelated places is more than I really want to review at once.

(Also, a reminder, please post patches with full context.)

[mgrang]

In D78853#2003866, @efriedma wrote:

Please don't add null checks for pointers that can't be null. It makes the code slower and harder to understand. And least one of the checks you added is actively breaking the code.

In some cases, the analysis is pointing to cases where the code could be made more clear for both humans and machines with some refactoring or assertions. Patches welcome, but please make sure any assertions properly explain the invariant. And please split the patches up a bit more; adding assertions for complex invariants in ten different unrelated places is more than I really want to review at once.

(Also, a reminder, please post patches with full context.)

Thanks @efriedma. Will split up the patches into smaller chunks and also post with full context.

[dblaikie]

clang/lib/Analysis/ThreadSafety.cpp

1938–1940 ↗

(On Diff #260517)

Doesn't seem like the most informative comment or assertion string - the invariant "isScopedVar implies VD is non-null" is established earlier in the function, where isScopedVar only becomes true under the "VD is non-null" condition at 1809.

Would it be better to improve whatever static analysis you're using to be able to track that correlation, rather than adding lots of extra assertions to LLVM? (can the Clang Static Analyzer understand this code and avoid warning on it, for instance - that'd be a good existence proof for such "smarts" being reasonably possible for static analysis)

[aaronpuchert]

Could you explain why we are doing this? Clang has its own static analyzer, which can find null dereferences as well but also tracks constraints to prevent false positives like this.

You can find pretty recent results of Clang static analyzer runs here: http://llvm.org/reports/scan-build/.

clang/lib/Analysis/ThreadSafety.cpp

1938–1940 ↗

(On Diff #260517)

I haven't tested it, but the Clang static analyzer shouldn't complain here. It explores different code paths and collects constraints along them. All code paths including the assignment isScopedVar = true should have VD being non-null as constraint, because that's the condition to get there.

I know that solving constraints can be hard in general, but here the constraint is exactly what we need to disprove null references from happening.

clang/lib/Sema/SemaDecl.cpp

       (!IsInline || (IsMicrosoft && IsTemplate)) && !IsStaticDataMember &&
       !NewDecl->isLocalExternDecl() && !IsQualifiedFriend) {
     if (IsMicrosoft && IsDefinition) {
       S.Diag(NewDecl->getLocation(),
              diag::warn_redeclaration_without_import_attribute)
           << NewDecl;
       S.Diag(OldDecl->getLocation(), diag::note_previous_declaration);
       NewDecl->dropAttr<DLLImportAttr>();
+
+      assert(NewImportAttr && "!NewImportAttr");
       NewDecl->addAttr(
           DLLExportAttr::CreateImplicit(S.Context, NewImportAttr->getRange()));
     } else {
       S.Diag(NewDecl->getLocation(),
              diag::warn_redeclaration_without_attribute_prev_attribute_ignored)
           << NewDecl << OldImportAttr;
       S.Diag(OldDecl->getLocation(), diag::note_previous_declaration);
       S.Diag(OldImportAttr->getLocation(), diag::note_previous_attribute);

     NewFD->setInvalidDecl();
     return true;
   }
 
   MultiVersionKind  MVType = NewFD->getMultiVersionKind();
 
   // Main isn't allowed to become a multiversion function, however it IS
   // permitted to have 'main' be marked with the 'target' optimization hint.
-  if (NewFD->isMain()) {
+  if (NewTA && NewFD->isMain()) {
     if ((MVType == MultiVersionKind::Target && NewTA->isDefaultVersion()) ||
         MVType == MultiVersionKind::CPUDispatch ||
         MVType == MultiVersionKind::CPUSpecific) {
       S.Diag(NewFD->getLocation(), diag::err_multiversion_not_allowed_on_main);
       NewFD->setInvalidDecl();
       return true;
     }
     return false;

         break;
       DeducedType *DT = D->getType()->getContainedDeducedType();
       if (!DT || DT->getDeducedType().isNull())
         continue;
       if (Deduced.isNull()) {
         Deduced = DT->getDeducedType();
         DeducedDecl = D;
       } else if (!Context.hasSameType(DT->getDeducedType(), Deduced)) {
+        assert(DeducedDecl && "!DeducedDecl");
+
         auto *AT = dyn_cast<AutoType>(DT);
         Diag(D->getTypeSourceInfo()->getTypeLoc().getBeginLoc(),
              diag::err_auto_different_deductions)
           << (AT ? (unsigned)AT->getKeyword() : 3)
           << Deduced << DeducedDecl->getDeclName()
           << DT->getDeducedType() << D->getDeclName()
           << DeducedDecl->getInit()->getSourceRange()
           << D->getInit()->getSourceRange();

clang/lib/Sema/SemaDeclCXX.cpp

       Diag(Old->getLocation(), diag::note_previous_declaration);
     }
   }
 
   const FunctionDecl *Def;
   // C++11 [dcl.constexpr]p1: If any declaration of a function or function
   // template has a constexpr specifier then all its declarations shall
   // contain the constexpr specifier.
+  assert(Old && "!Old");
   if (New->getConstexprKind() != Old->getConstexprKind()) {
     Diag(New->getLocation(), diag::err_constexpr_redecl_mismatch)
         << New << New->getConstexprKind() << Old->getConstexprKind();
     Diag(Old->getLocation(), diag::note_previous_declaration);
     Invalid = true;
   } else if (!Old->getMostRecentDecl()->isInlined() && New->isInlined() &&
              Old->isDefined(Def) &&
              // If a friend function is inlined but does not have 'inline'

           << false << RD << BestPath->back().Base->getType()
           << P.back().Base->getType();
         return DeclAccessPair();
       } else if (P.Access < BestPath->Access) {
         BestPath = &P;
       }
     }
 
+    assert(BestPath && "!BestPath");
+
     //   ... unambiguous ...
     QualType BaseType = BestPath->back().Base->getType();
     if (Paths.isAmbiguous(S.Context.getCanonicalType(BaseType))) {
       S.Diag(Loc, diag::err_decomp_decl_ambiguous_base)
         << RD << BaseType << S.getAmbiguousPathsDisplayString(Paths);
       return DeclAccessPair();
     }
 

       // Note this base's direct & indirect bases, if there could be ambiguity.
       if (Bases.size() > 1)
         NoteIndirectBases(Context, IndirectBaseTypes, NewBaseType);
 
       if (const RecordType *Record = NewBaseType->getAs<RecordType>()) {
         const CXXRecordDecl *RD = cast<CXXRecordDecl>(Record->getDecl());
         if (Class->isInterface() &&
               (!RD->isInterfaceLike() ||
-               KnownBase->getAccessSpecifier() != AS_public)) {
+               (KnownBase && KnownBase->getAccessSpecifier() != AS_public))) {
           // The Microsoft extension __interface does not permit bases that
           // are not themselves public interfaces.
           Diag(KnownBase->getBeginLoc(), diag::err_invalid_base_in_interface)
               << getRecordDiagFromTagKind(RD->getTagKind()) << RD
               << RD->getSourceRange();
           Invalid = true;
         }
         if (RD->hasAttr<WeakAttr>())

clang/lib/Sema/SemaExpr.cpp

   }
 
   // Handle any non-overload placeholder types in the base and index
   // expressions.  We can't handle overloads here because the other
   // operand might be an overloadable type, in which case the overload
   // resolution for the operator overload should get the first crack
   // at the overload.
   bool IsMSPropertySubscript = false;
-  if (base->getType()->isNonOverloadPlaceholderType()) {
+  if (base && base->getType()->isNonOverloadPlaceholderType()) {
     IsMSPropertySubscript = isMSPropertySubscriptExpr(*this, base);
     if (!IsMSPropertySubscript) {
       ExprResult result = CheckPlaceholderExpr(base);
       if (result.isInvalid())
         return ExprError();
       base = result.get();
     }
   }

clang/lib/Sema/SemaExprCXX.cpp

     Qualifiers Quals;
     QualType UnqualT = Context.getUnqualifiedArrayType(T, Quals);
     if (!Context.hasSameType(T, UnqualT)) {
       T = UnqualT;
       E = ImpCastExprToType(E, UnqualT, CK_NoOp, E->getValueKind()).get();
     }
   }
 
-  if (E->getType()->isVariablyModifiedType())
+  if (E && E->getType()->isVariablyModifiedType())
     return ExprError(Diag(TypeidLoc, diag::err_variably_modified_typeid)
                      << E->getType());
   else if (!inTemplateInstantiation() &&
-           E->HasSideEffects(Context, WasEvaluated)) {
+           E && E->HasSideEffects(Context, WasEvaluated)) {
     // The expression operand for typeid is in an unevaluated expression
     // context, so side effects could result in unintended consequences.
     Diag(E->getExprLoc(), WasEvaluated
                               ? diag::warn_side_effects_typeid
                               : diag::warn_side_effects_unevaluated_context);
   }
 
   return new (Context) CXXTypeidExpr(TypeInfoType.withConst(), E,

clang/lib/Sema/SemaExprObjC.cpp

         if (!validateBoxingMethod(*this, Loc, NSStringDecl,
                                   stringWithUTF8String, BoxingMethod))
            return ExprError();
 
         StringWithUTF8StringMethod = BoxingMethod;
       }
 
       BoxingMethod = StringWithUTF8StringMethod;
+      assert(BoxingMethod && "!BoxingMethod");
+
       BoxedType = NSStringPointer;
       // Transfer the nullability from method's return type.
       Optional<NullabilityKind> Nullability =
           BoxingMethod->getReturnType()->getNullability(Context);
       if (Nullability)
         BoxedType = Context.getAttributedType(
             AttributedType::getNullabilityAttrKind(*Nullability), BoxedType,
             BoxedType);

clang/lib/Sema/SemaInit.cpp

       AddConversionSequenceStep(LvalueICS, ICS.Standard.getToType(0));
     }
 
     AddPassByIndirectCopyRestoreStep(DestType, ShouldCopy);
   } else if (ICS.isBad()) {
     DeclAccessPair dap;
     if (isLibstdcxxPointerReturnFalseHack(S, Entity, Initializer)) {
       AddZeroInitializationStep(Entity.getType());
-    } else if (Initializer->getType() == Context.OverloadTy &&
+    } else if (Initializer && Initializer->getType() == Context.OverloadTy &&
                !S.ResolveAddressOfOverloadedFunction(Initializer, DestType,
                                                      false, dap))
       SetFailed(InitializationSequence::FK_AddressOfOverloadFailed);
-    else if (Initializer->getType()->isFunctionType() &&
+    else if (Initializer && Initializer->getType()->isFunctionType() &&
              isExprAnUnaddressableFunction(S, Initializer))
       SetFailed(InitializationSequence::FK_AddressOfUnaddressableFunction);
     else
       SetFailed(InitializationSequence::FK_ConversionFailed);
   } else {
     AddConversionSequenceStep(ICS, DestType, TopLevelOfInitList);
 
     MaybeProduceObjCObject(S, *this, Entity);

clang/lib/Sema/SemaOverload.cpp

               FromIface->isSuperClassOf(ToIface))
             BaseToDerivedConversion = 2;
   } else if (const ReferenceType *ToRefTy = ToTy->getAs<ReferenceType>()) {
     if (ToRefTy->getPointeeType().isAtLeastAsQualifiedAs(FromTy) &&
         !FromTy->isIncompleteType() &&
         !ToRefTy->getPointeeType()->isIncompleteType() &&
         S.IsDerivedFrom(SourceLocation(), ToRefTy->getPointeeType(), FromTy)) {
       BaseToDerivedConversion = 3;
-    } else if (ToTy->isLValueReferenceType() && !FromExpr->isLValue() &&
+    } else if (ToTy->isLValueReferenceType() && FromExpr &&
+               !FromExpr->isLValue() &&
                ToTy.getNonReferenceType().getCanonicalType() ==
                FromTy.getNonReferenceType().getCanonicalType()) {
       S.Diag(Fn->getLocation(), diag::note_ovl_candidate_bad_lvalue)
           << (unsigned)FnKindPair.first << (unsigned)FnKindPair.second << FnDesc
           << (unsigned)isObjectArgument << I + 1
           << (FromExpr ? FromExpr->getSourceRange() : SourceRange());
       MaybeEmitInheritedConstructorNote(S, Cand->FoundDecl);
       return;

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

   // Enter the scope of this instantiation. We don't use
   // PushDeclContext because we don't have a scope.
   ContextRAII PreviousContext(*this, Var->getDeclContext());
   LocalInstantiationScope Local(*this);
 
   LocalEagerInstantiationScope LocalInstantiations(*this);
 
   VarDecl *OldVar = Var;
-  if (Def->isStaticDataMember() && !Def->isOutOfLine()) {
+  if (Def && Def->isStaticDataMember() && !Def->isOutOfLine()) {
     // We're instantiating an inline static data member whose definition was
     // provided inside the class.
     InstantiateVariableInitializer(Var, Def, TemplateArgs);
   } else if (!VarSpec) {
     Var = cast_or_null<VarDecl>(SubstDecl(Def, Var->getDeclContext(),
                                           TemplateArgs));
   } else if (Var->isStaticDataMember() &&
              Var->getLexicalDeclContext()->isRecord()) {

clang/lib/Sema/SemaTemplateVariadic.cpp

   //   template<typename ...T> struct A {
   //     template<typename ...U> void f(pair<T, U>...);
   //   };
   //
   // ... a call to 'A<int, int>().f<int>' should expand the pack once and
   // retain an expansion.
   if (NumPartialExpansions) {
     if (NumExpansions && *NumExpansions < *NumPartialExpansions) {
+      assert(CurrentInstantiationScope && "!CurrentInstantiationScope");
+
       NamedDecl *PartialPack =
           CurrentInstantiationScope->getPartiallySubstitutedPack();
       Diag(EllipsisLoc, diag::err_pack_expansion_length_conflict_partial)
         << PartialPack << *NumPartialExpansions << *NumExpansions
         << SourceRange(PartiallySubstitutedPackLoc);
       return true;
     }
 

clang/lib/Sema/SemaType.cpp

   ObjCInterfaceDecl *IFace = dyn_cast_or_null<ObjCInterfaceDecl>(Def);
 
   // Give the external source a chance to provide a definition of the type.
   // This is kept separate from completing the redeclaration chain so that
   // external sources such as LLDB can avoid synthesizing a type definition
   // unless it's actually needed.
   if (Tag || IFace) {
     // Avoid diagnosing invalid decls as incomplete.
-    if (Def->isInvalidDecl())
+    if (Def && Def->isInvalidDecl())
       return true;
 
     // Give the external AST source a chance to complete the type.
     if (auto *Source = Context.getExternalSource()) {
       if (Tag && Tag->hasExternalLexicalStorage())
           Source->CompleteType(Tag);
       if (IFace && IFace->hasExternalLexicalStorage())
           Source->CompleteType(IFace);

clang/lib/Sema/TreeTransform.h

   if (Second && Second->getObjectKind() == OK_ObjCProperty) {
     ExprResult Result = SemaRef.CheckPlaceholderExpr(Second);
     if (Result.isInvalid())
       return ExprError();
     Second = Result.get();
   }
 
   // Determine whether this should be a builtin operation.
-  if (Op == OO_Subscript) {
+  if (Op == OO_Subscript && Second) {
     if (!First->getType()->isOverloadableType() &&
         !Second->getType()->isOverloadableType())
       return getSema().CreateBuiltinArraySubscriptExpr(
           First, Callee->getBeginLoc(), Second, OpLoc);
   } else if (Op == OO_Arrow) {
     // -> is never a builtin operation.
     return SemaRef.BuildOverloadedArrowExpr(nullptr, First, OpLoc);
   } else if (Second == nullptr || isPostIncDec) {