[clang] [analyzer] Report violations of the "returns_nonnull" attribute (PR #106048)

Arseniy Zaostrovnykh via cfe-commits cfe-commits at lists.llvm.org
Tue Aug 27 04:41:42 PDT 2024


https://github.com/necto updated https://github.com/llvm/llvm-project/pull/106048

>From 7d5ae515f7727de98e7e8ce2f259e579a1f24463 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh <necto.ne at gmail.com>
Date: Tue, 20 Aug 2024 17:31:11 +0200
Subject: [PATCH 1/8] [analyzer] Report violations of the "returns_nonnull"
 attribute

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.

CPP-4741
---
 .../Checkers/NullabilityChecker.cpp           |  8 ++++
 clang/test/Analysis/nullability.c             | 43 ++++++++++++++++++-
 2 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
index 60934e51febe84..2035d50eea4c2d 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<ReturnsNonNullAttr>() &&
+      (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.
diff --git a/clang/test/Analysis/nullability.c b/clang/test/Analysis/nullability.c
index fbc03c864ad83f..9ddb9c8d2dc34f 100644
--- a/clang/test/Analysis/nullability.c
+++ b/clang/test/Analysis/nullability.c
@@ -1,4 +1,6 @@
-// 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 -Wno-deprecated-non-prototype -verify %s
+
+void clang_analyzer_warnIfReached();
 
 void it_takes_two(int a, int b);
 void function_pointer_arity_mismatch() {
@@ -10,3 +12,42 @@ void block_arity_mismatch() {
   void(^b)() = ^(int a, int b) { };
   b(1);  // no-crash expected-warning {{Block taking 2 arguments is called with fewer (1)}}
 }
+
+int *nonnull_return_annotation_indirect() __attribute__((returns_nonnull));
+int *nonnull_return_annotation_indirect() {
+  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() __attribute__((returns_nonnull));
+int *nonnull_return_annotation_direct() {
+  return 0; // expected-warning {{Null returned from a function that is expected to return a non-null value}}
+} // expected-warning at -1 {{null returned from function that requires a non-null return value}}
+
+int *nonnull_return_annotation_assumed() __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() __attribute__((returns_nonnull));
+
+__attribute__((returns_nonnull))
+int *cannot_return_null() {
+  int *x = produce_nonnull_ptr();
+  if (!x) {
+    clang_analyzer_warnIfReached();
+    // Incorrect: expected-warning at -1 {{REACHABLE}}
+    // According to produce_nonnull_ptr contract, 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 at -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
+}

>From d3b30f3f31cc3a4cc71ae967ea6fc554a4764e37 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh <arseniy.zaostrovnykh at sonarsource.com>
Date: Mon, 26 Aug 2024 15:59:59 +0200
Subject: [PATCH 2/8] Update clang/test/Analysis/nullability.c
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Co-authored-by: Donát Nagy <donat.nagy at ericsson.com>
---
 clang/test/Analysis/nullability.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/clang/test/Analysis/nullability.c b/clang/test/Analysis/nullability.c
index 9ddb9c8d2dc34f..341d29c6e99d1e 100644
--- a/clang/test/Analysis/nullability.c
+++ b/clang/test/Analysis/nullability.c
@@ -39,8 +39,9 @@ int *cannot_return_null() {
   int *x = produce_nonnull_ptr();
   if (!x) {
     clang_analyzer_warnIfReached();
-    // Incorrect: expected-warning at -1 {{REACHABLE}}
-    // According to produce_nonnull_ptr contract, x cannot be null.
+    // expected-warning at -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.

>From 3d5e750aec0d35b20d5c479db74cd0a5d3ea7853 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh <necto.ne at gmail.com>
Date: Mon, 26 Aug 2024 16:47:57 +0200
Subject: [PATCH 3/8] Add test for inlined nullability violation

---
 clang/test/Analysis/nullability.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/clang/test/Analysis/nullability.c b/clang/test/Analysis/nullability.c
index 341d29c6e99d1e..92fd077f01cd96 100644
--- a/clang/test/Analysis/nullability.c
+++ b/clang/test/Analysis/nullability.c
@@ -52,3 +52,15 @@ int *cannot_return_null() {
 __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__((noreturn))
+void exit(int);
+
+__attribute__((returns_nonnull)) int *passthrough2(int *p);
+int *passthrough2(int *p) {
+  return p; // FIXME: no-warning  Explicitly disabled to avoid FPs
+}
+
+void call_with_null(void) {
+  passthrough2(0);
+}

>From 598c574e0703d316c369f5796fd496af725da6f5 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh <necto.ne at gmail.com>
Date: Mon, 26 Aug 2024 16:52:35 +0200
Subject: [PATCH 4/8] Undo report suppression for null returned from non-null
 functions

This removes the additional constraint introduced in 49bd58f1ebe28d97e4949e9c757bc5dfd8b2d72f
because its premise (producing FP if a parameter reclaimed early) seems
to no longer hold, and because it interferes with C coverage.
---
 clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp | 2 +-
 clang/test/Analysis/nullability.c                        | 2 +-
 clang/test/Analysis/nullability.mm                       | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
index 2035d50eea4c2d..04472bb3895a78 100644
--- a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -713,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 92fd077f01cd96..ed70336c412016 100644
--- a/clang/test/Analysis/nullability.c
+++ b/clang/test/Analysis/nullability.c
@@ -58,7 +58,7 @@ void exit(int);
 
 __attribute__((returns_nonnull)) int *passthrough2(int *p);
 int *passthrough2(int *p) {
-  return p; // FIXME: no-warning  Explicitly disabled to avoid FPs
+  return p; // expected-warning{{Null returned from a function that is expected to return a non-null value}}
 }
 
 void call_with_null(void) {
diff --git a/clang/test/Analysis/nullability.mm b/clang/test/Analysis/nullability.mm
index d69116d03df746..64222d939bdd06 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) {

>From a9c489f683fc7d18a2a26933b65bca7b668b708f Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh <necto.ne at gmail.com>
Date: Mon, 26 Aug 2024 16:57:27 +0200
Subject: [PATCH 5/8] Remove -Wno-deprecated-non-prototype

---
 clang/test/Analysis/nullability.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/clang/test/Analysis/nullability.c b/clang/test/Analysis/nullability.c
index ed70336c412016..4e801b355bd9d5 100644
--- a/clang/test/Analysis/nullability.c
+++ b/clang/test/Analysis/nullability.c
@@ -1,30 +1,32 @@
-// RUN: %clang_analyze_cc1 -fblocks -analyzer-checker=core,nullability,debug.ExprInspection -Wno-deprecated-non-prototype -verify %s
+// RUN: %clang_analyze_cc1 -fblocks -analyzer-checker=core,nullability,debug.ExprInspection -verify %s
 
-void clang_analyzer_warnIfReached();
+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 at -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 at -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() __attribute__((returns_nonnull));
-int *nonnull_return_annotation_indirect() {
+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() __attribute__((returns_nonnull));
-int *nonnull_return_annotation_direct() {
+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 at -1 {{null returned from function that requires a non-null return value}}
 
-int *nonnull_return_annotation_assumed() __attribute__((returns_nonnull));
+int *nonnull_return_annotation_assumed(int* ptr) __attribute__((returns_nonnull));
 int *nonnull_return_annotation_assumed(int* ptr) {
   if (ptr) {
     return ptr;
@@ -32,10 +34,10 @@ int *nonnull_return_annotation_assumed(int* ptr) {
   return ptr; // expected-warning {{Null returned from a function that is expected to return a non-null value}}
 }
 
-int *produce_nonnull_ptr() __attribute__((returns_nonnull));
+int *produce_nonnull_ptr(void) __attribute__((returns_nonnull));
 
 __attribute__((returns_nonnull))
-int *cannot_return_null() {
+int *cannot_return_null(void) {
   int *x = produce_nonnull_ptr();
   if (!x) {
     clang_analyzer_warnIfReached();

>From 2d2ab3185a002edc04d2cf274491208915127075 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh <necto.ne at gmail.com>
Date: Mon, 26 Aug 2024 17:21:09 +0200
Subject: [PATCH 6/8] Update documentation

mention that NullReturnedFromNonnull now also works for C
---
 clang/docs/analyzer/checkers.rst | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 0bfbc995579d41..aa7d250e633bad 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
+"""""""""""""""""""""""""""""""""""
 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)

>From e174221253aa61ea2077d38909a85a13b21ec422 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh <arseniy.zaostrovnykh at sonarsource.com>
Date: Tue, 27 Aug 2024 09:02:01 +0200
Subject: [PATCH 7/8] Update clang/docs/analyzer/checkers.rst
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Co-authored-by: Donát Nagy <donat.nagy at ericsson.com>
---
 clang/docs/analyzer/checkers.rst | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index aa7d250e633bad..89a1018e14c0e6 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -588,8 +588,8 @@ Warns when a null pointer is passed to a pointer which has a _Nonnull type.
 
 .. _nullability-NullReturnedFromNonnull:
 
-nullability.NullReturnedFromNonnull
-"""""""""""""""""""""""""""""""""""
+nullability.NullReturnedFromNonnull (C, C++, ObjC)
+""""""""""""""""""""""""""""""""""""""""""""""""""
 Warns when a null pointer is returned from a function that has _Nonnull return type.
 
 .. code-block:: objc

>From 419561235896936c1b5b005cfefae0ffea4ea6b3 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh <necto.ne at gmail.com>
Date: Tue, 27 Aug 2024 09:04:03 +0200
Subject: [PATCH 8/8] Remove unused function dclaration

---
 clang/test/Analysis/nullability.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/clang/test/Analysis/nullability.c b/clang/test/Analysis/nullability.c
index 4e801b355bd9d5..57ce9ac8aee3d0 100644
--- a/clang/test/Analysis/nullability.c
+++ b/clang/test/Analysis/nullability.c
@@ -55,9 +55,6 @@ __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__((noreturn))
-void exit(int);
-
 __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}}



More information about the cfe-commits mailing list