From: Artem Dergachev Date: Fri, 5 Apr 2019 20:18:53 +0000 (+0000) Subject: [analyzer] NoStoreFuncVisitor: Suppress reports with no-store in system headers. X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=5c6fc36de89702b9096973cfc5fd9b071d9db2c5;p=platform%2Fupstream%2Fllvm.git [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 llvm-svn: 357810 --- diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp index cd03d16..cf29560 100644 --- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -306,9 +306,14 @@ public: ID.AddPointer(RegionOfInterest); } + void *getTag() const { + static int Tag = 0; + return static_cast(&Tag); + } + std::shared_ptr 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(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 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 - 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); diff --git a/clang/test/Analysis/Inputs/no-store-suppression.h b/clang/test/Analysis/Inputs/no-store-suppression.h new file mode 100644 index 0000000..6f69b6d --- /dev/null +++ b/clang/test/Analysis/Inputs/no-store-suppression.h @@ -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; +}; diff --git a/clang/test/Analysis/no-store-suppression.cpp b/clang/test/Analysis/no-store-suppression.cpp new file mode 100644 index 0000000..0ef4e0c --- /dev/null +++ b/clang/test/Analysis/no-store-suppression.cpp @@ -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