Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp =================================================================== --- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -1516,6 +1516,8 @@ return nullptr; } +/// \return A subexpression of {@code Ex} which represents the +/// expression-of-interest. static const Expr *peelOffOuterExpr(const Expr *Ex, const ExplodedNode *N) { Ex = Ex->IgnoreParenCasts(); @@ -1560,125 +1562,70 @@ if (const Expr *SubEx = peelOffPointerArithmetic(BO)) return peelOffOuterExpr(SubEx, N); - if (auto *UO = dyn_cast(Ex)) + if (auto *UO = dyn_cast(Ex)) { if (UO->getOpcode() == UO_LNot) return peelOffOuterExpr(UO->getSubExpr(), N); + if (UO->getOpcode() == UO_AddrOf && UO->getSubExpr()->isLValue()) + if (const Expr *DerefEx = bugreporter::getDerefExpr(UO->getSubExpr())) + return peelOffOuterExpr(DerefEx, N); + } 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) { + const Expr *Inner) { while (N) { - if (auto P = N->getLocation().getAs()) { - if (P->getStmt() == Inner) - break; - } + if (PathDiagnosticLocation::getStmt(N) == Inner) + return N; 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, +bool bugreporter::trackNullOrUndefValue(const ExplodedNode *InputNode, + const Stmt *InputS, BugReport &report, bool IsArg, bool EnableNullFPSuppression) { - if (!S || !N) + if (!InputS || !InputNode) return false; - if (const auto *Ex = dyn_cast(S)) - S = peelOffOuterExpr(Ex, N); - const Expr *Inner = nullptr; - if (const auto *Ex = dyn_cast(S)) { - Ex = peelOfOuterAddrOf(Ex); - Ex = Ex->IgnoreParenCasts(); - - if (Ex && (ExplodedGraph::isInterestingLValueExpr(Ex) - || CallEvent::isCallStmt(Ex))) - Inner = Ex; - } + if (const auto *Ex = dyn_cast(InputS)) + Inner = peelOffOuterExpr(Ex, InputNode); - if (IsArg && !Inner) { - assert(N->getLocation().getAs() && "Tracking arg but not at call"); - } else { - N = findNodeForStatement(N, S, Inner); - if (!N) - return false; - } + const ExplodedNode *LVNode = findNodeForExpression(InputNode, Inner); + if (!LVNode) + return false; - ProgramStateRef state = N->getState(); + ProgramStateRef state = LVNode->getState(); // The message send could be nil due to the receiver being nil. // 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, /* IsArg=*/ false, + if (const Expr *Receiver = NilReceiverBRVisitor::getNilReceiver(Inner, LVNode)) + trackNullOrUndefValue(LVNode, 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 ExplodedNode *LVNode = findNodeForExpression(N, Inner); + if (Inner && LVNode && ExplodedGraph::isInterestingLValueExpr(Inner)) { ProgramStateRef LVState = LVNode->getState(); SVal LVal = LVNode->getSVal(Inner); - const MemRegion *RR = getLocationRegionIfReference(Inner, N); + const MemRegion *RR = getLocationRegionIfReference(Inner, LVNode); 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 (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. @@ -1688,7 +1635,6 @@ LVNode->getSVal(Inner).getAsRegion(); if (R) { - ProgramStateRef S = N->getState(); // Mark both the variable region and its contents as interesting. SVal V = LVState->getRawSVal(loc::MemRegionVal(R)); @@ -1696,9 +1642,8 @@ llvm::make_unique(cast(R))); MacroNullReturnSuppressionVisitor::addMacroVisitorIfNecessary( - N, R, EnableNullFPSuppression, report, V); + LVNode, R, EnableNullFPSuppression, report, V); - report.markInteresting(R); report.markInteresting(V); report.addVisitor(llvm::make_unique(R)); @@ -1708,14 +1653,12 @@ V.castAs(), false)); // Add visitor, which will suppress inline defensive checks. - if (auto DV = V.getAs()) { + if (auto DV = V.getAs()) if (!DV->isZeroConstant() && LVState->isNull(*DV).isConstrainedTrue() && - EnableNullFPSuppression) { + EnableNullFPSuppression) report.addVisitor( llvm::make_unique(*DV, - LVNode)); - } - } + LVNode)); if (auto KV = V.getAs()) report.addVisitor(llvm::make_unique( @@ -1726,41 +1669,35 @@ // If the expression is not an "lvalue expression", we can still // track the constraints on its contents. - SVal V = state->getSValAsScalarOrLoc(S, N->getLocationContext()); + SVal V = state->getSValAsScalarOrLoc(Inner, LVNode->getLocationContext()); - // 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 auto *E = dyn_cast(S)) - S = E->IgnoreParenCasts(); + ReturnVisitor::addVisitorIfNecessary( + LVNode, Inner, report, EnableNullFPSuppression); - ReturnVisitor::addVisitorIfNecessary(N, S, report, EnableNullFPSuppression); - - // Uncomment this to find cases where we aren't properly getting the - // base value that was dereferenced. - // assert(!V.isUnknownOrUndef()); // Is it a symbolic value? 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 auto *E = dyn_cast(S)) - RVal = state->getRawSVal(L.getValue(), E->getType()); - else - RVal = state->getSVal(L->getRegion()); - // FIXME: this is a hack for fixing a later crash when attempting to // dereference a void* pointer. // We should not try to dereference pointers at all when we don't care // what is written inside the pointer. - bool ShouldFindLastStore = true; + bool CanDereference = true; if (const auto *SR = dyn_cast(L->getRegion())) if (SR->getSymbol()->getType()->getPointeeType()->isVoidType()) - ShouldFindLastStore = false; + CanDereference = false; + + // 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 (ExplodedGraph::isInterestingLValueExpr(Inner)) { + RVal = state->getRawSVal(L.getValue(), Inner->getType()); + } else if (CanDereference) { + RVal = state->getSVal(L->getRegion()); + } - if (ShouldFindLastStore) + if (CanDereference) if (auto KV = RVal.getAs()) report.addVisitor(llvm::make_unique( *KV, L->getRegion(), EnableNullFPSuppression));