Author: dergachev Date: Fri Apr 5 13:18:53 2019 New Revision: 357810 URL: http://llvm.org/viewvc/llvm-project?rev=357810&view=rev Log: [analyzer] NoStoreFuncVisitor: Suppress reports with no-store in system headers.
The idea behind this heuristic is that normally the visitor is there to inform the user that a certain function may fail to initialize a certain out-parameter. For system header functions this is usually dictated by the contract, and it's unlikely that the header function has accidentally forgot to put the value into the out-parameter; it's more likely that the user has intentionally skipped the error check. Warnings on skipped error checks are more like security warnings; they aren't necessarily useful for all users, and they should instead be introduced on a per-API basis. Differential Revision: https://reviews.llvm.org/D60107 Added: cfe/trunk/test/Analysis/Inputs/no-store-suppression.h cfe/trunk/test/Analysis/no-store-suppression.cpp Modified: cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp Modified: cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp?rev=357810&r1=357809&r2=357810&view=diff ============================================================================== --- cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp (original) +++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp Fri Apr 5 13:18:53 2019 @@ -306,9 +306,14 @@ public: ID.AddPointer(RegionOfInterest); } + void *getTag() const { + static int Tag = 0; + return static_cast<void *>(&Tag); + } + std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N, BugReporterContext &BR, - BugReport &) override { + BugReport &R) override { const LocationContext *Ctx = N->getLocationContext(); const StackFrameContext *SCtx = Ctx->getStackFrame(); @@ -322,9 +327,6 @@ public: CallEventRef<> Call = BR.getStateManager().getCallEventManager().getCaller(SCtx, State); - if (Call->isInSystemHeader()) - 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<ObjCMethodCall>(Call)) { @@ -333,8 +335,8 @@ public: if (RegionOfInterest->isSubRegionOf(SelfRegion) && potentiallyWritesIntoIvar(Call->getRuntimeDefinition().getDecl(), IvarR->getDecl())) - return notModifiedDiagnostics(N, {}, SelfRegion, "self", - /*FirstIsReferenceType=*/false, 1); + return maybeEmitNode(R, *Call, N, {}, SelfRegion, "self", + /*FirstIsReferenceType=*/false, 1); } } @@ -342,8 +344,8 @@ public: const MemRegion *ThisR = CCall->getCXXThisVal().getAsRegion(); if (RegionOfInterest->isSubRegionOf(ThisR) && !CCall->getDecl()->isImplicit()) - return notModifiedDiagnostics(N, {}, ThisR, "this", - /*FirstIsReferenceType=*/false, 1); + return maybeEmitNode(R, *Call, N, {}, ThisR, "this", + /*FirstIsReferenceType=*/false, 1); // Do not generate diagnostics for not modified parameters in // constructors. @@ -353,27 +355,26 @@ public: ArrayRef<ParmVarDecl *> parameters = getCallParameters(Call); for (unsigned I = 0; I < Call->getNumArgs() && I < parameters.size(); ++I) { const ParmVarDecl *PVD = parameters[I]; - SVal S = Call->getArgSVal(I); + SVal V = Call->getArgSVal(I); bool ParamIsReferenceType = PVD->getType()->isReferenceType(); std::string ParamName = PVD->getNameAsString(); int IndirectionLevel = 1; QualType T = PVD->getType(); - while (const MemRegion *R = S.getAsRegion()) { - if (RegionOfInterest->isSubRegionOf(R) && !isPointerToConst(T)) - return notModifiedDiagnostics(N, {}, R, ParamName, - ParamIsReferenceType, IndirectionLevel); + while (const MemRegion *MR = V.getAsRegion()) { + if (RegionOfInterest->isSubRegionOf(MR) && !isPointerToConst(T)) + return maybeEmitNode(R, *Call, N, {}, MR, ParamName, + ParamIsReferenceType, IndirectionLevel); QualType PT = T->getPointeeType(); if (PT.isNull() || PT->isVoidType()) break; if (const RecordDecl *RD = PT->getAsRecordDecl()) - if (auto P = findRegionOfInterestInRecord(RD, State, R)) - return notModifiedDiagnostics(N, *P, RegionOfInterest, ParamName, - ParamIsReferenceType, - IndirectionLevel); + if (auto P = findRegionOfInterestInRecord(RD, State, MR)) + return maybeEmitNode(R, *Call, N, *P, RegionOfInterest, ParamName, + ParamIsReferenceType, IndirectionLevel); - S = State->getSVal(R, PT); + V = State->getSVal(MR, PT); T = PT; IndirectionLevel++; } @@ -543,11 +544,37 @@ private: Ty->getPointeeType().getCanonicalType().isConstQualified(); } - /// \return Diagnostics piece for region not modified in the current function. + /// Consume the information on the no-store stack frame in order to + /// either emit a note or suppress the report enirely. + /// \return Diagnostics piece for region not modified in the current function, + /// if it decides to emit one. std::shared_ptr<PathDiagnosticPiece> - notModifiedDiagnostics(const ExplodedNode *N, const RegionVector &FieldChain, - const MemRegion *MatchedRegion, StringRef FirstElement, - bool FirstIsReferenceType, unsigned IndirectionLevel) { + maybeEmitNode(BugReport &R, const CallEvent &Call, const ExplodedNode *N, + const RegionVector &FieldChain, const MemRegion *MatchedRegion, + StringRef FirstElement, bool FirstIsReferenceType, + unsigned IndirectionLevel) { + // Optimistically suppress uninitialized value bugs that result + // from system headers having a chance to initialize the value + // but failing to do so. It's too unlikely a system header's fault. + // It's much more likely a situation in which the function has a failure + // mode that the user decided not to check. If we want to hunt such + // omitted checks, we should provide an explicit function-specific note + // describing the precondition under which the function isn't supposed to + // initialize its out-parameter, and additionally check that such + // precondition can actually be fulfilled on the current path. + if (Call.isInSystemHeader()) { + // We make an exception for system header functions that have no branches, + // i.e. have exactly 3 CFG blocks: begin, all its code, end. Such + // functions unconditionally fail to initialize the variable. + // If they call other functions that have more paths within them, + // this suppression would still apply when we visit these inner functions. + // One common example of a standard function that doesn't ever initialize + // its out parameter is operator placement new; it's up to the follow-up + // constructor (if any) to initialize the memory. + if (N->getStackFrame()->getCFG()->size() > 3) + R.markInvalid(getTag(), nullptr); + return nullptr; + } PathDiagnosticLocation L = PathDiagnosticLocation::create(N->getLocation(), SM); Added: cfe/trunk/test/Analysis/Inputs/no-store-suppression.h URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/Inputs/no-store-suppression.h?rev=357810&view=auto ============================================================================== --- cfe/trunk/test/Analysis/Inputs/no-store-suppression.h (added) +++ cfe/trunk/test/Analysis/Inputs/no-store-suppression.h Fri Apr 5 13:18:53 2019 @@ -0,0 +1,17 @@ +#pragma clang system_header + +namespace std { +class istream { +public: + bool is_eof(); + char get_char(); +}; + +istream &operator>>(istream &is, char &c) { + if (is.is_eof()) + return; + c = is.get_char(); +} + +extern istream cin; +}; Added: cfe/trunk/test/Analysis/no-store-suppression.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/no-store-suppression.cpp?rev=357810&view=auto ============================================================================== --- cfe/trunk/test/Analysis/no-store-suppression.cpp (added) +++ cfe/trunk/test/Analysis/no-store-suppression.cpp Fri Apr 5 13:18:53 2019 @@ -0,0 +1,22 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s + +// expected-no-diagnostics + +#include "Inputs/no-store-suppression.h" + +using namespace std; + +namespace value_uninitialized_after_stream_shift { +void use(char c); + +// Technically, it is absolutely necessary to check the status of cin after +// read before using the value that just read from it. Practically, we don't +// really care unless we eventually come up with a special security check +// for just that purpose. Static Analyzer shouldn't be yelling at every person's +// third program in their C++ 101. +void foo() { + char c; + std::cin >> c; + use(c); // no-warning +} +} // namespace value_uninitialized_after_stream_shift _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits