NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, rnkovacs, mikhail.ramalho,
Szelethus, baloghadamsoftware, Charusso.
Herald added subscribers: cfe-commits, jdoerfert, dkrupp, donat.nagy,
a.sidorin, szepet.
Herald added a project: clang.
Hmm, i literally patched the same line of code a few days ago in D59901
<https://reviews.llvm.org/D59901>. That's fairly accidental.
`NoStoreFuncVisitor` is mostly attached to uninitialized value reports and is
responsible for adding path notes within (a.k.a. disabling pruning of) inlined
calls that could have initialized the memory region but didn't end up doing it.
It emits notes that say `Returning without writing to ...` at the respective
return sites of such calls.
George decided to suppress the note for calls into system headers. I guess the
reason was that it was otherwise too loud. After all, it's an un-pruning effort
- it can cause massive bring-ins of inlined calls into reports.
However, when the note is suppressed, the very original issue that caused us to
write this visitor bites us again: the report becomes incomprehensible. Here's
a specific example i'm looking at:
#include <iostream>
void use(char);
void foo() {
char A;
std::cin >> A;
use(A); // Use of uninitialized variable?!
}
This is a "true" positive: there are a bunch of failure modes in `std::cin`
that may lead to not initializing the variable and the developer has to check
for them before using the variable. However,
- You'll never be able to understand that from the report;
- Even if it's true, the user would most likely still not bother fixing it
unless it's a security-critical application.
What our options here?
1. We can model `operator>>()`. We can either model it as something that always
initializes the value to an unknown (and possibly tainted) value, or force a
state split with a specific note about the contract of the operator, such as
`Value A may remain uninitialized when B is called (ρ=0.56), given C, assuming
D and under E conditions` (we'll have to also make sure that these
preconditions are compatible with the current state). That's the ideal solution
because it gives us the perfect modeling that we want and gives us ultimate
flexibility.
2. We can disable inlining of `operator>>()` and maybe other system header
functions that take out-parameters.
3. We can suppress bug reports that would have caused the no-store visitor to
emit its notes in system headers.
In this patch i'm implementing solution 3.
Solution 1 is not incompatible with solutions 2 and 3; it can be incrementally
added on top of either solution 2 or solution 3 as a more man-hour-expensive
incremental improvement (suppress inlining/reporting but also unsuppress by
modeling a few known functions explicitly).
Solution 2 is similar to our container inlining heuristic: just don't bother
inlining 'cause we'll never understand what's going on anyway. I ended up
hating that heuristic and dreaming of carefully removing it and replacing it
with visitor-based suppressions such as the solution 3. By suppressing
inlining, we have all of the downsides of the conservative evaluation: we end
up exploring obviously infeasible execution paths because we're losing
information about the program. Solution 3 is more targeted: it only suppresses
reports of a specific checker for which the problem has actually manifested
rather than all checkers, it doesn't cause arbitrary unpredictable coverage
skew, and it doesn't destroy any valid information that we managed to obtain
during inlining.
The patch is trivial and mostly consists of inevitable renaming functions and
variables. There's one interesting gotcha though: if the function has no
branches whatsoever, disable the suppression. Like, if the function
unconditionally fails to initialize anything, the developer probably knows
about that. I think we should do more of such un-suppressions. This was
inspired by a test case in `test/Analysis/new.cpp` that otherwise regressed:
200 int testNoInitializationPlacement() {
201 int n;
202 new (&n) int; // Doesn't initialize 'n'!
203
204 if (n) { // expected-warning{{Branch condition evaluates to a garbage
value}}
205 return 0;
206 }
207 return 1;
208 }
Repository:
rC Clang
https://reviews.llvm.org/D60107
Files:
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
clang/test/Analysis/Inputs/no-store-suppression.h
clang/test/Analysis/no-store-suppression.cpp
Index: clang/test/Analysis/no-store-suppression.cpp
===================================================================
--- /dev/null
+++ clang/test/Analysis/no-store-suppression.cpp
@@ -0,0 +1,17 @@
+// 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);
+
+void foo() {
+ char c;
+ std::cin >> c;
+ use(c); // no-warning
+}
+} // namespace value_uninitialized_after_stream_shift
Index: clang/test/Analysis/Inputs/no-store-suppression.h
===================================================================
--- /dev/null
+++ 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;
+};
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -306,9 +306,14 @@
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 @@
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 @@
if (RegionOfInterest->isSubRegionOf(SelfRegion) &&
potentiallyWritesIntoIvar(Call->getRuntimeDefinition().getDecl(),
IvarR->getDecl()))
- return notModifiedDiagnostics(N, {}, SelfRegion, "self",
- /*FirstIsReferenceType=*/false, 1);
+ return emitNoteOrSuppress(R, *Call, N, {}, SelfRegion, "self",
+ /*FirstIsReferenceType=*/false, 1);
}
}
@@ -342,8 +344,8 @@
const MemRegion *ThisR = CCall->getCXXThisVal().getAsRegion();
if (RegionOfInterest->isSubRegionOf(ThisR)
&& !CCall->getDecl()->isImplicit())
- return notModifiedDiagnostics(N, {}, ThisR, "this",
- /*FirstIsReferenceType=*/false, 1);
+ return emitNoteOrSuppress(R, *Call, N, {}, ThisR, "this",
+ /*FirstIsReferenceType=*/false, 1);
// Do not generate diagnostics for not modified parameters in
// constructors.
@@ -359,21 +361,20 @@
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 = S.getAsRegion()) {
+ if (RegionOfInterest->isSubRegionOf(MR) && !isPointerToConst(T))
+ return emitNoteOrSuppress(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 emitNoteOrSuppress(R, *Call, N, *P, RegionOfInterest, ParamName,
+ ParamIsReferenceType, IndirectionLevel);
- S = State->getSVal(R, PT);
+ S = State->getSVal(MR, PT);
T = PT;
IndirectionLevel++;
}
@@ -545,9 +546,29 @@
/// \return Diagnostics piece for region not modified in the current function.
std::shared_ptr<PathDiagnosticPiece>
- notModifiedDiagnostics(const ExplodedNode *N, const RegionVector &FieldChain,
- const MemRegion *MatchedRegion, StringRef FirstElement,
- bool FirstIsReferenceType, unsigned IndirectionLevel) {
+ emitNoteOrSuppress(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.
+ if (N->getStackFrame()->getCFG()->size() > 3)
+ R.markInvalid(getTag(), nullptr);
+ return nullptr;
+ }
PathDiagnosticLocation L =
PathDiagnosticLocation::create(N->getLocation(), SM);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
