From 4f33e7c683104ea72e013d4ddd104b711a25d620 Mon Sep 17 00:00:00 2001 From: Arseniy Zaostrovnykh Date: Tue, 27 Aug 2024 14:41:52 +0200 Subject: [PATCH] [analyzer] Report violations of the "returns_nonnull" attribute (#106048) Make sure code respects the GNU-extension `__attribute__((returns_nonnull))`. Extend the NullabilityChecker to check that a function returns_nonnull does not return a nullptr. This commit also reverts an old hack introduced by 49bd58f1ebe28d97e4949e9c757bc5dfd8b2d72f because it is no longer needed CPP-4741 --- clang/docs/analyzer/checkers.rst | 22 ++++++- .../Checkers/NullabilityChecker.cpp | 10 +++- clang/test/Analysis/nullability.c | 59 ++++++++++++++++++- clang/test/Analysis/nullability.mm | 2 +- 4 files changed, 85 insertions(+), 8 deletions(-) diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst index 0bfbc995579d413..89a1018e14c0e66 100644 --- a/clang/docs/analyzer/checkers.rst +++ b/clang/docs/analyzer/checkers.rst @@ -571,7 +571,7 @@ if ((y = make_int())) { nullability ^^^^^^^^^^^ -Objective C checkers that warn for null pointer passing and dereferencing errors. +Checkers (mostly Objective C) that warn for null pointer passing and dereferencing errors. .. _nullability-NullPassedToNonnull: @@ -588,8 +588,8 @@ Warns when a null pointer is passed to a pointer which has a _Nonnull type. .. _nullability-NullReturnedFromNonnull: -nullability.NullReturnedFromNonnull (ObjC) -"""""""""""""""""""""""""""""""""""""""""" +nullability.NullReturnedFromNonnull (C, C++, ObjC) +"""""""""""""""""""""""""""""""""""""""""""""""""" Warns when a null pointer is returned from a function that has _Nonnull return type. .. code-block:: objc @@ -604,6 +604,22 @@ Warns when a null pointer is returned from a function that has _Nonnull return t return result; } +Warns when a null pointer is returned from a function annotated with ``__attribute__((returns_nonnull))`` + +.. code-block:: cpp + + int global; + __attribute__((returns_nonnull)) void* getPtr(void* p); + + void* getPtr(void* p) { + if (p) { // forgot to negate the condition + return &global; + } + // Warning: nullptr returned from a function that is expected + // to return a non-null value + return p; + } + .. _nullability-NullableDereferenced: nullability.NullableDereferenced (ObjC) diff --git a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp index 60934e51febe840..04472bb3895a78d 100644 --- a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp @@ -692,6 +692,14 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S, NullConstraint Nullness = getNullConstraint(*RetSVal, State); Nullability RequiredNullability = getNullabilityAnnotation(RequiredRetType); + if (const auto *FunDecl = C.getLocationContext()->getDecl(); + FunDecl && FunDecl->getAttr() && + (RequiredNullability == Nullability::Unspecified || + RequiredNullability == Nullability::Nullable)) { + // If a function is marked with the returns_nonnull attribute, + // the return value must be non-null. + RequiredNullability = Nullability::Nonnull; + } // If the returned value is null but the type of the expression // generating it is nonnull then we will suppress the diagnostic. @@ -705,7 +713,7 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S, Nullness == NullConstraint::IsNull); if (ChecksEnabled[CK_NullReturnedFromNonnull] && NullReturnedFromNonNull && RetExprTypeLevelNullability != Nullability::Nonnull && - !InSuppressedMethodFamily && C.getLocationContext()->inTopFrame()) { + !InSuppressedMethodFamily) { static CheckerProgramPointTag Tag(this, "NullReturnedFromNonnull"); ExplodedNode *N = C.generateErrorNode(State, &Tag); if (!N) diff --git a/clang/test/Analysis/nullability.c b/clang/test/Analysis/nullability.c index fbc03c864ad83f4..57ce9ac8aee3d01 100644 --- a/clang/test/Analysis/nullability.c +++ b/clang/test/Analysis/nullability.c @@ -1,12 +1,65 @@ -// RUN: %clang_analyze_cc1 -fblocks -analyzer-checker=core,nullability -Wno-deprecated-non-prototype -verify %s +// RUN: %clang_analyze_cc1 -fblocks -analyzer-checker=core,nullability,debug.ExprInspection -verify %s + +void clang_analyzer_warnIfReached(void); void it_takes_two(int a, int b); -void function_pointer_arity_mismatch() { +void function_pointer_arity_mismatch(void) { void(*fptr)() = it_takes_two; fptr(1); // no-crash expected-warning {{Function taking 2 arguments is called with fewer (1)}} + // expected-warning@-1 {{passing arguments to a function without a prototype is deprecated in all versions of C and is not supported in C23}} } -void block_arity_mismatch() { +void block_arity_mismatch(void) { void(^b)() = ^(int a, int b) { }; b(1); // no-crash expected-warning {{Block taking 2 arguments is called with fewer (1)}} + // expected-warning@-1 {{passing arguments to a function without a prototype is deprecated in all versions of C and is not supported in C23}} +} + +int *nonnull_return_annotation_indirect(void) __attribute__((returns_nonnull)); +int *nonnull_return_annotation_indirect(void) { + int *x = 0; + return x; // expected-warning {{Null returned from a function that is expected to return a non-null value}} +} + +int *nonnull_return_annotation_direct(void) __attribute__((returns_nonnull)); +int *nonnull_return_annotation_direct(void) { + return 0; // expected-warning {{Null returned from a function that is expected to return a non-null value}} +} // expected-warning@-1 {{null returned from function that requires a non-null return value}} + +int *nonnull_return_annotation_assumed(int* ptr) __attribute__((returns_nonnull)); +int *nonnull_return_annotation_assumed(int* ptr) { + if (ptr) { + return ptr; + } + return ptr; // expected-warning {{Null returned from a function that is expected to return a non-null value}} +} + +int *produce_nonnull_ptr(void) __attribute__((returns_nonnull)); + +__attribute__((returns_nonnull)) +int *cannot_return_null(void) { + int *x = produce_nonnull_ptr(); + if (!x) { + clang_analyzer_warnIfReached(); + // expected-warning@-1 {{REACHABLE}} + // TODO: This warning is a false positive, according to the contract of + // produce_nonnull_ptr, x cannot be null. + } + // Regardless of the potential state split above, x cannot be nullptr + // according to the produce_nonnull_ptr annotation. + return x; + // False positive: expected-warning@-1 {{Null returned from a function that is expected to return a non-null value}} +} + +__attribute__((returns_nonnull)) int *passthrough(int *p) { + return p; // no-warning: we have no evidence that `p` is null, i.e., violating the contract +} + +__attribute__((returns_nonnull)) int *passthrough2(int *p); +int *passthrough2(int *p) { + return p; // expected-warning{{Null returned from a function that is expected to return a non-null value}} +} + +void call_with_null(void) { + passthrough2(0); } diff --git a/clang/test/Analysis/nullability.mm b/clang/test/Analysis/nullability.mm index d69116d03df7465..64222d939bdd065 100644 --- a/clang/test/Analysis/nullability.mm +++ b/clang/test/Analysis/nullability.mm @@ -438,7 +438,7 @@ -(Dummy *)callerWithParam:(Dummy * _Nonnull) p1 { int * _Nonnull InlinedReturnNullOverSuppressionCallee(int * _Nonnull p2) { int *result = 0; - return result; // no-warning; but this is an over suppression + return result; // expected-warning{{Null returned from a function that is expected to return a non-null value}} } int *InlinedReturnNullOverSuppressionCaller(int * _Nonnull p1) {