Index: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp =================================================================== --- lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -988,6 +988,65 @@ return Ex; } +/// Walk through nodes until we get one that matches the statement exactly. +/// Alternately, if we hit a known lvalue for the statement, we know we've +/// gone too far (though we can likely track the lvalue better anyway). +static const ExplodedNode* findNodeForStatement(const ExplodedNode *N, + const Stmt *S, + const Expr *Inner) { + do { + const ProgramPoint &pp = N->getLocation(); + if (auto ps = pp.getAs()) { + if (ps->getStmt() == S || ps->getStmt() == Inner) + break; + } else if (auto CEE = pp.getAs()) { + if (CEE->getCalleeContext()->getCallSite() == S || + CEE->getCalleeContext()->getCallSite() == Inner) + break; + } + N = N->getFirstPred(); + } while (N); + return N; +} + +/// Find the ExplodedNode where the lvalue (the value of 'Ex') +/// was computed. +static const ExplodedNode* findNodeForExpression(const ExplodedNode *N, + const Expr *Inner) { + while (N) { + if (auto P = N->getLocation().getAs()) { + if (P->getStmt() == Inner) + break; + } + N = N->getFirstPred(); + } + assert(N && "Unable to find the lvalue node."); + return N; + +} + +/// 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. +static const Expr* peelOfOuterAddrOf(const Expr* Ex) { + Ex = Ex->IgnoreParenCasts(); + + // FIXME: There's a hack in our Store implementation that always computes + // field offsets around null pointers as if they are always equal to 0. + // The idea here is to report accesses to fields as null dereferences + // even though the pointer value that's being dereferenced is actually + // the offset of the field rather than exactly 0. + // See the FIXME in StoreManager's getLValueFieldOrIvar() method. + // 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(Ex)) + if (Op->getOpcode() == UO_AddrOf && Op->getSubExpr()->isLValue()) + if (const Expr *DerefEx = bugreporter::getDerefExpr(Op->getSubExpr())) + return DerefEx; + return Ex; + +} + bool bugreporter::trackNullOrUndefValue(const ExplodedNode *N, const Stmt *S, BugReport &report, bool IsArg, @@ -995,52 +1054,23 @@ if (!S || !N) return false; - if (const Expr *Ex = dyn_cast(S)) + if (const auto *Ex = dyn_cast(S)) S = peelOffOuterExpr(Ex, N); const Expr *Inner = nullptr; - if (const Expr *Ex = dyn_cast(S)) { + if (const auto *Ex = dyn_cast(S)) { + Ex = peelOfOuterAddrOf(Ex); Ex = Ex->IgnoreParenCasts(); - // 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: There's a hack in our Store implementation that always computes - // field offsets around null pointers as if they are always equal to 0. - // The idea here is to report accesses to fields as null dereferences - // even though the pointer value that's being dereferenced is actually - // the offset of the field rather than exactly 0. - // See the FIXME in StoreManager's getLValueFieldOrIvar() method. - // 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(Ex)) - if (Op->getOpcode() == UO_AddrOf && Op->getSubExpr()->isLValue()) - if (const Expr *DerefEx = getDerefExpr(Op->getSubExpr())) - Ex = DerefEx; - - if (Ex && (ExplodedGraph::isInterestingLValueExpr(Ex) || CallEvent::isCallStmt(Ex))) + if (Ex && (ExplodedGraph::isInterestingLValueExpr(Ex) + || CallEvent::isCallStmt(Ex))) Inner = Ex; } if (IsArg && !Inner) { assert(N->getLocation().getAs() && "Tracking arg but not at call"); } else { - // Walk through nodes until we get one that matches the statement exactly. - // Alternately, if we hit a known lvalue for the statement, we know we've - // gone too far (though we can likely track the lvalue better anyway). - do { - const ProgramPoint &pp = N->getLocation(); - if (Optional ps = pp.getAs()) { - if (ps->getStmt() == S || ps->getStmt() == Inner) - break; - } else if (Optional CEE = pp.getAs()) { - if (CEE->getCalleeContext()->getCallSite() == S || - CEE->getCalleeContext()->getCallSite() == Inner) - break; - } - N = N->getFirstPred(); - } while (N); - + N = findNodeForStatement(N, S, Inner); if (!N) return false; } @@ -1051,48 +1081,35 @@ // At this point in the path, the receiver should be live since we are at the // message send expr. If it is nil, start tracking it. if (const Expr *Receiver = NilReceiverBRVisitor::getNilReceiver(S, N)) - trackNullOrUndefValue(N, Receiver, report, false, EnableNullFPSuppression); - + trackNullOrUndefValue(N, Receiver, report, /* IsArg=*/ false, + EnableNullFPSuppression); // See if the expression we're interested refers to a variable. // If so, we can track both its contents and constraints on its value. if (Inner && ExplodedGraph::isInterestingLValueExpr(Inner)) { - const MemRegion *R = nullptr; - - // Find the ExplodedNode where the lvalue (the value of 'Ex') - // was computed. We need this for getting the location value. - const ExplodedNode *LVNode = N; - while (LVNode) { - if (Optional P = LVNode->getLocation().getAs()) { - if (P->getStmt() == Inner) - break; - } - LVNode = LVNode->getFirstPred(); - } - assert(LVNode && "Unable to find the lvalue node."); + const ExplodedNode *LVNode = findNodeForExpression(N, Inner); ProgramStateRef LVState = LVNode->getState(); SVal LVal = LVState->getSVal(Inner, LVNode->getLocationContext()); - if (LVState->isNull(LVal).isConstrainedTrue()) { - // In case of C++ references, we want to differentiate between a null - // reference and reference to null pointer. - // If the LVal is null, check if we are dealing with null reference. - // For those, we want to track the location of the reference. - if (const MemRegion *RR = getLocationRegionIfReference(Inner, N)) - R = RR; - } else { - R = LVState->getSVal(Inner, LVNode->getLocationContext()).getAsRegion(); - - // If this is a C++ reference to a null pointer, we are tracking the - // pointer. In addition, we should find the store at which the reference - // got initialized. - if (const MemRegion *RR = getLocationRegionIfReference(Inner, N)) { - if (Optional KV = LVal.getAs()) - report.addVisitor(llvm::make_unique( + const MemRegion *RR = getLocationRegionIfReference(Inner, N); + bool LVIsNull = LVState->isNull(LVal).isConstrainedTrue(); + + // If this is a C++ reference to a null pointer, we are tracking the + // pointer. In addition, we should find the store at which the reference + // got initialized. + if (RR && !LVIsNull) { + if (auto KV = LVal.getAs()) + report.addVisitor(llvm::make_unique( *KV, RR, EnableNullFPSuppression)); - } } + // In case of C++ references, we want to differentiate between a null + // reference and reference to null pointer. + // If the LVal is null, check if we are dealing with null reference. + // For those, we want to track the location of the reference. + const MemRegion *R = (RR && LVIsNull) ? RR : + LVState->getSVal(Inner, LVNode->getLocationContext()).getAsRegion(); + if (R) { // Mark both the variable region and its contents as interesting. SVal V = LVState->getRawSVal(loc::MemRegionVal(R)); @@ -1104,21 +1121,21 @@ // If the contents are symbolic, find out when they became null. if (V.getAsLocSymbol(/*IncludeBaseRegions*/ true)) report.addVisitor(llvm::make_unique( - V.castAs(), false)); + V.castAs(), false)); // Add visitor, which will suppress inline defensive checks. - if (Optional DV = V.getAs()) { + if (auto DV = V.getAs()) { if (!DV->isZeroConstant() && LVState->isNull(*DV).isConstrainedTrue() && EnableNullFPSuppression) { report.addVisitor( llvm::make_unique(*DV, - LVNode)); + LVNode)); } } - if (Optional KV = V.getAs()) + if (auto KV = V.getAs()) report.addVisitor(llvm::make_unique( - *KV, R, EnableNullFPSuppression)); + *KV, R, EnableNullFPSuppression)); return true; } } @@ -1129,7 +1146,7 @@ // If the value came from an inlined function call, we should at least make // sure that function isn't pruned in our output. - if (const Expr *E = dyn_cast(S)) + if (const auto *E = dyn_cast(S)) S = E->IgnoreParenCasts(); ReturnVisitor::addVisitorIfNecessary(N, S, report, EnableNullFPSuppression); @@ -1138,29 +1155,29 @@ // base value that was dereferenced. // assert(!V.isUnknownOrUndef()); // Is it a symbolic value? - if (Optional L = V.getAs()) { + if (auto L = V.getAs()) { + report.addVisitor(llvm::make_unique(L->getRegion())); + // At this point we are dealing with the region's LValue. // However, if the rvalue is a symbolic region, we should track it as well. // Try to use the correct type when looking up the value. SVal RVal; - if (const Expr *E = dyn_cast(S)) + if (const auto *E = dyn_cast(S)) RVal = state->getRawSVal(L.getValue(), E->getType()); else RVal = state->getSVal(L->getRegion()); - report.addVisitor(llvm::make_unique(L->getRegion())); - if (Optional KV = RVal.getAs()) + if (auto KV = RVal.getAs()) report.addVisitor(llvm::make_unique( - *KV, L->getRegion(), EnableNullFPSuppression)); + *KV, L->getRegion(), EnableNullFPSuppression)); const MemRegion *RegionRVal = RVal.getAsRegion(); if (RegionRVal && isa(RegionRVal)) { report.markInteresting(RegionRVal); report.addVisitor(llvm::make_unique( - loc::MemRegionVal(RegionRVal), false)); + loc::MemRegionVal(RegionRVal), false)); } } - return true; }