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,82 @@ 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()) { + unsigned ParameterIndex = Parameter->getFunctionScopeIndex(); + if (ParameterIndex == AttrNonNull.size()) + break; + + if (Parameter->hasAttr()) + AttrNonNull.set(ParameterIndex); + } +} + +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 +114,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 +122,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 +136,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 +180,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. @@ -164,8 +199,8 @@ if (stateNull) { if (ExplodedNode *N = C.generateSink(stateNull, C.getPredecessor())) { ImplicitNullDerefEvent event = { - V, false, N, &C.getBugReporter(), - /*IsDirectDereference=*/haveRefTypeParam}; + V, false, N, &C.getBugReporter(), + /*IsDirectDereference=*/HasRefTypeParam}; dispatchEvent(event); } } @@ -180,6 +215,59 @@ C.addTransition(state); } +/// We want to trust developer annotations and consider all 'nonnull' 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 { + // Planned assumption makes sense only for top-level functions. + // Inlined functions will get similar constraints as part of 'checkPreCall'. + if (!Context.inTopFrame()) + return; + + 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); + // We never consider top-level function parameters undefined. + auto StoredVal = + State->getSVal(ParameterLoc).castAs(); + + // 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 @@ -22,6 +22,7 @@ - (void)myMethodWhichMayFail:(NSError **)error; - (BOOL)myMethodWhichMayFail2:(NSError **)error; - (BOOL)myMethodWhichMayFail3:(NSError **_Nonnull)error; +- (BOOL)myMethodWhichMayFail4:(NSError **)error __attribute__((nonnull)); @end @implementation A @@ -38,6 +39,11 @@ *error = [NSError errorWithDomain:@"domain" code:1 userInfo:0]; // no-warning return 0; } + +- (BOOL)myMethodWhichMayFail4:(NSError **)error { // no-warning + *error = [NSError errorWithDomain:@"domain" code:1 userInfo:0]; // no-warning + return 0; +} @end struct __CFError {}; @@ -62,4 +68,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/UserNullabilityAnnotations.m b/clang/test/Analysis/UserNullabilityAnnotations.m new file mode 100644 --- /dev/null +++ b/clang/test/Analysis/UserNullabilityAnnotations.m @@ -0,0 +1,36 @@ +// RUN: %clang_analyze_cc1 -verify -Wno-objc-root-class %s \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=nullability \ +// RUN: -analyzer-checker=debug.ExprInspection + +void clang_analyzer_eval(int); + +@interface TestFunctionLevelAnnotations +- (void)method1:(int *_Nonnull)x; +- (void)method2:(int *)x __attribute__((nonnull)); +@end + +@implementation TestFunctionLevelAnnotations +- (void)method1:(int *_Nonnull)x { + clang_analyzer_eval(x != 0); // expected-warning{{TRUE}} +} + +- (void)method2:(int *)x { + clang_analyzer_eval(x != 0); // expected-warning{{TRUE}} +} +@end + +typedef struct NestedNonnullMember { + struct NestedNonnullMember *Child; + int *_Nonnull Value; +} NestedNonnullMember; + +NestedNonnullMember *foo(); + +void f1(NestedNonnullMember *Root) { + NestedNonnullMember *Grandson = Root->Child->Child; + + clang_analyzer_eval(Root->Value != 0); // expected-warning{{TRUE}} + clang_analyzer_eval(Grandson->Value != 0); // expected-warning{{TRUE}} + clang_analyzer_eval(foo()->Child->Value != 0); // expected-warning{{TRUE}} +} 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,34 @@ +// 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}} +} + +int globalVar = 15; +void moreParamsThanArgs [[gnu::nonnull(2, 4)]] (int a, int *p, int b = 42, int *q = &globalVar); + +void f4(int a, int *p) { + if (p) + return; + moreParamsThanArgs(a, p); //expected-warning{{nonnull}} +}