vsavchenko updated this revision to Diff 256331.
vsavchenko added a comment.
Add extra check to be safe
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77806/new/
https://reviews.llvm.org/D77806
Files:
clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
clang/test/Analysis/CheckNSError.m
clang/test/Analysis/nonnull.cpp
Index: clang/test/Analysis/nonnull.cpp
===================================================================
--- /dev/null
+++ 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}}
+}
Index: clang/test/Analysis/CheckNSError.m
===================================================================
--- clang/test/Analysis/CheckNSError.m
+++ 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;
+}
Index: clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
+++ 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<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 @@
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 @@
// 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];
+ bool ExpectedToBeNonNull = AttrNonNull.test(idx);
- // Check if the parameter is also marked 'nonnull'.
- if (!haveAttrNonNull && HasParam)
- haveAttrNonNull = parms[idx]->hasAttr<NonNullAttr>();
-
- 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<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 @@
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 @@
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,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<loc::MemRegionVal>();
+ 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<PathSensitiveBugReport>
NonNullParamChecker::genReportNullAttrNonNull(const ExplodedNode *ErrorNode,
const Expr *ArgE,
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
