Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp =================================================================== --- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -268,12 +268,19 @@ /// but not written inside, and it has caused an undefined read or a null /// pointer dereference outside. class NoStoreFuncVisitor final : public BugReporterVisitor { + const SubRegion *RegionOfInterest; const SourceManager &SM; const PrintingPolicy &PP; + MemRegionManager &MmrMgr; + static constexpr const char *DiagnosticsMsg = "Returning without writing to '"; + /// Recursion limit for dereferencing fields when looking for the + /// region of interest. + static const unsigned DEREFERENCE_LIMIT = 3; + /// Frames writing into \c RegionOfInterest. /// This visitor generates a note only if a function does not write into /// a region of interest. This information is not immediately available @@ -285,10 +292,14 @@ llvm::SmallPtrSet FramesModifyingRegion; llvm::SmallPtrSet FramesModifyingCalculated; + using FieldVector = SmallVector; + public: NoStoreFuncVisitor(const SubRegion *R, const ASTContext &Ctx, - SourceManager &SM) - : RegionOfInterest(R), SM(SM), PP(Ctx.getPrintingPolicy()) {} + SourceManager &SM, + MemRegionManager &MmrMgr) + : RegionOfInterest(R), SM(SM), PP(Ctx.getPrintingPolicy()), + MmrMgr(MmrMgr) {} void Profile(llvm::FoldingSetNodeID &ID) const override { static int Tag = 0; @@ -306,28 +317,36 @@ auto CallExitLoc = N->getLocationAs(); // No diagnostic if region was modified inside the frame. - if (!CallExitLoc) + if (!CallExitLoc || isRegionOfInterestModifiedInFrame(N)) return nullptr; CallEventRef<> Call = BRC.getStateManager().getCallEventManager().getCaller(SCtx, State); + if (SM.isInSystemHeader(Call->getDecl()->getSourceRange().getBegin())) { + + // TODO: test coverage + return nullptr; + } + // Region of interest corresponds to an IVar, exiting a method // which could have written into that IVar, but did not. if (const auto *MC = dyn_cast(Call)) if (const auto *IvarR = dyn_cast(RegionOfInterest)) if (potentiallyWritesIntoIvar(Call->getRuntimeDefinition().getDecl(), - IvarR->getDecl()) && - !isRegionOfInterestModifiedInFrame(N)) + IvarR->getDecl())) return notModifiedMemberDiagnostics( Ctx, *CallExitLoc, Call, MC->getReceiverSVal().getAsRegion()); if (const auto *CCall = dyn_cast(Call)) { const MemRegion *ThisR = CCall->getCXXThisVal().getAsRegion(); if (RegionOfInterest->isSubRegionOf(ThisR) - && !CCall->getDecl()->isImplicit() - && !isRegionOfInterestModifiedInFrame(N)) + && !CCall->getDecl()->isImplicit()) return notModifiedMemberDiagnostics(Ctx, *CallExitLoc, Call, ThisR); + + // Do not generate diagnostics for not modified parameters in + // constructors. + return nullptr; } ArrayRef parameters = getCallParameters(Call); @@ -337,27 +356,85 @@ unsigned IndirectionLevel = 1; QualType T = PVD->getType(); while (const MemRegion *R = S.getAsRegion()) { - if (RegionOfInterest->isSubRegionOf(R) - && !isPointerToConst(PVD->getType())) { - - if (isRegionOfInterestModifiedInFrame(N)) - return nullptr; + if (RegionOfInterest->isSubRegionOf(R) && !isPointerToConst(T)) return notModifiedParameterDiagnostics( Ctx, *CallExitLoc, Call, PVD, R, IndirectionLevel); - } + QualType PT = T->getPointeeType(); + if (PT.isNull() || PT->isVoidType()) break; + + if (const CXXRecordDecl *RD = PT->getAsCXXRecordDecl()) + if (auto P = findRegionOfInterestInRecord(RD, State, R)) + return notModifiedParameterDiagnostics(Ctx, *CallExitLoc, Call, PVD, + RegionOfInterest, + IndirectionLevel, *P); + S = State->getSVal(R, PT); T = PT; + IndirectionLevel++; } + } return nullptr; } private: + /// Attempts to find the region of interest in a given CXX decl, + /// by either following the base classes or fields. + /// Dereferences fields up to a given recursion limit. + /// \return A chain fields leading to the region of interest or None. + const Optional + findRegionOfInterestInRecord(const CXXRecordDecl *RD, ProgramStateRef State, + const MemRegion *R, FieldVector Vec = {}, + int depth = 0) { + + if (depth == DEREFERENCE_LIMIT) // Limit the recursion depth. + return None; + + if (!RD->hasDefinition()) + return None; + + for (const auto II : RD->bases()) { + if (const CXXRecordDecl *RRD = II.getType()->getAsCXXRecordDecl()) + if (auto Out = + findRegionOfInterestInRecord(RRD, State, R, Vec, depth + 1)) + return Out; + } + + for (const FieldDecl *I : RD->fields()) { + QualType FT = I->getType(); + const FieldRegion *FR = MmrMgr.getFieldRegion(I, cast(R)); + const SVal V = State->getSVal(FR); + const MemRegion *VR = V.getAsRegion(); + + FieldVector VecF = Vec; + VecF.push_back(FR); + + if (RegionOfInterest == VR) { + return VecF; + } + + if (const CXXRecordDecl *RRD = FT->getAsCXXRecordDecl()) + if (auto Out = + findRegionOfInterestInRecord(RRD, State, FR, VecF, depth + 1)) + return Out; + + QualType PT = FT->getPointeeType(); + if (PT.isNull() || PT->isVoidType() || !VR) continue; + + if (const CXXRecordDecl *RRD = PT->getAsCXXRecordDecl()) + if (auto Out = + findRegionOfInterestInRecord(RRD, State, VR, VecF, depth + 1)) + return Out; + + } + + return None; + } /// \return Whether the method declaration \p Parent /// syntactically has a binary operation writing into the ivar \p Ivar. @@ -454,7 +531,7 @@ llvm::raw_svector_ostream os(sbuf); os << DiagnosticsMsg; bool out = prettyPrintRegionName(TopRegionName, "->", /*IsReference=*/true, - /*IndirectionLevel=*/1, ArgRegion, os, PP); + /*IndirectionLevel=*/1, ArgRegion, os); // Return nothing if we have failed to pretty-print. if (!out) @@ -476,7 +553,8 @@ CallEventRef<> Call, const ParmVarDecl *PVD, const MemRegion *ArgRegion, - unsigned IndirectionLevel) { + unsigned IndirectionLevel=1, + FieldVector UsedFields={}) { PathDiagnosticLocation L = getPathDiagnosticLocation( CallExitLoc.getReturnStmt(), SM, Ctx, Call); SmallString<256> sbuf; @@ -484,13 +562,14 @@ os << DiagnosticsMsg; bool IsReference = PVD->getType()->isReferenceType(); const char *Sep = IsReference && IndirectionLevel == 1 ? "." : "->"; - bool Success = prettyPrintRegionName( - PVD->getQualifiedNameAsString().c_str(), - Sep, IsReference, IndirectionLevel, ArgRegion, os, PP); + bool Success = + prettyPrintRegionName(PVD->getQualifiedNameAsString(), Sep, IsReference, + IndirectionLevel, ArgRegion, os, UsedFields); // Print the parameter name if the pretty-printing has failed. if (!Success) PVD->printQualifiedName(os); + os << "'"; return std::make_shared(L, os.str()); } @@ -515,7 +594,7 @@ int IndirectionLevel, const MemRegion *ArgRegion, llvm::raw_svector_ostream &os, - const PrintingPolicy &PP) { + FieldVector Vec={}) { SmallVector Subregions; const MemRegion *R = RegionOfInterest; while (R != ArgRegion) { @@ -558,6 +637,11 @@ llvm_unreachable("Previous check has missed an unexpected region"); } } + + for (const FieldRegion *FR : Vec) { + os << "."; // FIXME: differentiate between '.' and '->' + FR->getDecl()->getDeclName().print(os, PP); + } return true; } }; @@ -1594,14 +1678,16 @@ LVNode->getSVal(Inner).getAsRegion(); if (R) { + ProgramStateRef S = N->getState(); // Mark both the variable region and its contents as interesting. SVal V = LVState->getRawSVal(loc::MemRegionVal(R)); report.addVisitor( llvm::make_unique( cast(R), - N->getState()->getStateManager().getContext(), - N->getState()->getAnalysisManager().getSourceManager())); + S->getStateManager().getContext(), + S->getAnalysisManager().getSourceManager(), + S->getStateManager().getRegionManager())); MacroNullReturnSuppressionVisitor::addMacroVisitorIfNecessary( N, R, EnableNullFPSuppression, report, V); Index: clang/test/Analysis/diagnostics/no-store-func-path-notes.cpp =================================================================== --- clang/test/Analysis/diagnostics/no-store-func-path-notes.cpp +++ clang/test/Analysis/diagnostics/no-store-func-path-notes.cpp @@ -175,3 +175,119 @@ //expected-note@-1{{}} (void)useLocal; } + +//////// + +struct HasRef { + int &a; + HasRef(int &a) : a(a) {} +}; + + +void maybeInitialize(const HasRef &&pA) { + if (coin()) // expected-note{{Assuming the condition is false}} + // expected-note@-1{{Taking false branch}} + pA.a = 120; +} // expected-note{{Returning without writing to 'pA.a'}} + +int useMaybeInitializerWritingIntoField() { + int z; // expected-note{{'z' declared without an initial value}} + maybeInitialize(HasRef(z)); // expected-note{{Calling constructor for 'HasRef'}} + // expected-note@-1{{Returning from constructor for 'HasRef'}} + // expected-note@-2{{Calling 'maybeInitialize'}} + // expected-note@-3{{Returning from 'maybeInitialize'}} + return z; // expected-warning{{Undefined or garbage value returned to caller}} + // expected-note@-1{{Undefined or garbage value returned to caller}} +} + +//////// + +struct HasParentWithRef : public HasRef { + HasParentWithRef(int &a) : HasRef(a) {} // expected-note{{Calling constructor for 'HasRef'}} + // expected-note@-1{{Returning from constructor for 'HasRef'}} +}; + +void maybeInitializeWithParent(const HasParentWithRef &pA) { + if (coin()) // expected-note{{Assuming the condition is false}} + // expected-note@-1{{Taking false branch}} + pA.a = 120; +} // expected-note{{Returning without writing to 'pA.a'}} + +int useMaybeInitializerWritingIntoParentField() { + int z; // expected-note{{'z' declared without an initial value}} + maybeInitializeWithParent(HasParentWithRef(z)); // expected-note{{Calling constructor for 'HasParentWithRef'}} + // expected-note@-1{{Returning from constructor for 'HasParentWithRef'}} + // expected-note@-2{{Calling 'maybeInitializeWithParent'}} + // expected-note@-3{{Returning from 'maybeInitializeWithParent'}} + return z; // expected-warning{{Undefined or garbage value returned to caller}} + // expected-note@-1{{Undefined or garbage value returned to caller}} +} + +//////// + +struct HasIndirectRef { + HasRef &Ref; + HasIndirectRef(HasRef &Ref) : Ref(Ref) {} +}; + +void maybeInitializeIndirectly(const HasIndirectRef &pA) { + if (coin()) // expected-note{{Assuming the condition is false}} + // expected-note@-1{{Taking false branch}} + pA.Ref.a = 120; +} // expected-note{{Returning without writing to 'pA.Ref.a'}} + +int useMaybeInitializeIndirectly() { + int z; // expected-note{{'z' declared without an initial value}} + HasRef r(z); // expected-note{{Calling constructor for 'HasRef'}} + // expected-note@-1{{Returning from constructor for 'HasRef'}} + maybeInitializeIndirectly(HasIndirectRef(r)); // expected-note{{Calling 'maybeInitializeIndirectly'}} + // expected-note@-1{{Returning from 'maybeInitializeIndirectly'}} + return z; // expected-warning{{Undefined or garbage value returned to caller}} + // expected-note@-1{{Undefined or garbage value returned to caller}} +} + +//////// + +struct HasIndirectRefByValue { + HasRef Ref; + HasIndirectRefByValue(HasRef Ref) : Ref(Ref) {} +}; + +void maybeInitializeIndirectly(const HasIndirectRefByValue &pA) { + if (coin()) // expected-note{{Assuming the condition is false}} + // expected-note@-1{{Taking false branch}} + pA.Ref.a = 120; +} // expected-note{{Returning without writing to 'pA.Ref.a'}} + +int useMaybeInitializeIndirectlyIndirectRefByValue() { + int z; // expected-note{{'z' declared without an initial value}} + HasRef r(z); // expected-note{{Calling constructor for 'HasRef'}} + // expected-note@-1{{Returning from constructor for 'HasRef'}} + maybeInitializeIndirectly(HasIndirectRefByValue(r)); // expected-note{{Calling 'maybeInitializeIndirectly'}} + // expected-note@-1{{Returning from 'maybeInitializeIndirectly'}} + return z; // expected-warning{{Undefined or garbage value returned to caller}} + // expected-note@-1{{Undefined or garbage value returned to caller}} +} + +//////// + +struct HasIndirectPointerRef { + HasRef *Ref; + HasIndirectPointerRef(HasRef *Ref) : Ref(Ref) {} +}; + +void maybeInitializeIndirectly(const HasIndirectPointerRef &pA) { + if (coin()) // expected-note{{Assuming the condition is false}} + // expected-note@-1{{Taking false branch}} + pA.Ref->a = 120; +} // expected-note{{Returning without writing to 'pA.Ref.a'}} + +int useMaybeInitializeIndirectlyWithPointer() { + int z; // expected-note{{'z' declared without an initial value}} + HasRef r(z); // expected-note{{Calling constructor for 'HasRef'}} + // expected-note@-1{{Returning from constructor for 'HasRef'}} + maybeInitializeIndirectly(HasIndirectPointerRef(&r)); // expected-note{{Calling 'maybeInitializeIndirectly'}} + // expected-note@-1{{Returning from 'maybeInitializeIndirectly'}} + return z; // expected-warning{{Undefined or garbage value returned to caller}} + // expected-note@-1{{Undefined or garbage value returned to caller}} +}