xazax.hun updated this revision to Diff 33704.
xazax.hun added a comment.
Made sure that inlined defensive checks do not generate false positives.
http://reviews.llvm.org/D12445
Files:
lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
test/Analysis/nullability.mm
Index: test/Analysis/nullability.mm
===================================================================
--- test/Analysis/nullability.mm
+++ test/Analysis/nullability.mm
@@ -179,3 +179,65 @@
takesNullable(p);
takesNonnull(p);
}
+
+void onlyReportFirstPreconditionViolationOnPath() {
+ Dummy *p = returnsNullable();
+ takesNonnull(p); // expected-warning {{}}
+ takesNonnull(p); // No warning.
+ // The first warning was not a sink. The analysis expected to continue.
+ int i = 0;
+ i = 5 / i; // expected-warning {{Division by zero}}
+ (void)i;
+}
+
+Dummy *_Nonnull doNotWarnWhenPreconditionIsViolatedInTopFunc(
+ Dummy *_Nonnull p) {
+ if (!p) {
+ Dummy *ret =
+ 0; // avoid compiler warning (which is not generated by the analyzer)
+ if (getRandom())
+ return ret; // no warning
+ else
+ return p; // no warning
+ } else {
+ return p;
+ }
+}
+
+Dummy *_Nonnull doNotWarnWhenPreconditionIsViolated(Dummy *_Nonnull p) {
+ if (!p) {
+ Dummy *ret =
+ 0; // avoid compiler warning (which is not generated by the analyzer)
+ if (getRandom())
+ return ret; // no warning
+ else
+ return p; // no warning
+ } else {
+ return p;
+ }
+}
+
+void testPreconditionViolationInInlinedFunction(Dummy *p) {
+ doNotWarnWhenPreconditionIsViolated(p);
+}
+
+void inlinedNullable(Dummy *_Nullable p) {
+ if (p) return;
+}
+void inlinedNonnull(Dummy *_Nonnull p) {
+ if (p) return;
+}
+void inlinedUnspecified(Dummy *p) {
+ if (p) return;
+}
+
+Dummy *_Nonnull testDefensiveInlineChecks(Dummy * p) {
+ switch (getRandom()) {
+ case 1: inlinedNullable(p); break;
+ case 2: inlinedNonnull(p); break;
+ case 3: inlinedUnspecified(p); break;
+ }
+ if (getRandom())
+ takesNonnull(p);
+ return p;
+}
Index: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -161,6 +161,16 @@
const MemRegion *Region;
};
+ /// When any of the nonnull arguments of the analyzed function is null, do not
+ /// report anything and turn off the check.
+ ///
+ /// When \p SuppressPath is set to true, no more bugs will be reported on this
+ /// path by this checker.
+ void reportBugConditionally(ErrorKind Error, ExplodedNode *N,
+ const MemRegion *Region, CheckerContext &C,
+ const Stmt *ValueExpr = nullptr,
+ bool SuppressPath = false) const;
+
void reportBug(ErrorKind Error, ExplodedNode *N, const MemRegion *Region,
BugReporter &BR, const Stmt *ValueExpr = nullptr) const {
if (!BT)
@@ -220,6 +230,13 @@
REGISTER_MAP_WITH_PROGRAMSTATE(NullabilityMap, const MemRegion *,
NullabilityState)
+// If the nullability precondition of a function is violated, we should not
+// report nullability related issues on that path. For this reason once a
+// precondition is not met on a path, this checker will be esentially turned off
+// for the rest of the analysis. We do not want to generate a sink node however,
+// so this checker would not lead to reduced coverage.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(PreconditionViolated, bool)
+
enum class NullConstraint { IsNull, IsNotNull, Unknown };
static NullConstraint getNullConstraint(DefinedOrUnknownSVal Val,
@@ -302,6 +319,76 @@
return Nullability::Unspecified;
}
+template <typename ParamVarDeclRange>
+static ProgramStateRef
+checkParamsForPreconditionViolation(const ParamVarDeclRange &Params,
+ ProgramStateRef State,
+ const LocationContext *LocCtxt) {
+ for (const auto *ParamDecl : Params) {
+ if (ParamDecl->isParameterPack())
+ break;
+
+ if (getNullabilityAnnotation(ParamDecl->getType()) != Nullability::Nonnull)
+ continue;
+
+ auto RegVal = State->getLValue(ParamDecl, LocCtxt)
+ .template getAs<loc::MemRegionVal>();
+ if (!RegVal)
+ continue;
+
+ auto ParamValue = State->getSVal(RegVal->getRegion())
+ .template getAs<DefinedOrUnknownSVal>();
+ if (!ParamValue)
+ continue;
+
+ if (getNullConstraint(*ParamValue, State) == NullConstraint::IsNull) {
+ return State->set<PreconditionViolated>(true);
+ }
+ }
+ return State;
+}
+
+static ProgramStateRef
+checkPreconditionViolation(ProgramStateRef State,
+ const LocationContext *LocCtxt) {
+ const Decl *D = LocCtxt->getDecl();
+ if (!D)
+ return State;
+
+ if (const auto *BlockD = dyn_cast<BlockDecl>(D)) {
+ return checkParamsForPreconditionViolation(BlockD->parameters(), State,
+ LocCtxt);
+ }
+
+ if (const auto *FuncDecl = dyn_cast<FunctionDecl>(D)) {
+ return checkParamsForPreconditionViolation(FuncDecl->parameters(), State,
+ LocCtxt);
+ }
+ return State;
+}
+
+void NullabilityChecker::reportBugConditionally(
+ ErrorKind Error, ExplodedNode *N, const MemRegion *Region,
+ CheckerContext &C, const Stmt *ValueExpr, bool SuppressPath) const {
+ ProgramStateRef OriginalState = N->getState();
+ if (OriginalState->get<PreconditionViolated>())
+ return;
+
+ ProgramStateRef NewState =
+ checkPreconditionViolation(OriginalState, C.getLocationContext());
+ if (NewState != OriginalState) {
+ if (!N->isSink())
+ C.addTransition(NewState, N);
+ return;
+ }
+ if (SuppressPath) {
+ OriginalState = OriginalState->set<PreconditionViolated>(true);
+ N = C.addTransition(OriginalState, N);
+ }
+
+ reportBug(Error, N, Region, C.getBugReporter(), ValueExpr);
+}
+
/// Cleaning up the program state.
void NullabilityChecker::checkDeadSymbols(SymbolReaper &SR,
CheckerContext &C) const {
@@ -314,12 +401,29 @@
State = State->remove<NullabilityMap>(I->first);
}
}
+ // When one of the nonnull arguments are constrained to be null, nullability
+ // preconditions are violated. It is not enough to check this only when we
+ // actually report an error, because at that time interesting symbols might be
+ // reaped.
+ if (!State->get<PreconditionViolated>()) {
+ ProgramStateRef NewState =
+ checkPreconditionViolation(State, C.getLocationContext());
+ if (NewState != State) {
+ NewState = NewState->set<PreconditionViolated>(true);
+ C.addTransition(NewState);
+ return;
+ }
+ }
+ C.addTransition(State);
}
/// This callback triggers when a pointer is dereferenced and the analyzer does
/// not know anything about the value of that pointer. When that pointer is
/// nullable, this code emits a warning.
void NullabilityChecker::checkEvent(ImplicitNullDerefEvent Event) const {
+ if (Event.SinkNode->getState()->get<PreconditionViolated>())
+ return;
+
const MemRegion *Region =
getTrackRegion(Event.Location, /*CheckSuperregion=*/true);
if (!Region)
@@ -335,6 +439,8 @@
if (Filter.CheckNullableDereferenced &&
TrackedNullability->getValue() == Nullability::Nullable) {
BugReporter &BR = *Event.BR;
+ // Do not suppress errors on defensive code paths, because dereferencing
+ // a nullable pointer is always an error.
if (Event.IsDirectDereference)
reportBug(ErrorKind::NullableDereferenced, Event.SinkNode, Region, BR);
else
@@ -358,6 +464,9 @@
return;
ProgramStateRef State = C.getState();
+ if (State->get<PreconditionViolated>())
+ return;
+
auto RetSVal =
State->getSVal(S, C.getLocationContext()).getAs<DefinedOrUnknownSVal>();
if (!RetSVal)
@@ -378,9 +487,9 @@
Nullness == NullConstraint::IsNull &&
StaticNullability == Nullability::Nonnull) {
static CheckerProgramPointTag Tag(this, "NullReturnedFromNonnull");
- ExplodedNode *N = C.addTransition(State, C.getPredecessor(), &Tag);
- reportBug(ErrorKind::NilReturnedToNonnull, N, nullptr, C.getBugReporter(),
- S);
+ ExplodedNode *N = C.generateSink(State, C.getPredecessor(), &Tag);
+ reportBugConditionally(ErrorKind::NilReturnedToNonnull, N, nullptr, C,
+ RetExpr);
return;
}
@@ -398,8 +507,8 @@
StaticNullability == Nullability::Nonnull) {
static CheckerProgramPointTag Tag(this, "NullableReturnedFromNonnull");
ExplodedNode *N = C.addTransition(State, C.getPredecessor(), &Tag);
- reportBug(ErrorKind::NullableReturnedToNonnull, N, Region,
- C.getBugReporter());
+ reportBugConditionally(ErrorKind::NullableReturnedToNonnull, N, Region,
+ C);
}
return;
}
@@ -418,6 +527,9 @@
return;
ProgramStateRef State = C.getState();
+ if (State->get<PreconditionViolated>())
+ return;
+
ProgramStateRef OrigState = State;
unsigned Idx = 0;
@@ -445,10 +557,9 @@
if (Filter.CheckNullPassedToNonnull && Nullness == NullConstraint::IsNull &&
ArgStaticNullability != Nullability::Nonnull &&
ParamNullability == Nullability::Nonnull) {
- static CheckerProgramPointTag Tag(this, "NullPassedToNonnull");
- ExplodedNode *N = C.generateSink(State, C.getPredecessor(), &Tag);
- reportBug(ErrorKind::NilPassedToNonnull, N, nullptr, C.getBugReporter(),
- ArgExpr);
+ ExplodedNode *N = C.generateSink(State);
+ reportBugConditionally(ErrorKind::NilPassedToNonnull, N, nullptr, C,
+ ArgExpr);
return;
}
@@ -466,18 +577,16 @@
if (Filter.CheckNullablePassedToNonnull &&
ParamNullability == Nullability::Nonnull) {
- static CheckerProgramPointTag Tag(this, "NullablePassedToNonnull");
- ExplodedNode *N = C.generateSink(State, C.getPredecessor(), &Tag);
- reportBug(ErrorKind::NullablePassedToNonnull, N, Region,
- C.getBugReporter(), ArgExpr);
+ ExplodedNode *N = C.addTransition(State);
+ reportBugConditionally(ErrorKind::NullablePassedToNonnull, N, Region, C,
+ ArgExpr, /*SuppressPath=*/true);
return;
}
if (Filter.CheckNullableDereferenced &&
Param->getType()->isReferenceType()) {
- static CheckerProgramPointTag Tag(this, "NullableDereferenced");
- ExplodedNode *N = C.generateSink(State, C.getPredecessor(), &Tag);
- reportBug(ErrorKind::NullableDereferenced, N, Region,
- C.getBugReporter(), ArgExpr);
+ ExplodedNode *N = C.addTransition(State);
+ reportBugConditionally(ErrorKind::NullableDereferenced, N, Region, C,
+ ArgExpr, /*SuppressPath=*/true);
return;
}
continue;
@@ -507,10 +616,13 @@
QualType ReturnType = FuncType->getReturnType();
if (!ReturnType->isAnyPointerType())
return;
+ ProgramStateRef State = C.getState();
+ if (State->get<PreconditionViolated>())
+ return;
+
const MemRegion *Region = getTrackRegion(Call.getReturnValue());
if (!Region)
return;
- ProgramStateRef State = C.getState();
// CG headers are misannotated. Do not warn for symbols that are the results
// of CG calls.
@@ -576,11 +688,14 @@
if (!RetType->isAnyPointerType())
return;
+ ProgramStateRef State = C.getState();
+ if (State->get<PreconditionViolated>())
+ return;
+
const MemRegion *ReturnRegion = getTrackRegion(M.getReturnValue());
if (!ReturnRegion)
return;
- ProgramStateRef State = C.getState();
auto Interface = Decl->getClassInterface();
auto Name = Interface ? Interface->getName() : "";
// In order to reduce the noise in the diagnostics generated by this checker,
@@ -688,14 +803,17 @@
if (!DestType->isAnyPointerType())
return;
+ ProgramStateRef State = C.getState();
+ if (State->get<PreconditionViolated>())
+ return;
+
Nullability DestNullability = getNullabilityAnnotation(DestType);
// No explicit nullability in the destination type, so this cast does not
// change the nullability.
if (DestNullability == Nullability::Unspecified)
return;
- ProgramStateRef State = C.getState();
auto RegionSVal =
State->getSVal(CE, C.getLocationContext()).getAs<DefinedOrUnknownSVal>();
const MemRegion *Region = getTrackRegion(*RegionSVal);
@@ -744,11 +862,14 @@
if (!LocType->isAnyPointerType())
return;
+ ProgramStateRef State = C.getState();
+ if (State->get<PreconditionViolated>())
+ return;
+
auto ValDefOrUnknown = V.getAs<DefinedOrUnknownSVal>();
if (!ValDefOrUnknown)
return;
- ProgramStateRef State = C.getState();
NullConstraint RhsNullness = getNullConstraint(*ValDefOrUnknown, State);
Nullability ValNullability = Nullability::Unspecified;
@@ -761,9 +882,8 @@
ValNullability != Nullability::Nonnull &&
LocNullability == Nullability::Nonnull) {
static CheckerProgramPointTag Tag(this, "NullPassedToNonnull");
- ExplodedNode *N = C.addTransition(State, C.getPredecessor(), &Tag);
- reportBug(ErrorKind::NilAssignedToNonnull, N, nullptr, C.getBugReporter(),
- S);
+ ExplodedNode *N = C.generateSink(State, C.getPredecessor(), &Tag);
+ reportBugConditionally(ErrorKind::NilAssignedToNonnull, N, nullptr, C, S);
return;
}
// Intentionally missing case: '0' is bound to a reference. It is handled by
@@ -784,8 +904,8 @@
LocNullability == Nullability::Nonnull) {
static CheckerProgramPointTag Tag(this, "NullablePassedToNonnull");
ExplodedNode *N = C.addTransition(State, C.getPredecessor(), &Tag);
- reportBug(ErrorKind::NullableAssignedToNonnull, N, ValueRegion,
- C.getBugReporter());
+ reportBugConditionally(ErrorKind::NullableAssignedToNonnull, N,
+ ValueRegion, C);
}
return;
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
