diff --git a/clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp --- a/clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp @@ -14,8 +14,9 @@ // //===----------------------------------------------------------------------===// -#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/AST/Attr.h" +#include "clang/Analysis/AnyCall.h" +#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" #include "clang/StaticAnalyzer/Core/CheckerManager.h" @@ -28,44 +29,78 @@ namespace { class NonNullParamChecker - : public Checker< check::PreCall, EventDispatcher > { + : public Checker> { mutable std::unique_ptr BTAttrNonNull; mutable std::unique_ptr BTNullRefArg; public: - void checkPreCall(const CallEvent &Call, CheckerContext &C) const; + void checkBeginFunction(CheckerContext &C) const; std::unique_ptr - genReportNullAttrNonNull(const ExplodedNode *ErrorN, - const Expr *ArgE, + genReportNullAttrNonNull(const ExplodedNode *ErrorN, const Expr *ArgE, unsigned IdxOfArg) const; std::unique_ptr genReportReferenceToNullPointer(const ExplodedNode *ErrorN, const Expr *ArgE) const; }; -} // end anonymous namespace -/// \return Bitvector marking non-null attributes. -static llvm::SmallBitVector getNonNullAttrs(const CallEvent &Call) { +template +void setBitsAccordingToFunctionAttributes(const CallType &Call, + llvm::SmallBitVector &AttrNonNull) { const Decl *FD = Call.getDecl(); - unsigned NumArgs = Call.getNumArgs(); - llvm::SmallBitVector AttrNonNull(NumArgs); + for (const auto *NonNull : FD->specific_attrs()) { if (!NonNull->args_size()) { - AttrNonNull.set(0, NumArgs); + // Lack of attribute parameters means that all of the parameters are + // implicitly marked as non-null. + AttrNonNull.set(); break; } + for (const ParamIdx &Idx : NonNull->args()) { + // 'nonnull' attribute's parameters are 1-based and should be adjusted to + // match actual AST parameter/argument indices. unsigned IdxAST = Idx.getASTIndex(); - if (IdxAST >= NumArgs) + if (IdxAST >= AttrNonNull.size()) continue; AttrNonNull.set(IdxAST); } } +} + +template +void setBitsAccordingToParameterAttributes(const CallType &Call, + llvm::SmallBitVector &AttrNonNull) { + for (const ParmVarDecl *Parameter : Call.parameters()) { + if (Parameter->hasAttr()) + AttrNonNull.set(Parameter->getFunctionScopeIndex()); + } +} + +template +llvm::SmallBitVector getNonNullAttrsImpl(const CallType &Call, + unsigned ExpectedSize) { + llvm::SmallBitVector AttrNonNull(ExpectedSize); + + setBitsAccordingToFunctionAttributes(Call, AttrNonNull); + setBitsAccordingToParameterAttributes(Call, AttrNonNull); + return AttrNonNull; } +/// \return Bitvector marking non-null attributes. +llvm::SmallBitVector getNonNullAttrs(const CallEvent &Call) { + return getNonNullAttrsImpl(Call, Call.getNumArgs()); +} + +/// \return Bitvector marking non-null attributes. +llvm::SmallBitVector getNonNullAttrs(const AnyCall &Call) { + return getNonNullAttrsImpl(Call, Call.param_size()); +} +} // end anonymous namespace + void NonNullParamChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const { if (!Call.getDecl()) @@ -75,7 +110,7 @@ unsigned NumArgs = Call.getNumArgs(); ProgramStateRef state = C.getState(); - ArrayRef parms = Call.parameters(); + ArrayRef parms = Call.parameters(); for (unsigned idx = 0; idx < NumArgs; ++idx) { // For vararg functions, a corresponding parameter decl may not exist. @@ -83,15 +118,11 @@ // Check if the parameter is a reference. We want to report when reference // to a null pointer is passed as a parameter. - bool haveRefTypeParam = + bool HasRefTypeParam = HasParam ? parms[idx]->getType()->isReferenceType() : false; - bool haveAttrNonNull = AttrNonNull[idx]; - - // Check if the parameter is also marked 'nonnull'. - if (!haveAttrNonNull && HasParam) - haveAttrNonNull = parms[idx]->hasAttr(); + bool ExpectedToBeNonNull = AttrNonNull.test(idx); - if (!haveAttrNonNull && !haveRefTypeParam) + if (!ExpectedToBeNonNull && !HasRefTypeParam) continue; // If the value is unknown or undefined, we can't perform this check. @@ -101,10 +132,10 @@ if (!DV) continue; - assert(!haveRefTypeParam || DV->getAs()); + assert(!HasRefTypeParam || DV->getAs()); // Process the case when the argument is not a location. - if (haveAttrNonNull && !DV->getAs()) { + if (ExpectedToBeNonNull && !DV->getAs()) { // If the argument is a union type, we want to handle a potential // transparent_union GCC extension. if (!ArgE) @@ -145,9 +176,9 @@ if (ExplodedNode *errorNode = C.generateErrorNode(stateNull)) { std::unique_ptr R; - if (haveAttrNonNull) + if (ExpectedToBeNonNull) R = genReportNullAttrNonNull(errorNode, ArgE, idx + 1); - else if (haveRefTypeParam) + else if (HasRefTypeParam) R = genReportReferenceToNullPointer(errorNode, ArgE); // Highlight the range of the argument that was null. @@ -180,6 +211,54 @@ C.addTransition(state); } +/// We want to trust developer annotations and consider all 'nonnul' parameters +/// as non-null indeed. Each marked parameter will get a corresponding +/// constraint. +/// +/// This approach will not only help us to get rid of some false positives, but +/// remove duplicates and shorten warning traces as well. +/// +/// \code +/// void foo(int *x) [[gnu::nonnull]] { +/// // . . . +/// *x = 42; // we don't want to consider this as an error... +/// // . . . +/// } +/// +/// foo(nullptr); // ...and report here instead +/// \endcode +void NonNullParamChecker::checkBeginFunction(CheckerContext &Context) const { + const LocationContext *LocContext = Context.getLocationContext(); + + const Decl *FD = LocContext->getDecl(); + // AnyCall helps us here to avoid checking for FunctionDecl and ObjCMethodDecl + // separately and aggregates interfaces of these classes. + auto AbstractCall = AnyCall::forDecl(FD); + if (!AbstractCall) + return; + + ProgramStateRef State = Context.getState(); + llvm::SmallBitVector ParameterNonNullMarks = getNonNullAttrs(*AbstractCall); + + for (const ParmVarDecl *Parameter : AbstractCall->parameters()) { + // 1. Check parameter if it is annotated as non-null + if (!ParameterNonNullMarks.test(Parameter->getFunctionScopeIndex())) + continue; + + Loc ParameterLoc = State->getLValue(Parameter, LocContext); + auto StoredVal = State->getSVal(ParameterLoc).getAs(); + if (!StoredVal) + continue; + + // 2. Assume that it is indeed non-null + if (ProgramStateRef NewState = State->assume(*StoredVal, true)) { + State = NewState; + } + } + + Context.addTransition(State); +} + std::unique_ptr NonNullParamChecker::genReportNullAttrNonNull(const ExplodedNode *ErrorNode, const Expr *ArgE, diff --git a/clang/test/Analysis/CheckNSError.m b/clang/test/Analysis/CheckNSError.m --- a/clang/test/Analysis/CheckNSError.m +++ b/clang/test/Analysis/CheckNSError.m @@ -62,4 +62,17 @@ return 0; } +int __attribute__((nonnull)) f4(CFErrorRef *error) { + *error = 0; // no-warning + return 0; +} +int __attribute__((nonnull(1))) f5(int *x, CFErrorRef *error) { + *error = 0; // expected-warning {{Potential null dereference}} + return 0; +} + +int __attribute__((nonnull(2))) f6(int *x, CFErrorRef *error) { + *error = 0; // no-warning + return 0; +} diff --git a/clang/test/Analysis/nonnull.cpp b/clang/test/Analysis/nonnull.cpp new file mode 100644 --- /dev/null +++ b/clang/test/Analysis/nonnull.cpp @@ -0,0 +1,25 @@ +// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core -verify %s + +void nonnull [[gnu::nonnull]] (int *q); + +void f1(int *p) { + if (p) + return; + nonnull(p); //expected-warning{{nonnull}} +} + +void f2(int *p) { + if (p) + return; + auto lambda = [](int *q) __attribute__((nonnull)){}; + lambda(p); //expected-warning{{nonnull}} +} + +template +void variadicNonnull(ARGS... args) __attribute__((nonnull)); + +void f3(int a, float b, int *p) { + if (p) + return; + variadicNonnull(a, b, p); //expected-warning{{nonnull}} +}