[clang] 1f67508 - [analyzer] Do not report CFError null dereference for nonnull params.
Artem Dergachev via cfe-commits
cfe-commits at lists.llvm.org
Mon Apr 20 02:45:54 PDT 2020
Author: Valeriy Savchenko
Date: 2020-04-20T12:33:01+03:00
New Revision: 1f67508b7fedb3a0c8ea55a1f5674346563e2bf2
URL: https://github.com/llvm/llvm-project/commit/1f67508b7fedb3a0c8ea55a1f5674346563e2bf2
DIFF: https://github.com/llvm/llvm-project/commit/1f67508b7fedb3a0c8ea55a1f5674346563e2bf2.diff
LOG: [analyzer] Do not report CFError null dereference for nonnull params.
We want to trust user type annotations and stop assuming pointers declared
as nonnull still can be null. This functionality is implemented as part
of NonNullParamChecker because it already checks parameter attributes.
Whenever we start analyzing a new function, we assume that all parameters
with 'nonnull' attribute are indeed non-null.
Patch by Valeriy Savchenko!
Differential Revision: https://reviews.llvm.org/D77806
Added:
clang/test/Analysis/UserNullabilityAnnotations.m
clang/test/Analysis/nonnull.cpp
Modified:
clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
clang/test/Analysis/CheckNSError.m
Removed:
################################################################################
diff --git a/clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
index 1bff681fb0d0..c3c6a69a222c 100644
--- 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 @@ using namespace ento;
namespace {
class NonNullParamChecker
- : public Checker< check::PreCall, EventDispatcher<ImplicitNullDerefEvent> > {
+ : public Checker<check::PreCall, check::BeginFunction,
+ EventDispatcher<ImplicitNullDerefEvent>> {
mutable std::unique_ptr<BugType> BTAttrNonNull;
mutable std::unique_ptr<BugType> BTNullRefArg;
public:
-
void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
+ void checkBeginFunction(CheckerContext &C) const;
std::unique_ptr<PathSensitiveBugReport>
- genReportNullAttrNonNull(const ExplodedNode *ErrorN,
- const Expr *ArgE,
+ genReportNullAttrNonNull(const ExplodedNode *ErrorN, const Expr *ArgE,
unsigned IdxOfArg) const;
std::unique_ptr<PathSensitiveBugReport>
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 <class CallType>
+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<NonNullAttr>()) {
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 <class CallType>
+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<NonNullAttr>())
+ AttrNonNull.set(ParameterIndex);
+ }
+}
+
+template <class CallType>
+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 @@ void NonNullParamChecker::checkPreCall(const CallEvent &Call,
unsigned NumArgs = Call.getNumArgs();
ProgramStateRef state = C.getState();
- ArrayRef<ParmVarDecl*> parms = Call.parameters();
+ ArrayRef<ParmVarDecl *> parms = Call.parameters();
for (unsigned idx = 0; idx < NumArgs; ++idx) {
// For vararg functions, a corresponding parameter decl may not exist.
@@ -83,15 +122,11 @@ void NonNullParamChecker::checkPreCall(const CallEvent &Call,
// 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<NonNullAttr>();
+ 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 @@ void NonNullParamChecker::checkPreCall(const CallEvent &Call,
if (!DV)
continue;
- assert(!haveRefTypeParam || DV->getAs<Loc>());
+ assert(!HasRefTypeParam || DV->getAs<Loc>());
// Process the case when the argument is not a location.
- if (haveAttrNonNull && !DV->getAs<Loc>()) {
+ if (ExpectedToBeNonNull && !DV->getAs<Loc>()) {
// If the argument is a union type, we want to handle a potential
// transparent_union GCC extension.
if (!ArgE)
@@ -145,9 +180,9 @@ void NonNullParamChecker::checkPreCall(const CallEvent &Call,
if (ExplodedNode *errorNode = C.generateErrorNode(stateNull)) {
std::unique_ptr<BugReport> 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 @@ void NonNullParamChecker::checkPreCall(const CallEvent &Call,
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 @@ void NonNullParamChecker::checkPreCall(const CallEvent &Call,
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<DefinedOrUnknownSVal>();
+
+ // 2. Assume that it is indeed non-null
+ if (ProgramStateRef NewState = State->assume(StoredVal, true)) {
+ State = NewState;
+ }
+ }
+
+ Context.addTransition(State);
+}
+
std::unique_ptr<PathSensitiveBugReport>
NonNullParamChecker::genReportNullAttrNonNull(const ExplodedNode *ErrorNode,
const Expr *ArgE,
diff --git a/clang/test/Analysis/CheckNSError.m b/clang/test/Analysis/CheckNSError.m
index 10890a8627a1..510f11c05579 100644
--- a/clang/test/Analysis/CheckNSError.m
+++ b/clang/test/Analysis/CheckNSError.m
@@ -22,6 +22,7 @@ @interface A
- (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 @@ - (BOOL)myMethodWhichMayFail3:(NSError **_Nonnull)error { // no-warning
*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 @@ int f3(CFErrorRef* error) {
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
index 000000000000..e3c2b6fb05d7
--- /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);
+
+ at interface TestFunctionLevelAnnotations
+- (void)method1:(int *_Nonnull)x;
+- (void)method2:(int *)x __attribute__((nonnull));
+ at end
+
+ at 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}}
+}
+ at 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
index 000000000000..a3a0568686ae
--- /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 <class... ARGS>
+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}}
+}
More information about the cfe-commits
mailing list