[analyzer] Report violations of the "returns_nonnull" attribute

[necto]

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 49bd58f
because it is no longer needed

CPP-4741

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Arseniy Zaostrovnykh (necto)

Changes

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

---

Full diff: https://github.com/llvm/llvm-project/pull/106048.diff

2 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp (+8)
• (modified) clang/test/Analysis/nullability.c (+42-1)

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@-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@-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@-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
+}

[NagyDonat]

Thanks for this improvement, I'm really happy to see this! (I added some inline comments, but they are all minor and tangential.)

Right now all the nullability.* checkers are marked as (ObjC) in the documentation, because (as far as I know) previously they were only supporting the Objective-C style _Nullable and _Nonnull annotations.

However, as your change shows, the same "backend" can be used to handle different kinds of nullability annotations, which would make these checkers available for more users.

Please update the documentation of the affected checkers (in clang/docs/analyzer/checkers.rst) to mark that they are no longer limited to Objective-C.

[NagyDonat]

Consider adding a variant of this where a passthrough function is used in a situation where it's known to return null.

__attribute__((returns_nonnull)) int *passthrough2(int *p) {
  return p;
}
void call_with_null(void) {
  passthrough2(NULL);
}

[necto]

Added in 3d5e750
However, it did not trigger because of an explicit suppression of the reports in inlined functions:
49bd58f

I believe the reasoning for that suppression no longer holds, so I lifted the suppression: 598c574
Do you agree? If so, do you think it should be done in a separate change set?

[NagyDonat]

If the tests within nullability.mm still pass (expect for the over-suppression one), then there is no need to preserve that suppression.

I'd say that it's OK to lift the suppression within this commit, but add a remark like "This commit also reverts an old hack introduced by 49bd58f because it is no longer needed." to the commit message. (Perhaps putting it into a separate commit would be slightly more elegant, but I don't think that it would produce real practical benefits.)

[necto]

Please update the documentation of the affected checkers (in clang/docs/analyzer/checkers.rst) to mark that they are no longer limited to Objective-C.

Updated in 2d2ab31

[NagyDonat]

Thanks for the updates!

[NagyDonat]

Looks good to me now, thanks 😄. Feel free to merge this / I can help with merging if you don't have commit access yet.