[clang][analyzer] StdLibraryFunctionsChecker: Allow NULL buffer in `fread` and `fwrite` if size is zero.

[donat.nagy]

LGTM.

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

414–415

What would you think about a SmallVector<2> for these? It would allow you to handle them with a (short) for loop instead of separate commands.

[donat.nagy]

By the way here is a concrete example where this patch is needed to squash a false positive: https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=postgres_REL_13_0_stdclf_notetag_interesting_test_3&is-unique=on&report-hash=49939ae1896dff1ad782092b8534bd68&report-filepath=%2arelcache.c&report-id=1942889

[balazske]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

414–415

It does not look to be much better, the only place where a loop can be used is in the apply function but because optionals (that are to be checked at both values) it is not much better. And a vector could be constructed in the apply function too, but for 2 elements it is not important.

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

protected:
     bool checkSpecificValidity(const FunctionDecl *FD) const override {
       const bool ValidArg = getArgType(FD, ArgN)->isPointerType();
       assert(ValidArg &&
              "This constraint should be applied only on a pointer type");
       return ValidArg;
     }
   };
 
+  /// Check null or non-null-ness of an argument that is of pointer type.
+  /// The argument is meant to be a buffer that has a size constraint, and it
+  /// is allowed to have a NULL value if the size is 0. The size can depend on
+  /// 1 or 2 additional arguments, if one of these is 0 the buffer is allowed to
+  /// be NULL. This is useful for functions like `fread` which have this special
+  /// property.
+  class NotNullBufferConstraint : public ValueConstraint {
+    using ValueConstraint::ValueConstraint;
+    ArgNo SizeArg1N;
+    std::optional<ArgNo> SizeArg2N;
+    // This variable has a role when we negate the constraint.
+    bool CannotBeNull = true;
+
+  public:
+    NotNullBufferConstraint(ArgNo ArgN, ArgNo SizeArg1N,
+                            std::optional<ArgNo> SizeArg2N,
+                            bool CannotBeNull = true)
+        : ValueConstraint(ArgN), SizeArg1N(SizeArg1N), SizeArg2N(SizeArg2N),
+          CannotBeNull(CannotBeNull) {}
+
+    ProgramStateRef apply(ProgramStateRef State, const CallEvent &Call,
+                          const Summary &Summary,
+                          CheckerContext &C) const override;
+
+    void describe(DescriptionKind DK, const CallEvent &Call,
+                  ProgramStateRef State, const Summary &Summary,
+                  llvm::raw_ostream &Out) const override;
+
+    bool describeArgumentValue(const CallEvent &Call, ProgramStateRef State,
+                               const Summary &Summary,
+                               llvm::raw_ostream &Out) const override;
+
+    ValueConstraintPtr negate() const override {
+      NotNullBufferConstraint Tmp(*this);
+      Tmp.CannotBeNull = !this->CannotBeNull;
+      return std::make_shared<NotNullBufferConstraint>(Tmp);
+    }
+
+  protected:
+    bool checkSpecificValidity(const FunctionDecl *FD) const override {
+      const bool ValidArg = getArgType(FD, ArgN)->isPointerType();
+      assert(ValidArg &&
+             "This constraint should be applied only on a pointer type");
+      return ValidArg;
+    }
+  };
+
   // Represents a buffer argument with an additional size constraint. The
   // constraint may be a concrete value, or a symbolic value in an argument.
   // Example 1. Concrete value as the minimum buffer size.
   //   char *asctime_r(const struct tm *restrict tm, char *restrict buf);
   //   // `buf` size must be at least 26 bytes according the POSIX standard.
   // Example 2. Argument as a buffer size.
   //   ctime_s(char *buffer, rsize_t bufsz, const time_t *time);
   // Example 3. The size is computed as a multiplication of other args.

 bool StdLibraryFunctionsChecker::NotNullConstraint::describeArgumentValue(
     const CallEvent &Call, ProgramStateRef State, const Summary &Summary,
     llvm::raw_ostream &Out) const {
   assert(!CannotBeNull && "This function is used when the value is NULL");
   Out << "is NULL";
   return true;
 }
 
+ProgramStateRef StdLibraryFunctionsChecker::NotNullBufferConstraint::apply(
+    ProgramStateRef State, const CallEvent &Call, const Summary &Summary,
+    CheckerContext &C) const {
+  SVal V = getArgSVal(Call, getArgNo());
+  if (V.isUndef())
+    return State;
+  DefinedOrUnknownSVal L = V.castAs<DefinedOrUnknownSVal>();
+  if (!isa<Loc>(L))
+    return State;
+
+  std::optional<DefinedOrUnknownSVal> SizeArg1 =
+      getArgSVal(Call, SizeArg1N).getAs<DefinedOrUnknownSVal>();
+  std::optional<DefinedOrUnknownSVal> SizeArg2;
+  if (SizeArg2N)
+    SizeArg2 = getArgSVal(Call, *SizeArg2N).getAs<DefinedOrUnknownSVal>();
+
+  auto IsArgZero = [State](std::optional<DefinedOrUnknownSVal> Val) {
+    if (!Val)
+      return false;
+    auto [IsNonNull, IsNull] = State->assume(*Val);
+    return IsNull && !IsNonNull;
+  };
+
+  if (IsArgZero(SizeArg1) || IsArgZero(SizeArg2))
+    return State;
+
+  return State->assume(L, CannotBeNull);
+}
+
+void StdLibraryFunctionsChecker::NotNullBufferConstraint::describe(
+    DescriptionKind DK, const CallEvent &Call, ProgramStateRef State,
+    const Summary &Summary, llvm::raw_ostream &Out) const {
+  assert(CannotBeNull &&
+         "Describe should not be used when the value must be NULL");
+  if (DK == Violation)
+    Out << "should not be NULL";
+  else
+    Out << "is not NULL";
+}
+
+bool StdLibraryFunctionsChecker::NotNullBufferConstraint::describeArgumentValue(
+    const CallEvent &Call, ProgramStateRef State, const Summary &Summary,
+    llvm::raw_ostream &Out) const {
+  assert(!CannotBeNull && "This function is used when the value is NULL");
+  Out << "is NULL";
+  return true;
+}
+
 ProgramStateRef StdLibraryFunctionsChecker::BufferSizeConstraint::apply(
     ProgramStateRef State, const CallEvent &Call, const Summary &Summary,
     CheckerContext &C) const {
   SValBuilder &SvalBuilder = C.getSValBuilder();
   // The buffer argument.
   SVal BufV = getArgSVal(Call, getArgNo());
 
   // Get the size constraint.
void StdLibraryFunctionsChecker::initFunctionSummaries(
   };
   auto LessThanOrEq = BO_LE;
   auto NotNull = [&](ArgNo ArgN) {
     return std::make_shared<NotNullConstraint>(ArgN);
   };
   auto IsNull = [&](ArgNo ArgN) {
     return std::make_shared<NotNullConstraint>(ArgN, false);
   };
+  auto NotNullBuffer = [&](ArgNo ArgN, ArgNo SizeArg1N, ArgNo SizeArg2N) {
+    return std::make_shared<NotNullBufferConstraint>(ArgN, SizeArg1N,
+                                                     SizeArg2N);
+  };
 
   std::optional<QualType> FileTy = lookupTy("FILE");
   std::optional<QualType> FilePtrTy = getPointerTy(FileTy);
   std::optional<QualType> FilePtrRestrictTy = getRestrictTy(FilePtrTy);
 
   std::optional<QualType> FPosTTy = lookupTy("fpos_t");
   std::optional<QualType> FPosTPtrTy = getPointerTy(FPosTTy);
   std::optional<QualType> ConstFPosTPtrTy = getPointerTy(getConstTy(FPosTTy));
auto FreadSummary =
                 ErrnoNEZeroIrrelevant, GenericFailureMsg)
           .Case({ArgumentCondition(1U, WithinRange, Range(1, SizeMax)),
                  ReturnValueCondition(BO_EQ, ArgNo(2)),
                  ReturnValueCondition(WithinRange, Range(0, SizeMax))},
                 ErrnoMustNotBeChecked, GenericSuccessMsg)
           .Case({ArgumentCondition(1U, WithinRange, SingleValue(0)),
                  ReturnValueCondition(WithinRange, SingleValue(0))},
                 ErrnoMustNotBeChecked, GenericSuccessMsg)
-          .ArgConstraint(NotNull(ArgNo(0)))
+          .ArgConstraint(NotNullBuffer(ArgNo(0), ArgNo(1), ArgNo(2)))
           .ArgConstraint(NotNull(ArgNo(3)))
-          // FIXME: It should be allowed to have a null buffer if any of
-          // args 1 or 2 are zero. Remove NotNull check of arg 0, add a check
-          // for non-null buffer if non-zero size to BufferSizeConstraint?
           .ArgConstraint(BufferSize(/*Buffer=*/ArgNo(0), /*BufSize=*/ArgNo(1),
                                     /*BufSizeMultiplier=*/ArgNo(2)));
 
   // size_t fread(void *restrict ptr, size_t size, size_t nitems,
   //              FILE *restrict stream);
   addToFunctionSummaryMap(
       "fread",
       Signature(ArgTypes{VoidPtrRestrictTy, SizeTy, SizeTy, FilePtrRestrictTy},

   // Functions for testing.
   if (AddTestFunctions) {
     const RangeInt IntMin = BVF.getMinValue(IntTy).getLimitedValue();
 
     addToFunctionSummaryMap(
         "__not_null", Signature(ArgTypes{IntPtrTy}, RetType{IntTy}),
         Summary(EvalCallAsPure).ArgConstraint(NotNull(ArgNo(0))));
 
+    addToFunctionSummaryMap(
+        "__not_null_buffer",
+        Signature(ArgTypes{VoidPtrTy, IntTy, IntTy}, RetType{IntTy}),
+        Summary(EvalCallAsPure)
+            .ArgConstraint(NotNullBuffer(ArgNo(0), ArgNo(1), ArgNo(2))));
+
     // Test inside range constraints.
     addToFunctionSummaryMap(
         "__single_val_0", Signature(ArgTypes{IntTy}, RetType{IntTy}),
         Summary(EvalCallAsPure)
             .ArgConstraint(ArgumentCondition(0U, WithinRange, SingleValue(0))));
     addToFunctionSummaryMap(
         "__single_val_1", Signature(ArgTypes{IntTy}, RetType{IntTy}),
         Summary(EvalCallAsPure)

clang/test/Analysis/std-c-library-functions-arg-constraints.c

 void test_notnull_symbolic2(FILE *fp, int *buf) {
   if (!buf)                          // bugpath-note{{Assuming 'buf' is null}} \
             // bugpath-note{{Taking true branch}}
     fread(buf, sizeof(int), 10, fp); // \
     // report-warning{{The 1st argument to 'fread' is NULL but should not be NULL}} \
     // bugpath-warning{{The 1st argument to 'fread' is NULL but should not be NULL}} \
     // bugpath-note{{The 1st argument to 'fread' is NULL but should not be NULL}}
 }
+
+int __not_null_buffer(void *, int, int);
+
+void test_notnull_buffer_1(void *buf) {
+  __not_null_buffer(buf, 0, 1);
+  clang_analyzer_eval(buf != 0); // \
+  // report-warning{{TRUE}} \
+  // bugpath-warning{{TRUE}} \
+  // report-warning{{FALSE}} \
+  // bugpath-warning{{FALSE}} \
+  // bugpath-note{{TRUE}} \
+  // bugpath-note{{FALSE}} \
+  // bugpath-note{{Assuming 'buf' is equal to null}} \
+  // bugpath-note{{Assuming 'buf' is not equal to null}}
+}
+
+void test_notnull_buffer_2(void *buf) {
+  __not_null_buffer(buf, 1, 0);
+  clang_analyzer_eval(buf != 0); // \
+  // report-warning{{TRUE}} \
+  // bugpath-warning{{TRUE}} \
+  // report-warning{{FALSE}} \
+  // bugpath-warning{{FALSE}} \
+  // bugpath-note{{TRUE}} \
+  // bugpath-note{{FALSE}} \
+  // bugpath-note{{Assuming 'buf' is equal to null}} \
+  // bugpath-note{{Assuming 'buf' is not equal to null}}
+}
+
+void test_notnull_buffer_3(void *buf) {
+  __not_null_buffer(buf, 1, 1);
+  clang_analyzer_eval(buf != 0); // \
+  // report-warning{{TRUE}} \
+  // bugpath-warning{{TRUE}} \
+  // bugpath-note{{TRUE}} \
+  // bugpath-note{{'buf' is not equal to null}}
+}
+
 void test_no_node_after_bug(FILE *fp, size_t size, size_t n, void *buf) {
   if (fp) // \
   // bugpath-note{{Assuming 'fp' is null}} \
   // bugpath-note{{Taking false branch}}
     return;
   size_t ret = fread(buf, size, n, fp); // \
   // report-warning{{The 4th argument to 'fread' is NULL but should not be NULL}} \
   // bugpath-warning{{The 4th argument to 'fread' is NULL but should not be NULL}} \