Index: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp =================================================================== --- lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp +++ lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp @@ -161,6 +161,11 @@ const MemRegion *Region; }; + // This overload of report bug reports the bugs conditionally. When a + // nullability precondition is violated it will not report any bugs at all. + void reportBug(ErrorKind Error, ExplodedNode *N, const MemRegion *Region, + CheckerContext &C, const Stmt *ValueExpr = nullptr) const; + void reportBug(ErrorKind Error, ExplodedNode *N, const MemRegion *Region, BugReporter &BR, const Stmt *ValueExpr = nullptr) const { if (!BT) @@ -220,6 +225,13 @@ REGISTER_MAP_WITH_PROGRAMSTATE(NullabilityMap, const MemRegion *, NullabilityState) +// If the nullability preconditions of a function is violated, we should not +// report that, a postcondition is violated. For this reason once a precondition +// is not met on a path, this checker will be esentially turned of for the rest +// of the analysis. We do not want to generate a sink node however, so this +// check does not reduce the coverage. +REGISTER_TRAIT_WITH_PROGRAMSTATE(PreconditionViolated, bool) + enum class NullConstraint { IsNull, IsNotNull, Unknown }; static NullConstraint getNullConstraint(DefinedOrUnknownSVal Val, @@ -302,6 +314,68 @@ return Nullability::Unspecified; } +template +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(); + if (!RegVal) + continue; + + auto ParamValue = State->getSVal(RegVal->getRegion()) + .template getAs(); + if (!ParamValue) + continue; + + if (getNullConstraint(*ParamValue, State) == NullConstraint::IsNull) { + return State->set(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(D)) { + return checkParamsForPreconditionViolation(BlockD->parameters(), State, + LocCtxt); + } + + if (const auto *FuncDecl = dyn_cast(D)) { + return checkParamsForPreconditionViolation(FuncDecl->parameters(), State, + LocCtxt); + } + return State; +} + +void NullabilityChecker::reportBug(ErrorKind Error, ExplodedNode *N, + const MemRegion *Region, CheckerContext &C, + const Stmt *ValueExpr) const { + ProgramStateRef OriginalState = N->getState(); + ProgramStateRef NewState = + checkPreconditionViolation(OriginalState, C.getLocationContext()); + if (NewState != OriginalState) { + C.addTransition(NewState, N); + return; + } + + reportBug(Error, N, Region, C.getBugReporter(), ValueExpr); +} + /// Cleaning up the program state. void NullabilityChecker::checkDeadSymbols(SymbolReaper &SR, CheckerContext &C) const { @@ -314,12 +388,29 @@ 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 (!State->get()) { + ProgramStateRef NewState = + checkPreconditionViolation(State, C.getLocationContext()); + if (NewState != State) { + NewState = NewState->set(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()) + return; + const MemRegion *Region = getTrackRegion(Event.Location, /*CheckSuperregion=*/true); if (!Region) @@ -335,6 +426,8 @@ if (Filter.CheckNullableDereferenced && TrackedNullability->getValue() == Nullability::Nullable) { BugReporter &BR = *Event.BR; + // Do not suppress errors on defensive code paths, because dereferencing + // null is alwazs an error. if (Event.IsDirectDereference) reportBug(ErrorKind::NullableDereferenced, Event.SinkNode, Region, BR); else @@ -358,6 +451,9 @@ return; ProgramStateRef State = C.getState(); + if (State->get()) + return; + auto RetSVal = State->getSVal(S, C.getLocationContext()).getAs(); if (!RetSVal) @@ -379,8 +475,7 @@ StaticNullability == Nullability::Nonnull) { static CheckerProgramPointTag Tag(this, "NullReturnedFromNonnull"); ExplodedNode *N = C.addTransition(State, C.getPredecessor(), &Tag); - reportBug(ErrorKind::NilReturnedToNonnull, N, nullptr, C.getBugReporter(), - S); + reportBug(ErrorKind::NilReturnedToNonnull, N, nullptr, C, S); return; } @@ -398,8 +493,7 @@ StaticNullability == Nullability::Nonnull) { static CheckerProgramPointTag Tag(this, "NullableReturnedFromNonnull"); ExplodedNode *N = C.addTransition(State, C.getPredecessor(), &Tag); - reportBug(ErrorKind::NullableReturnedToNonnull, N, Region, - C.getBugReporter()); + reportBug(ErrorKind::NullableReturnedToNonnull, N, Region, C); } return; } @@ -418,6 +512,9 @@ return; ProgramStateRef State = C.getState(); + if (State->get()) + return; + ProgramStateRef OrigState = State; unsigned Idx = 0; @@ -445,10 +542,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); + State = State->set(true); + ExplodedNode *N = C.addTransition(State); + reportBug(ErrorKind::NilPassedToNonnull, N, nullptr, C, ArgExpr); return; } @@ -466,18 +562,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); + State = State->set(true); + ExplodedNode *N = C.addTransition(State); + reportBug(ErrorKind::NullablePassedToNonnull, N, Region, C, ArgExpr); 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); + State = State->set(true); + ExplodedNode *N = C.addTransition(State); + reportBug(ErrorKind::NullableDereferenced, N, Region, C, ArgExpr); return; } continue; @@ -507,10 +601,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 +673,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 +788,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 +799,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 +847,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; @@ -762,8 +868,7 @@ LocNullability == Nullability::Nonnull) { static CheckerProgramPointTag Tag(this, "NullPassedToNonnull"); ExplodedNode *N = C.addTransition(State, C.getPredecessor(), &Tag); - reportBug(ErrorKind::NilAssignedToNonnull, N, nullptr, C.getBugReporter(), - S); + reportBug(ErrorKind::NilAssignedToNonnull, N, nullptr, C, S); return; } // Intentionally missing case: '0' is bound to a reference. It is handled by @@ -784,8 +889,7 @@ LocNullability == Nullability::Nonnull) { static CheckerProgramPointTag Tag(this, "NullablePassedToNonnull"); ExplodedNode *N = C.addTransition(State, C.getPredecessor(), &Tag); - reportBug(ErrorKind::NullableAssignedToNonnull, N, ValueRegion, - C.getBugReporter()); + reportBug(ErrorKind::NullableAssignedToNonnull, N, ValueRegion, C); } return; } Index: test/Analysis/nullability.mm =================================================================== --- test/Analysis/nullability.mm +++ test/Analysis/nullability.mm @@ -179,3 +179,44 @@ 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); +}