Index: cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp =================================================================== --- cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp +++ cfe/trunk/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 reportBugIfPreconditionHolds(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,82 @@ return Nullability::Unspecified; } +template +static bool +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(); + if (!RegVal) + continue; + + auto ParamValue = State->getSVal(RegVal->getRegion()) + .template getAs(); + if (!ParamValue) + continue; + + if (getNullConstraint(*ParamValue, State) == NullConstraint::IsNull) { + return true; + } + } + return false; +} + +static bool checkPreconditionViolation(ProgramStateRef State, ExplodedNode *N, + CheckerContext &C) { + if (State->get()) + return true; + + const LocationContext *LocCtxt = C.getLocationContext(); + const Decl *D = LocCtxt->getDecl(); + if (!D) + return false; + + if (const auto *BlockD = dyn_cast(D)) { + if (checkParamsForPreconditionViolation(BlockD->parameters(), State, + LocCtxt)) { + if (!N->isSink()) + C.addTransition(State->set(true), N); + return true; + } + return false; + } + + if (const auto *FuncDecl = dyn_cast(D)) { + if (checkParamsForPreconditionViolation(FuncDecl->parameters(), State, + LocCtxt)) { + if (!N->isSink()) + C.addTransition(State->set(true), N); + return true; + } + return false; + } + return false; +} + +void NullabilityChecker::reportBugIfPreconditionHolds( + ErrorKind Error, ExplodedNode *N, const MemRegion *Region, + CheckerContext &C, const Stmt *ValueExpr, bool SuppressPath) const { + ProgramStateRef OriginalState = N->getState(); + + if (checkPreconditionViolation(OriginalState, N, C)) + return; + if (SuppressPath) { + OriginalState = OriginalState->set(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 +407,22 @@ State = State->remove(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 (checkPreconditionViolation(State, C.getPredecessor(), C)) + 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()) + return; + const MemRegion *Region = getTrackRegion(Event.Location, /*CheckSuperregion=*/true); if (!Region) @@ -335,6 +438,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 +463,9 @@ return; ProgramStateRef State = C.getState(); + if (State->get()) + return; + auto RetSVal = State->getSVal(S, C.getLocationContext()).getAs(); if (!RetSVal) @@ -378,9 +486,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); + reportBugIfPreconditionHolds(ErrorKind::NilReturnedToNonnull, N, nullptr, C, + RetExpr); return; } @@ -398,8 +506,8 @@ StaticNullability == Nullability::Nonnull) { static CheckerProgramPointTag Tag(this, "NullableReturnedFromNonnull"); ExplodedNode *N = C.addTransition(State, C.getPredecessor(), &Tag); - reportBug(ErrorKind::NullableReturnedToNonnull, N, Region, - C.getBugReporter()); + reportBugIfPreconditionHolds(ErrorKind::NullableReturnedToNonnull, N, + Region, C); } return; } @@ -418,6 +526,9 @@ return; ProgramStateRef State = C.getState(); + if (State->get()) + return; + ProgramStateRef OrigState = State; unsigned Idx = 0; @@ -445,10 +556,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); + reportBugIfPreconditionHolds(ErrorKind::NilPassedToNonnull, N, nullptr, C, + ArgExpr); return; } @@ -466,18 +576,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); + reportBugIfPreconditionHolds(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); + reportBugIfPreconditionHolds(ErrorKind::NullableDereferenced, N, Region, + C, ArgExpr, /*SuppressPath=*/true); return; } continue; @@ -507,10 +615,13 @@ QualType ReturnType = FuncType->getReturnType(); if (!ReturnType->isAnyPointerType()) return; + ProgramStateRef State = C.getState(); + if (State->get()) + 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 +687,14 @@ if (!RetType->isAnyPointerType()) return; + ProgramStateRef State = C.getState(); + if (State->get()) + 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,6 +802,10 @@ if (!DestType->isAnyPointerType()) return; + ProgramStateRef State = C.getState(); + if (State->get()) + return; + Nullability DestNullability = getNullabilityAnnotation(DestType); // No explicit nullability in the destination type, so this cast does not @@ -695,7 +813,6 @@ if (DestNullability == Nullability::Unspecified) return; - ProgramStateRef State = C.getState(); auto RegionSVal = State->getSVal(CE, C.getLocationContext()).getAs(); const MemRegion *Region = getTrackRegion(*RegionSVal); @@ -744,11 +861,14 @@ if (!LocType->isAnyPointerType()) return; + ProgramStateRef State = C.getState(); + if (State->get()) + return; + auto ValDefOrUnknown = V.getAs(); if (!ValDefOrUnknown) return; - ProgramStateRef State = C.getState(); NullConstraint RhsNullness = getNullConstraint(*ValDefOrUnknown, State); Nullability ValNullability = Nullability::Unspecified; @@ -761,9 +881,9 @@ 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); + reportBugIfPreconditionHolds(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()); + reportBugIfPreconditionHolds(ErrorKind::NullableAssignedToNonnull, N, + ValueRegion, C); } return; } Index: cfe/trunk/test/Analysis/nullability.mm =================================================================== --- cfe/trunk/test/Analysis/nullability.mm +++ cfe/trunk/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; +}