NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet.
Herald added subscribers: cfe-commits, rnkovacs, eraman.
`bugreporter::trackNullOrUndefValue()` checker API function extends a bug
report with a recursive family of bug report visitors (`ReturnVisitor`,
`FindLastStoreBRVisitor`, etc.) that collectively try to figure out where the
given value came from. In particular, a null or undefined value, which is
useful for null dereferences, uninitialized value checks, or divisions by zero.
This not only improves the bug report with additional helpful diagnostic
pieces, but also helps us suppress bugs that come from inlined defensive checks
(the more solid potential solution for at least some of these false positives
would be to introduce a state merge at call exit, but state merge is a new
operation over the program states, so it would require checker side support,
which is heavy).
In this patch i attempt to add more logic into the tracking, namely to be able
to track a value stored in an arbitrary memory region `R` back to the
expression that caused this value to be stored there. Previously it only worked
when `R` is coming from a `ExplodedGraph::isInterestingLValueExpression()` (the
exact meaning of which is "the respective node would not be reclaimed during
garbage collection" (!), of course it would indeed be a shame if the node we're
looking for disappears). This leaves with plain variables, member variables,
and ObjC instance variables.
`FindLastStoreBRVisitor` is already capable of doing this in the general case
of an arbitrary memory region - not only it finds the node where `getSVal(R)`
changed, but also it finds `PostStore` nodes to this region even if the newly
written value is same as old value.
Note that visitors are deduplicated, so we're not afraid of adding too many
identical visitors.
TODO: For now i just stuffed the visitor in there, and it seems to work. But
i'd like to see how much the existing code for variables can (or even must) be
reused before committing, hence WIP.
Repository:
rC Clang
https://reviews.llvm.org/D41253
Files:
lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
test/Analysis/inlining/inline-defensive-checks.c
test/Analysis/nullptr.cpp
Index: test/Analysis/nullptr.cpp
===================================================================
--- test/Analysis/nullptr.cpp
+++ test/Analysis/nullptr.cpp
@@ -142,8 +142,9 @@
// expected-note@-1{{Passing null pointer value via 1st
parameter 'x'}}
if (getSymbol()) { // expected-note {{Assuming the condition is true}}
// expected-note@-1{{Taking true branch}}
- X *x = Type().x; // expected-note{{'x' initialized to a null pointer
value}}
- x->f(); // expected-warning{{Called C++ object pointer is null}}
+ X *xx = Type().x; // expected-note {{Null pointer value stored to field
'x'}}
+ // expected-note@-1{{'xx' initialized to a null pointer
value}}
+ xx->f(); // expected-warning{{Called C++ object pointer is null}}
// expected-note@-1{{Called C++ object pointer is null}}
}
}
Index: test/Analysis/inlining/inline-defensive-checks.c
===================================================================
--- test/Analysis/inlining/inline-defensive-checks.c
+++ test/Analysis/inlining/inline-defensive-checks.c
@@ -190,3 +190,21 @@
idc(s);
*(&(s->a[0])) = 7; // no-warning
}
+
+void idcTrackConstraintThroughSymbolicRegion(int **x) {
+ idc(*x);
+ // FIXME: Should not warn.
+ **x = 7; // expected-warning{{Dereference of null pointer}}
+}
+
+int *idcPlainNull(int coin) {
+ if (coin)
+ return 0;
+ static int X;
+ return &X;
+}
+
+void idcTrackZeroValueThroughSymbolicRegion(int coin, int **x) {
+ *x = idcPlainNull(coin);
+ **x = 7; // no-warning
+}
Index: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===================================================================
--- lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1142,9 +1142,12 @@
else
RVal = state->getSVal(L->getRegion());
- const MemRegion *RegionRVal = RVal.getAsRegion();
report.addVisitor(llvm::make_unique<UndefOrNullArgVisitor>(L->getRegion()));
+ if (Optional<KnownSVal> KV = RVal.getAs<KnownSVal>())
+ report.addVisitor(llvm::make_unique<FindLastStoreBRVisitor>(
+ *KV, L->getRegion(), EnableNullFPSuppression));
+ const MemRegion *RegionRVal = RVal.getAsRegion();
if (RegionRVal && isa<SymbolicRegion>(RegionRVal)) {
report.markInteresting(RegionRVal);
report.addVisitor(llvm::make_unique<TrackConstraintBRVisitor>(
Index: test/Analysis/nullptr.cpp
===================================================================
--- test/Analysis/nullptr.cpp
+++ test/Analysis/nullptr.cpp
@@ -142,8 +142,9 @@
// expected-note@-1{{Passing null pointer value via 1st parameter 'x'}}
if (getSymbol()) { // expected-note {{Assuming the condition is true}}
// expected-note@-1{{Taking true branch}}
- X *x = Type().x; // expected-note{{'x' initialized to a null pointer value}}
- x->f(); // expected-warning{{Called C++ object pointer is null}}
+ X *xx = Type().x; // expected-note {{Null pointer value stored to field 'x'}}
+ // expected-note@-1{{'xx' initialized to a null pointer value}}
+ xx->f(); // expected-warning{{Called C++ object pointer is null}}
// expected-note@-1{{Called C++ object pointer is null}}
}
}
Index: test/Analysis/inlining/inline-defensive-checks.c
===================================================================
--- test/Analysis/inlining/inline-defensive-checks.c
+++ test/Analysis/inlining/inline-defensive-checks.c
@@ -190,3 +190,21 @@
idc(s);
*(&(s->a[0])) = 7; // no-warning
}
+
+void idcTrackConstraintThroughSymbolicRegion(int **x) {
+ idc(*x);
+ // FIXME: Should not warn.
+ **x = 7; // expected-warning{{Dereference of null pointer}}
+}
+
+int *idcPlainNull(int coin) {
+ if (coin)
+ return 0;
+ static int X;
+ return &X;
+}
+
+void idcTrackZeroValueThroughSymbolicRegion(int coin, int **x) {
+ *x = idcPlainNull(coin);
+ **x = 7; // no-warning
+}
Index: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===================================================================
--- lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1142,9 +1142,12 @@
else
RVal = state->getSVal(L->getRegion());
- const MemRegion *RegionRVal = RVal.getAsRegion();
report.addVisitor(llvm::make_unique<UndefOrNullArgVisitor>(L->getRegion()));
+ if (Optional<KnownSVal> KV = RVal.getAs<KnownSVal>())
+ report.addVisitor(llvm::make_unique<FindLastStoreBRVisitor>(
+ *KV, L->getRegion(), EnableNullFPSuppression));
+ const MemRegion *RegionRVal = RVal.getAsRegion();
if (RegionRVal && isa<SymbolicRegion>(RegionRVal)) {
report.markInteresting(RegionRVal);
report.addVisitor(llvm::make_unique<TrackConstraintBRVisitor>(
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
