NoQ created this revision.
When encountering an array-to-pointer-decay and the array base is null (or any
other concrete pointer value) (eg. it's a member array in a structure, and the
structure pointer is null; of course it wouldn't happen to stack-based or
global arrays), do not yield UnknownVal; instead, yield that concrete value.
While obvious, this change now triggers false positives because our suppression
for inlined defensive checks was not prepared for dealing with array subscripts
(the `idcTrackZeroValueThroughUnaryPointerOperatorsWithArrayField` test in
`inlining/inline-defensive-checks.cpp` starts failing). So i additionally
improve the suppression.
As discussed in https://reviews.llvm.org/D31982, which added the aforementioned
test case, `bugreporter::getDerefExpr()` should have been used (we only used to
match member expressions earlier, but now that we encountered arrays, we could
use all the features it function can offer). Now that the code uses that
function, and a few issues within that function were further fixed in order to
support the new use case and avoid regressions.
https://reviews.llvm.org/D32291
Files:
lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
lib/StaticAnalyzer/Core/RegionStore.cpp
test/Analysis/null-deref-offsets.c
Index: test/Analysis/null-deref-offsets.c
===================================================================
--- test/Analysis/null-deref-offsets.c
+++ test/Analysis/null-deref-offsets.c
@@ -7,7 +7,7 @@
int z[2];
};
-void testOffsets(struct S *s) {
+void testOffsets(struct S *s, int coin) {
if (s != 0)
return;
@@ -21,14 +21,17 @@
// FIXME: These should ideally be true.
clang_analyzer_eval(&(s->y) == 4); // expected-warning{{FALSE}}
- clang_analyzer_eval(&(s->z[0]) == 8); // expected-warning{{UNKNOWN}}
- clang_analyzer_eval(&(s->z[1]) == 12); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(&(s->z[0]) == 8); // expected-warning{{FALSE}}
+ clang_analyzer_eval(&(s->z[1]) == 12); // expected-warning{{FALSE}}
// FIXME: These should ideally be false.
clang_analyzer_eval(&(s->y) == 0); // expected-warning{{TRUE}}
- clang_analyzer_eval(&(s->z[0]) == 0); // expected-warning{{UNKNOWN}}
- clang_analyzer_eval(&(s->z[1]) == 0); // expected-warning{{UNKNOWN}}
-
- // But this should still be a null dereference.
- s->y = 5; // expected-warning{{Access to field 'y' results in a dereference
of a null pointer (loaded from variable 's')}}
+ clang_analyzer_eval(&(s->z[0]) == 0); // expected-warning{{TRUE}}
+ clang_analyzer_eval(&(s->z[1]) == 0); // expected-warning{{TRUE}}
+
+ // But these should still be reported as null dereferences.
+ if (coin)
+ s->y = 5; // expected-warning{{Access to field 'y' results in a
dereference of a null pointer (loaded from variable 's')}}
+ else
+ s->z[1] = 6; // expected-warning{{Array access (via field 'z') results in
a null pointer dereference}}
}
Index: lib/StaticAnalyzer/Core/RegionStore.cpp
===================================================================
--- lib/StaticAnalyzer/Core/RegionStore.cpp
+++ lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1338,6 +1338,9 @@
/// the array). This is called by ExprEngine when evaluating casts
/// from arrays to pointers.
SVal RegionStoreManager::ArrayToPointer(Loc Array, QualType T) {
+ if (Array.getAs<loc::ConcreteInt>())
+ return Array;
+
if (!Array.getAs<loc::MemRegionVal>())
return UnknownVal();
Index: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===================================================================
--- lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -61,7 +61,9 @@
return U->getSubExpr()->IgnoreParenCasts();
}
else if (const MemberExpr *ME = dyn_cast<MemberExpr>(E)) {
- if (ME->isArrow() || isDeclRefExprToReference(ME->getBase())) {
+ if (ME->isImplicitAccess()) {
+ return ME;
+ } else if (ME->isArrow() || isDeclRefExprToReference(ME->getBase())) {
return ME->getBase()->IgnoreParenCasts();
} else {
// If we have a member expr with a dot, the base must have been
@@ -73,9 +75,9 @@
return IvarRef->getBase()->IgnoreParenCasts();
}
else if (const ArraySubscriptExpr *AE = dyn_cast<ArraySubscriptExpr>(E)) {
- return AE->getBase();
+ return getDerefExpr(AE->getBase());
}
- else if (isDeclRefExprToReference(E)) {
+ else if (isa<DeclRefExpr>(E)) {
return E;
}
break;
@@ -974,14 +976,11 @@
// This code interacts heavily with this hack; otherwise the value
// would not be null at all for most fields, so we'd be unable to track it.
if (const auto *Op = dyn_cast<UnaryOperator>(Ex))
- if (Op->getOpcode() == UO_AddrOf && Op->getSubExpr()->isLValue()) {
- Ex = Op->getSubExpr()->IgnoreParenCasts();
- while (const auto *ME = dyn_cast<MemberExpr>(Ex)) {
- Ex = ME->getBase()->IgnoreParenCasts();
- }
- }
+ if (Op->getOpcode() == UO_AddrOf && Op->getSubExpr()->isLValue())
+ if (const Expr *DerefEx = getDerefExpr(Op->getSubExpr()))
+ Ex = DerefEx;
- if (ExplodedGraph::isInterestingLValueExpr(Ex) ||
CallEvent::isCallStmt(Ex))
+ if (Ex && (ExplodedGraph::isInterestingLValueExpr(Ex) ||
CallEvent::isCallStmt(Ex)))
Inner = Ex;
}
Index: test/Analysis/null-deref-offsets.c
===================================================================
--- test/Analysis/null-deref-offsets.c
+++ test/Analysis/null-deref-offsets.c
@@ -7,7 +7,7 @@
int z[2];
};
-void testOffsets(struct S *s) {
+void testOffsets(struct S *s, int coin) {
if (s != 0)
return;
@@ -21,14 +21,17 @@
// FIXME: These should ideally be true.
clang_analyzer_eval(&(s->y) == 4); // expected-warning{{FALSE}}
- clang_analyzer_eval(&(s->z[0]) == 8); // expected-warning{{UNKNOWN}}
- clang_analyzer_eval(&(s->z[1]) == 12); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(&(s->z[0]) == 8); // expected-warning{{FALSE}}
+ clang_analyzer_eval(&(s->z[1]) == 12); // expected-warning{{FALSE}}
// FIXME: These should ideally be false.
clang_analyzer_eval(&(s->y) == 0); // expected-warning{{TRUE}}
- clang_analyzer_eval(&(s->z[0]) == 0); // expected-warning{{UNKNOWN}}
- clang_analyzer_eval(&(s->z[1]) == 0); // expected-warning{{UNKNOWN}}
-
- // But this should still be a null dereference.
- s->y = 5; // expected-warning{{Access to field 'y' results in a dereference of a null pointer (loaded from variable 's')}}
+ clang_analyzer_eval(&(s->z[0]) == 0); // expected-warning{{TRUE}}
+ clang_analyzer_eval(&(s->z[1]) == 0); // expected-warning{{TRUE}}
+
+ // But these should still be reported as null dereferences.
+ if (coin)
+ s->y = 5; // expected-warning{{Access to field 'y' results in a dereference of a null pointer (loaded from variable 's')}}
+ else
+ s->z[1] = 6; // expected-warning{{Array access (via field 'z') results in a null pointer dereference}}
}
Index: lib/StaticAnalyzer/Core/RegionStore.cpp
===================================================================
--- lib/StaticAnalyzer/Core/RegionStore.cpp
+++ lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1338,6 +1338,9 @@
/// the array). This is called by ExprEngine when evaluating casts
/// from arrays to pointers.
SVal RegionStoreManager::ArrayToPointer(Loc Array, QualType T) {
+ if (Array.getAs<loc::ConcreteInt>())
+ return Array;
+
if (!Array.getAs<loc::MemRegionVal>())
return UnknownVal();
Index: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===================================================================
--- lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -61,7 +61,9 @@
return U->getSubExpr()->IgnoreParenCasts();
}
else if (const MemberExpr *ME = dyn_cast<MemberExpr>(E)) {
- if (ME->isArrow() || isDeclRefExprToReference(ME->getBase())) {
+ if (ME->isImplicitAccess()) {
+ return ME;
+ } else if (ME->isArrow() || isDeclRefExprToReference(ME->getBase())) {
return ME->getBase()->IgnoreParenCasts();
} else {
// If we have a member expr with a dot, the base must have been
@@ -73,9 +75,9 @@
return IvarRef->getBase()->IgnoreParenCasts();
}
else if (const ArraySubscriptExpr *AE = dyn_cast<ArraySubscriptExpr>(E)) {
- return AE->getBase();
+ return getDerefExpr(AE->getBase());
}
- else if (isDeclRefExprToReference(E)) {
+ else if (isa<DeclRefExpr>(E)) {
return E;
}
break;
@@ -974,14 +976,11 @@
// This code interacts heavily with this hack; otherwise the value
// would not be null at all for most fields, so we'd be unable to track it.
if (const auto *Op = dyn_cast<UnaryOperator>(Ex))
- if (Op->getOpcode() == UO_AddrOf && Op->getSubExpr()->isLValue()) {
- Ex = Op->getSubExpr()->IgnoreParenCasts();
- while (const auto *ME = dyn_cast<MemberExpr>(Ex)) {
- Ex = ME->getBase()->IgnoreParenCasts();
- }
- }
+ if (Op->getOpcode() == UO_AddrOf && Op->getSubExpr()->isLValue())
+ if (const Expr *DerefEx = getDerefExpr(Op->getSubExpr()))
+ Ex = DerefEx;
- if (ExplodedGraph::isInterestingLValueExpr(Ex) || CallEvent::isCallStmt(Ex))
+ if (Ex && (ExplodedGraph::isInterestingLValueExpr(Ex) || CallEvent::isCallStmt(Ex)))
Inner = Ex;
}
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
