[StaticAnalyzer] Relax the pre-condition of 'setsockopt'

[ziqingluo-90]

For the unix function
int setsockopt(int, int, int, const void *, socklen_t);, the last two parameters represent a buffer and a size.

In case the size is zero, buffer can be null. Previously, the hard-coded pre-condition requires the buffer to never be null, which can cause false positives.

(rdar://146678142)

[llvmbot]

@llvm/pr-subscribers-clang

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

Author: Ziqing Luo (ziqingluo-90)

Changes

For the unix function
int setsockopt(int, int, int, const void *, socklen_t);, the last two parameters represent a buffer and a size.

In case the size is zero, buffer can be null. Previously, the hard-coded pre-condition requires the buffer to never be null, which can cause false positives.

(rdar://146678142)

---

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

2 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp (+3-2)
• (modified) clang/test/Analysis/std-c-library-functions-POSIX.c (+11)

diff --git a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
index 356d63e3e8b80..fef19b4547555 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -1797,7 +1797,8 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
   auto IsNull = [&](ArgNo ArgN) {
     return std::make_shared<NotNullConstraint>(ArgN, false);
   };
-  auto NotNullBuffer = [&](ArgNo ArgN, ArgNo SizeArg1N, ArgNo SizeArg2N) {
+  auto NotNullBuffer = [&](ArgNo ArgN, ArgNo SizeArg1N,
+                           std::optional<ArgNo> SizeArg2N = std::nullopt) {
     return std::make_shared<NotNullBufferConstraint>(ArgN, SizeArg1N,
                                                      SizeArg2N);
   };
@@ -3365,7 +3366,7 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
         Summary(NoEvalCall)
             .Case(ReturnsZero, ErrnoMustNotBeChecked, GenericSuccessMsg)
             .Case(ReturnsMinusOne, ErrnoNEZeroIrrelevant, GenericFailureMsg)
-            .ArgConstraint(NotNull(ArgNo(3)))
+            .ArgConstraint(NotNullBuffer(ArgNo(3), ArgNo(4)))
             .ArgConstraint(
                 BufferSize(/*Buffer=*/ArgNo(3), /*BufSize=*/ArgNo(4)))
             .ArgConstraint(
diff --git a/clang/test/Analysis/std-c-library-functions-POSIX.c b/clang/test/Analysis/std-c-library-functions-POSIX.c
index b53f3132b8687..f6d88e6c1502d 100644
--- a/clang/test/Analysis/std-c-library-functions-POSIX.c
+++ b/clang/test/Analysis/std-c-library-functions-POSIX.c
@@ -237,3 +237,14 @@ void test_readlinkat_bufsize_zero(int fd, char *Buf, size_t Bufsize) {
   else
     clang_analyzer_eval(Bufsize == 0); // expected-warning{{UNKNOWN}}
 }
+
+void test_setsockopt_bufptr_null(int x) {
+  char buf[10] = {0};
+
+  setsockopt(1, 2, 3, 0, 0);
+  setsockopt(1, 2, 3, buf, 10);
+  if (x)
+    setsockopt(1, 2, 3, buf, 11); // expected-warning{{The 4th argument to 'setsockopt' is a buffer with size 10 but should be a buffer with size equal to or greater than the value of the 5th argument (which is 11)}}
+  else
+    setsockopt(1, 2, 3, 0, 10);  // expected-warning{{The 4th argument to 'setsockopt' is NULL but should not be NULL}}
+}

[ziqingluo-90]

CC @dtarditi

[ziqingluo-90]

While the change is straightforward, I'd like to discuss about the following two constraints:

• NotNullConstraint
  • I think this one should be renamed to probably NullnessConstraint. Its current name was appropriate when it was introduced to check for non-null. But now it is used to check both non-null and null in different cases.
  • I do not understand why it assertsCannotBeNull in the describe member function and !CannotBeNull in describeArgumentValue.
• NotNullBufferConstraint
  • This one has the same naming issue as NotNullConstraint
  • Same confusion on the assertions as NotNullConstraint
  • The negation of the constraint does not fully match my expectation: this constraint is saying size == 0 || buf != 0. So the negation is suppose to be size != 0 && buf == 0. The implementation looks more like size == 0 || buf == 0 to me in the negation case.

In general, how is the negation of a constraint used? Will describe and describeArgumentValue be called on the path where the negation is assumed?
@balazske
CC: @Xazax-hun @haoNoQ @NagyDonat @steakhal

[Xazax-hun]

LGTM!

[Xazax-hun]

This change looks good to me. I think you could open PRs for the questions you brought up and we could sort them out in the review process.

[steakhal]

Thanks for the fix. Your raised points also make sense to me.

[ziqingluo-90]

Thanks for reviewing! I will create another PR for my questions.