Skip to content
Permalink
Browse files

[analyzer][NFC] Prepare visitors for different tracking kinds

When we're tracking a variable that is responsible for a null pointer
dereference or some other sinister programming error, we of course would like to
gather as much information why we think that the variable has that specific
value as possible. However, the newly introduced condition tracking shows that
tracking all values this thoroughly could easily cause an intolerable growth in
the bug report's length.

There are a variety of heuristics we discussed on the mailing list[1] to combat
this, all of them requiring to differentiate in between tracking a "regular
value" and a "condition".

This patch introduces the new `bugreporter::TrackingKind` enum, adds it to
several visitors as a non-optional argument, and moves some functions around to
make the code a little more coherent.

[1] http://lists.llvm.org/pipermail/cfe-dev/2019-June/062613.html

Differential Revision: https://reviews.llvm.org/D64270

llvm-svn: 368777
  • Loading branch information
Szelethus committed Aug 14, 2019
1 parent 2a39024 commit 3f7c66d551ef1210f1f48822e6bfef2e1b97881d
@@ -85,6 +85,40 @@ class BugReporterVisitor : public llvm::FoldingSetNode {
const BugReport &BR);
};

namespace bugreporter {

/// Specifies the type of tracking for an expression.
enum class TrackingKind {
/// Default tracking kind -- specifies that as much information should be
/// gathered about the tracked expression value as possible.
Thorough,
/// Specifies that a more moderate tracking should be used for the expression
/// value. This will essentially make sure that functions relevant to the it
/// aren't pruned, but otherwise relies on the user reading the code or
/// following the arrows.
Condition
};

/// Attempts to add visitors to track expression value back to its point of
/// origin.
///
/// \param N A node "downstream" from the evaluation of the statement.
/// \param E The expression value which we are tracking
/// \param R The bug report to which visitors should be attached.
/// \param EnableNullFPSuppression Whether we should employ false positive
/// suppression (inlined defensive checks, returned null).
///
/// \return Whether or not the function was able to add visitors for this
/// statement. Note that returning \c true does not actually imply
/// that any visitors were added.
bool trackExpressionValue(const ExplodedNode *N, const Expr *E, BugReport &R,
TrackingKind TKind = TrackingKind::Thorough,
bool EnableNullFPSuppression = true);

const Expr *getDerefExpr(const Stmt *S);

} // namespace bugreporter

/// Finds last store into the given region,
/// which is different from a given symbolic value.
class FindLastStoreBRVisitor final : public BugReporterVisitor {
@@ -96,15 +130,28 @@ class FindLastStoreBRVisitor final : public BugReporterVisitor {
/// bug, we are going to employ false positive suppression.
bool EnableNullFPSuppression;

using TrackingKind = bugreporter::TrackingKind;
TrackingKind TKind;

public:
/// Creates a visitor for every VarDecl inside a Stmt and registers it with
/// the BugReport.
static void registerStatementVarDecls(BugReport &BR, const Stmt *S,
bool EnableNullFPSuppression);

bool EnableNullFPSuppression,
TrackingKind TKind);

/// \param V We're searching for the store where \c R received this value.
/// \param R The region we're tracking.
/// \param EnableNullFPSuppression Whether we should employ false positive
/// suppression (inlined defensive checks, returned null).
/// \param TKind May limit the amount of notes added to the bug report.
FindLastStoreBRVisitor(KnownSVal V, const MemRegion *R,
bool InEnableNullFPSuppression)
: R(R), V(V), EnableNullFPSuppression(InEnableNullFPSuppression) {}
bool InEnableNullFPSuppression,
TrackingKind TKind)
: R(R), V(V), EnableNullFPSuppression(InEnableNullFPSuppression),
TKind(TKind) {
assert(R);
}

void Profile(llvm::FoldingSetNodeID &ID) const override;

@@ -347,27 +394,6 @@ class TagVisitor : public BugReporterVisitor {
BugReport &R) override;
};

namespace bugreporter {

/// Attempts to add visitors to track expression value back to its point of
/// origin.
///
/// \param N A node "downstream" from the evaluation of the statement.
/// \param E The expression value which we are tracking
/// \param R The bug report to which visitors should be attached.
/// \param EnableNullFPSuppression Whether we should employ false positive
/// suppression (inlined defensive checks, returned null).
///
/// \return Whether or not the function was able to add visitors for this
/// statement. Note that returning \c true does not actually imply
/// that any visitors were added.
bool trackExpressionValue(const ExplodedNode *N, const Expr *E, BugReport &R,
bool EnableNullFPSuppression = true);

const Expr *getDerefExpr(const Stmt *S);

} // namespace bugreporter

} // namespace ento

} // namespace clang
@@ -282,7 +282,8 @@ void MIGChecker::checkReturnAux(const ReturnStmt *RS, CheckerContext &C) const {
N);

R->addRange(RS->getSourceRange());
bugreporter::trackExpressionValue(N, RS->getRetValue(), *R, false);
bugreporter::trackExpressionValue(N, RS->getRetValue(), *R,
bugreporter::TrackingKind::Thorough, false);
C.emitReport(std::move(R));
}

@@ -146,8 +146,8 @@ void ObjCContainersChecker::checkPreStmt(const CallExpr *CE,
initBugType();
auto R = llvm::make_unique<BugReport>(*BT, "Index is out of bounds", N);
R->addRange(IdxExpr->getSourceRange());
bugreporter::trackExpressionValue(N, IdxExpr, *R,
/*EnableNullFPSuppression=*/false);
bugreporter::trackExpressionValue(
N, IdxExpr, *R, bugreporter::TrackingKind::Thorough, false);
C.emitReport(std::move(R));
return;
}
@@ -87,7 +87,8 @@ UndefCapturedBlockVarChecker::checkPostStmt(const BlockExpr *BE,
if (const Expr *Ex = FindBlockDeclRefExpr(BE->getBody(), VD))
R->addRange(Ex->getSourceRange());
R->addVisitor(llvm::make_unique<FindLastStoreBRVisitor>(
*V, VR, /*EnableNullFPSuppression*/ false));
*V, VR, /*EnableNullFPSuppression*/ false,
bugreporter::TrackingKind::Thorough));
R->disablePathPruning();
// need location of block
C.emitReport(std::move(R));
@@ -851,12 +851,13 @@ class ReturnVisitor : public BugReporterVisitor {
bool EnableNullFPSuppression;
bool ShouldInvalidate = true;
AnalyzerOptions& Options;
bugreporter::TrackingKind TKind;

public:
ReturnVisitor(const StackFrameContext *Frame, bool Suppressed,
AnalyzerOptions &Options)
AnalyzerOptions &Options, bugreporter::TrackingKind TKind)
: CalleeSFC(Frame), EnableNullFPSuppression(Suppressed),
Options(Options) {}
Options(Options), TKind(TKind) {}

static void *getTag() {
static int Tag = 0;
@@ -878,7 +879,8 @@ class ReturnVisitor : public BugReporterVisitor {
/// bug report, so it can print a note later.
static void addVisitorIfNecessary(const ExplodedNode *Node, const Stmt *S,
BugReport &BR,
bool InEnableNullFPSuppression) {
bool InEnableNullFPSuppression,
bugreporter::TrackingKind TKind) {
if (!CallEvent::isCallStmt(S))
return;

@@ -951,7 +953,7 @@ class ReturnVisitor : public BugReporterVisitor {

BR.addVisitor(llvm::make_unique<ReturnVisitor>(CalleeContext,
EnableNullFPSuppression,
Options));
Options, TKind));
}

PathDiagnosticPieceRef visitNodeInitial(const ExplodedNode *N,
@@ -999,8 +1001,9 @@ class ReturnVisitor : public BugReporterVisitor {

RetE = RetE->IgnoreParenCasts();

// If we're returning 0, we should track where that 0 came from.
bugreporter::trackExpressionValue(N, RetE, BR, EnableNullFPSuppression);
// Let's track the return value.
bugreporter::trackExpressionValue(
N, RetE, BR, TKind, EnableNullFPSuppression);

// Build an appropriate message based on the return value.
SmallString<64> Msg;
@@ -1115,7 +1118,7 @@ class ReturnVisitor : public BugReporterVisitor {
if (!State->isNull(*ArgV).isConstrainedTrue())
continue;

if (bugreporter::trackExpressionValue(N, ArgE, BR, EnableNullFPSuppression))
if (trackExpressionValue(N, ArgE, BR, TKind, EnableNullFPSuppression))
ShouldInvalidate = false;

// If we /can't/ track the null pointer, we should err on the side of
@@ -1159,9 +1162,45 @@ void FindLastStoreBRVisitor::Profile(llvm::FoldingSetNodeID &ID) const {
ID.AddPointer(&tag);
ID.AddPointer(R);
ID.Add(V);
ID.AddInteger(static_cast<int>(TKind));
ID.AddBoolean(EnableNullFPSuppression);
}

void FindLastStoreBRVisitor::registerStatementVarDecls(
BugReport &BR, const Stmt *S, bool EnableNullFPSuppression,
TrackingKind TKind) {

const ExplodedNode *N = BR.getErrorNode();
std::deque<const Stmt *> WorkList;
WorkList.push_back(S);

while (!WorkList.empty()) {
const Stmt *Head = WorkList.front();
WorkList.pop_front();

ProgramStateManager &StateMgr = N->getState()->getStateManager();

if (const auto *DR = dyn_cast<DeclRefExpr>(Head)) {
if (const auto *VD = dyn_cast<VarDecl>(DR->getDecl())) {
const VarRegion *R =
StateMgr.getRegionManager().getVarRegion(VD, N->getLocationContext());

// What did we load?
SVal V = N->getSVal(S);

if (V.getAs<loc::ConcreteInt>() || V.getAs<nonloc::ConcreteInt>()) {
// Register a new visitor with the BugReport.
BR.addVisitor(llvm::make_unique<FindLastStoreBRVisitor>(
V.castAs<KnownSVal>(), R, EnableNullFPSuppression, TKind));
}
}
}

for (const Stmt *SubStmt : Head->children())
WorkList.push_back(SubStmt);
}
}

/// Returns true if \p N represents the DeclStmt declaring and initializing
/// \p VR.
static bool isInitializationOfVar(const ExplodedNode *N, const VarRegion *VR) {
@@ -1332,7 +1371,7 @@ FindLastStoreBRVisitor::VisitNode(const ExplodedNode *Succ,
// should track the initializer expression.
if (Optional<PostInitializer> PIP = Pred->getLocationAs<PostInitializer>()) {
const MemRegion *FieldReg = (const MemRegion *)PIP->getLocationValue();
if (FieldReg && FieldReg == R) {
if (FieldReg == R) {
StoreSite = Pred;
InitE = PIP->getInitializer()->getInit();
}
@@ -1397,8 +1436,8 @@ FindLastStoreBRVisitor::VisitNode(const ExplodedNode *Succ,
if (!IsParam)
InitE = InitE->IgnoreParenCasts();

bugreporter::trackExpressionValue(StoreSite, InitE, BR,
EnableNullFPSuppression);
bugreporter::trackExpressionValue(
StoreSite, InitE, BR, TKind, EnableNullFPSuppression);
}

// Okay, we've found the binding. Emit an appropriate message.
@@ -1426,7 +1465,7 @@ FindLastStoreBRVisitor::VisitNode(const ExplodedNode *Succ,
if (const VarRegion *OriginalR = BDR->getOriginalRegion(VR)) {
if (auto KV = State->getSVal(OriginalR).getAs<KnownSVal>())
BR.addVisitor(llvm::make_unique<FindLastStoreBRVisitor>(
*KV, OriginalR, EnableNullFPSuppression));
*KV, OriginalR, EnableNullFPSuppression, TKind));
}
}
}
@@ -1724,7 +1763,8 @@ PathDiagnosticPieceRef TrackControlDependencyCondBRVisitor::VisitNode(
// expression, hence the BugReport level set.
if (BR.addTrackedCondition(N)) {
bugreporter::trackExpressionValue(
N, Condition, BR, /*EnableNullFPSuppression=*/false);
N, Condition, BR, bugreporter::TrackingKind::Condition,
/*EnableNullFPSuppression=*/false);
return constructDebugPieceForTrackedCondition(Condition, N, BRC);
}
}
@@ -1838,7 +1878,9 @@ static const ExplodedNode* findNodeForExpression(const ExplodedNode *N,

bool bugreporter::trackExpressionValue(const ExplodedNode *InputNode,
const Expr *E, BugReport &report,
bugreporter::TrackingKind TKind,
bool EnableNullFPSuppression) {

if (!E || !InputNode)
return false;

@@ -1862,12 +1904,13 @@ bool bugreporter::trackExpressionValue(const ExplodedNode *InputNode,
// 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(Inner, LVNode))
trackExpressionValue(LVNode, Receiver, report, EnableNullFPSuppression);
trackExpressionValue(
LVNode, Receiver, report, TKind, EnableNullFPSuppression);

// Track the index if this is an array subscript.
if (const auto *Arr = dyn_cast<ArraySubscriptExpr>(Inner))
trackExpressionValue(
LVNode, Arr->getIdx(), report, /*EnableNullFPSuppression*/ false);
LVNode, Arr->getIdx(), report, TKind, /*EnableNullFPSuppression*/false);

// See if the expression we're interested refers to a variable.
// If so, we can track both its contents and constraints on its value.
@@ -1883,7 +1926,7 @@ bool bugreporter::trackExpressionValue(const ExplodedNode *InputNode,
if (RR && !LVIsNull)
if (auto KV = LVal.getAs<KnownSVal>())
report.addVisitor(llvm::make_unique<FindLastStoreBRVisitor>(
*KV, RR, EnableNullFPSuppression));
*KV, RR, EnableNullFPSuppression, TKind));

// In case of C++ references, we want to differentiate between a null
// reference and reference to null pointer.
@@ -1920,7 +1963,7 @@ bool bugreporter::trackExpressionValue(const ExplodedNode *InputNode,

if (auto KV = V.getAs<KnownSVal>())
report.addVisitor(llvm::make_unique<FindLastStoreBRVisitor>(
*KV, R, EnableNullFPSuppression));
*KV, R, EnableNullFPSuppression, TKind));
return true;
}
}
@@ -1930,7 +1973,7 @@ bool bugreporter::trackExpressionValue(const ExplodedNode *InputNode,
SVal V = LVState->getSValAsScalarOrLoc(Inner, LVNode->getLocationContext());

ReturnVisitor::addVisitorIfNecessary(
LVNode, Inner, report, EnableNullFPSuppression);
LVNode, Inner, report, EnableNullFPSuppression, TKind);

// Is it a symbolic value?
if (auto L = V.getAs<loc::MemRegionVal>()) {
@@ -1959,7 +2002,7 @@ bool bugreporter::trackExpressionValue(const ExplodedNode *InputNode,
if (CanDereference)
if (auto KV = RVal.getAs<KnownSVal>())
report.addVisitor(llvm::make_unique<FindLastStoreBRVisitor>(
*KV, L->getRegion(), EnableNullFPSuppression));
*KV, L->getRegion(), EnableNullFPSuppression, TKind));

const MemRegion *RegionRVal = RVal.getAsRegion();
if (RegionRVal && isa<SymbolicRegion>(RegionRVal)) {
@@ -2017,53 +2060,15 @@ PathDiagnosticPieceRef NilReceiverBRVisitor::VisitNode(const ExplodedNode *N,
// The receiver was nil, and hence the method was skipped.
// Register a BugReporterVisitor to issue a message telling us how
// the receiver was null.
bugreporter::trackExpressionValue(N, Receiver, BR,
/*EnableNullFPSuppression*/ false);
bugreporter::trackExpressionValue(
N, Receiver, BR, bugreporter::TrackingKind::Thorough,
/*EnableNullFPSuppression*/ false);
// Issue a message saying that the method was skipped.
PathDiagnosticLocation L(Receiver, BRC.getSourceManager(),
N->getLocationContext());
return std::make_shared<PathDiagnosticEventPiece>(L, OS.str());
}

//===----------------------------------------------------------------------===//
// Implementation of FindLastStoreBRVisitor.
//===----------------------------------------------------------------------===//

// Registers every VarDecl inside a Stmt with a last store visitor.
void FindLastStoreBRVisitor::registerStatementVarDecls(BugReport &BR,
const Stmt *S,
bool EnableNullFPSuppression) {
const ExplodedNode *N = BR.getErrorNode();
std::deque<const Stmt *> WorkList;
WorkList.push_back(S);

while (!WorkList.empty()) {
const Stmt *Head = WorkList.front();
WorkList.pop_front();

ProgramStateManager &StateMgr = N->getState()->getStateManager();

if (const auto *DR = dyn_cast<DeclRefExpr>(Head)) {
if (const auto *VD = dyn_cast<VarDecl>(DR->getDecl())) {
const VarRegion *R =
StateMgr.getRegionManager().getVarRegion(VD, N->getLocationContext());

// What did we load?
SVal V = N->getSVal(S);

if (V.getAs<loc::ConcreteInt>() || V.getAs<nonloc::ConcreteInt>()) {
// Register a new visitor with the BugReport.
BR.addVisitor(llvm::make_unique<FindLastStoreBRVisitor>(
V.castAs<KnownSVal>(), R, EnableNullFPSuppression));
}
}
}

for (const Stmt *SubStmt : Head->children())
WorkList.push_back(SubStmt);
}
}

//===----------------------------------------------------------------------===//
// Visitor that tries to report interesting diagnostics from conditions.
//===----------------------------------------------------------------------===//

0 comments on commit 3f7c66d

Please sign in to comment.
You can’t perform that action at this time.