r357810 - [analyzer] NoStoreFuncVisitor: Suppress reports with no-store in system headers.
Artem Dergachev via cfe-commits
cfe-commits at lists.llvm.org
Fri Apr 5 13:18:53 PDT 2019
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
More information about the cfe-commits
mailing list