From 04dcb7235f279306376be64a525311c322f90c7e Mon Sep 17 00:00:00 2001 From: Jordan Rose Date: Sat, 22 Sep 2012 01:24:33 +0000 Subject: [PATCH] [analyzer] Check that a member expr is valid even when the result is an lvalue. We want to catch cases like this early, so that we can produce better diagnostics and path notes: Point *p = 0; int *px = &p->x; // should warn here *px = 1; llvm-svn: 164441 --- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 26 +++++++++++++------- clang/test/Analysis/fields.c | 7 ++++++ clang/test/Analysis/nullptr.cpp | 9 ++++--- 3 files changed, 29 insertions(+), 13 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index 3e5733f10c07..8e2c159ca7d7 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -1515,22 +1515,30 @@ void ExprEngine::VisitMemberExpr(const MemberExpr *M, ExplodedNode *Pred, return; } - // FIXME: Should we insert some assumption logic in here to determine - // if "Base" is a valid piece of memory? Before we put this assumption - // later when using FieldOffset lvals (which we no longer have). - // For all other cases, compute an lvalue. SVal L = state->getLValue(field, baseExprVal); if (M->isGLValue()) { + ExplodedNodeSet Tmp; + Bldr.takeNodes(Pred); + evalLocation(Tmp, M, M, Pred, state, baseExprVal, + /*Tag=*/0, /*isLoad=*/true); + Bldr.addNodes(Tmp); + + const MemRegion *ReferenceRegion = 0; if (field->getType()->isReferenceType()) { - if (const MemRegion *R = L.getAsRegion()) - L = state->getSVal(R); - else + ReferenceRegion = L.getAsRegion(); + if (!ReferenceRegion) L = UnknownVal(); } - Bldr.generateNode(M, Pred, state->BindExpr(M, LCtx, L), 0, - ProgramPoint::PostLValueKind); + for (ExplodedNodeSet::iterator I = Tmp.begin(), E = Tmp.end(); I != E; ++I){ + state = (*I)->getState(); + if (ReferenceRegion) + L = state->getSVal(ReferenceRegion); + + Bldr.generateNode(M, (*I), state->BindExpr(M, LCtx, L), 0, + ProgramPoint::PostLValueKind); + } } else { Bldr.takeNodes(Pred); evalLoad(Dst, M, M, Pred, state, L); diff --git a/clang/test/Analysis/fields.c b/clang/test/Analysis/fields.c index da0847a56074..a10d5a806016 100644 --- a/clang/test/Analysis/fields.c +++ b/clang/test/Analysis/fields.c @@ -26,3 +26,10 @@ void test() { Point p; (void)(p = getit()).x; } + + +void testNullAddress() { + Point *p = 0; + int *px = &p->x; // expected-warning{{Access to field 'x' results in a dereference of a null pointer (loaded from variable 'p')}} + *px = 1; // No warning because analysis stops at the previous line. +} diff --git a/clang/test/Analysis/nullptr.cpp b/clang/test/Analysis/nullptr.cpp index 050c3f8dc56a..80ef5fbf6d05 100644 --- a/clang/test/Analysis/nullptr.cpp +++ b/clang/test/Analysis/nullptr.cpp @@ -23,10 +23,11 @@ void foo3(void) { }; char *np = nullptr; // casting a nullptr to anything should be caught eventually - int *ip = &(((struct foo *)np)->f); - *ip = 0; // expected-warning{{Dereference of null pointer}} - // should be error here too, but analysis gets stopped -// *np = 0; + int *ip = &(((struct foo *)np)->f); // expected-warning{{Access to field 'f' results in a dereference of a null pointer (loaded from variable 'np')}} + + // Analysis stops at the first problem case, so we won't actually warn here. + *ip = 0; + *np = 0; } // nullptr is implemented as a zero integer value, so should be able to compare