NoQ created this revision.
In code
void foo(int *p) {
if (p) {}
}
void bar(int *p) {
foo(p);
*p = 5;
}
we suppress the null dereference warning in `*p = 5` because the state split
within the inlined function is essentially irrelevant, as this is merely a
defensive check that does not necessarily trigger in this caller context.
The suppression works by tracking the null pointer symbol in a bug report
visitor and seeing if it was constrained to 0 inside a callee context. The fact
that we have to rely on such manual suppressions is a downside of the
inlining-based approach to interprocedural analysis.
The suppression gets confused when the null dereference becomes more complex,
eg:
struct S {
int x;
}
void foo(struct S *s) {
if (s) {}
}
void bar(struct S *s) {
foo(s);
*(&(s->x)) = 5;
}
In this case `s` is `&SymRegion{reg_$0<s>}`, and `reg_$0<s>` is constrained to
`[0, 0]`, similarly to the above. However, while computing `s->x` and then
`&(s->x)`, the symbol is collapsed to a constant `0 (Loc)`, making it harder to
track the symbol. The visitor is improved to handle this case.
While the fact that //offset of field `x` in struct `S` in our example is equal
to 0// is purely accidental, with any other offset it'd still be a null
dereference, worth suppressing. How does the analyzer handle the non-zero
offset case and still see a null dereference? Simply and powerfully: it
mis-computes the offset, incorrectly believing that all offsets are equal to 0
(woohoo!). I add a test case to document this behavior; even though it's
trivial to fix, the positive side effects of this bug are for now too useful to
discard.
https://reviews.llvm.org/D31982
Files:
lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
lib/StaticAnalyzer/Core/Store.cpp
test/Analysis/explain-svals.cpp
test/Analysis/inlining/inline-defensive-checks.c
test/Analysis/inlining/inline-defensive-checks.cpp
test/Analysis/uninit-const.cpp
Index: test/Analysis/uninit-const.cpp
===================================================================
--- test/Analysis/uninit-const.cpp
+++ test/Analysis/uninit-const.cpp
@@ -122,7 +122,7 @@
}
void f_uninit(void) {
- int x;
+ int x; // expected-note {{'x' declared without an initial value}}
doStuff_uninit(&x); // expected-warning {{1st function call argument is a pointer to uninitialized value}}
// expected-note@-1 {{1st function call argument is a pointer to uninitialized value}}
}
Index: test/Analysis/inlining/inline-defensive-checks.cpp
===================================================================
--- test/Analysis/inlining/inline-defensive-checks.cpp
+++ test/Analysis/inlining/inline-defensive-checks.cpp
@@ -70,4 +70,17 @@
void test(int *p1, int *p2) {
idc(p1);
Foo f(p1);
-}
\ No newline at end of file
+}
+
+struct Bar {
+ int x;
+};
+void idcBar(Bar *b) {
+ if (b)
+ ;
+}
+void testRefToField(Bar *b) {
+ idcBar(b);
+ int &x = b->x; // no-warning
+ x = 5;
+}
Index: test/Analysis/inlining/inline-defensive-checks.c
===================================================================
--- test/Analysis/inlining/inline-defensive-checks.c
+++ test/Analysis/inlining/inline-defensive-checks.c
@@ -1,7 +1,7 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core -analyzer-config suppress-inlined-defensive-checks=true -verify %s
// Perform inline defensive checks.
-void idc(int *p) {
+void idc(void *p) {
if (p)
;
}
@@ -139,3 +139,42 @@
int z = y;
idcTriggerZeroValueThroughCall(z);
}
+
+struct S {
+ int f1;
+ int f2;
+};
+
+void idcTrackZeroValueThroughUnaryPointerOperators(struct S *s) {
+ idc(s);
+ *(&(s->f1)) = 7; // no-warning
+}
+
+void idcTrackZeroValueThroughUnaryPointerOperatorsWithOffset1(struct S *s) {
+ idc(s);
+ int *x = &(s->f2);
+ *x = 7; // no-warning
+}
+
+void idcTrackZeroValueThroughUnaryPointerOperatorsWithOffset2(struct S *s) {
+ idc(s);
+ int *x = &(s->f2) - 1;
+ // FIXME: Should not warn.
+ *x = 7; // expected-warning{{Dereference of null pointer}}
+}
+
+void idcTrackZeroValueThroughUnaryPointerOperatorsWithAssignment(struct S *s) {
+ idc(s);
+ int *x = &(s->f1);
+ *x = 7; // no-warning
+}
+
+
+struct S2 {
+ int a[1];
+};
+
+void idcTrackZeroValueThroughUnaryPointerOperatorsWithArrayField(struct S2 *s) {
+ idc(s);
+ *(&(s->a[0])) = 7; // no-warning
+}
Index: test/Analysis/explain-svals.cpp
===================================================================
--- test/Analysis/explain-svals.cpp
+++ test/Analysis/explain-svals.cpp
@@ -81,9 +81,10 @@
namespace {
class C {
+public:
int x[10];
+ int y;
-public:
void test_5(int i) {
clang_analyzer_explain(this); // expected-warning-re{{{{^pointer to 'this' object$}}}}
clang_analyzer_explain(&x[i]); // expected-warning-re{{{{^pointer to element of type 'int' with index 'argument 'i'' of field 'x' of 'this' object$}}}}
@@ -96,3 +97,9 @@
clang_analyzer_explain(conjure_S()); // expected-warning-re{{{{^lazily frozen compound value of temporary object constructed at statement 'conjure_S\(\)'$}}}}
clang_analyzer_explain(conjure_S().z); // expected-warning-re{{{{^value derived from \(symbol of type 'struct S' conjured at statement 'conjure_S\(\)'\) for field 'z' of temporary object constructed at statement 'conjure_S\(\)'$}}}}
}
+
+void test_7() {
+ C *c = 0;
+ // FIXME: we need to be explaining '40' rather than '0' here; not explainer bug.
+ clang_analyzer_explain(&c->y); // expected-warning-re{{{{^concrete memory address '0'$}}}}
+}
Index: lib/StaticAnalyzer/Core/Store.cpp
===================================================================
--- lib/StaticAnalyzer/Core/Store.cpp
+++ lib/StaticAnalyzer/Core/Store.cpp
@@ -406,6 +406,10 @@
// FIXME: What we should return is the field offset. For example,
// add the field offset to the integer value. That way funny things
// like this work properly: &(((struct foo *) 0xa)->f)
+ // However, that's not easy to fix without reducing our abilities
+ // to catch null pointer dereference. Eg., ((struct foo *)0x0)->f = 7
+ // is a null dereference even though we're dereferencing offset of f
+ // rather than null.
return Base;
default:
Index: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===================================================================
--- lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -961,8 +961,31 @@
const Expr *Inner = nullptr;
if (const Expr *Ex = dyn_cast<Expr>(S)) {
Ex = Ex->IgnoreParenCasts();
- if (ExplodedGraph::isInterestingLValueExpr(Ex) || CallEvent::isCallStmt(Ex))
+
+ // Performing operator `&' on an lvalue expression is essentially a no-op.
+ // Then, if we are taking addresses of fields or elements, these are also
+ // unlikely to matter.
+ // FIXME: Currently offsets of fields are computed incorrectly,
+ // being always equal to 0. See the FIXME in StoreManager's
+ // getLValueFieldOrIvar() method. This code interacts heavily with
+ // this incorrect computation; otherwise the value would not be null at all
+ // for most fields.
+ if (const auto *Op = dyn_cast<UnaryOperator>(Ex))
+ if (Op->getOpcode() == UO_AddrOf)
+ if (Op->getSubExpr()->isLValue()) {
+ Ex = Op->getSubExpr()->IgnoreParenCasts();
+ while (true) {
+ if (const auto *ME = dyn_cast<MemberExpr>(Ex)) {
+ Ex = ME->getBase()->IgnoreParenCasts();
+ } else {
+ break;
+ }
+ }
+ }
+
+ if (ExplodedGraph::isInterestingLValueExpr(Ex) || CallEvent::isCallStmt(Ex)) {
Inner = Ex;
+ }
}
if (IsArg && !Inner) {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
