[analyzer] StdLibraryFunctionsChecker: Add NotNull Arg Constraint

[steakhal]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

719

Two lines below you are using the {0U} initialization syntax, and here the simple constructor call syntax.
Shouldn't we pick one?

[Szelethus]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

188

What does this do? Is it ever used in the patch?

[NoQ]

This is basically a shorthand for "outside [0, 0]", right? I don't mind ^.^

[balazske]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

202

Is Tmp better name?

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

76

One difficulty is that fread can work if the buf is 0 and count is 0 too. There is for sure code that uses this feature.

[martong]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

188

Yes, it is used. We use it in apply the value is passed to assume.
And in negate we flip the value.

202

Absolutely, yes, I should avoid lower case variables, will rename.

719

Yes, definitely. I think I am going to use brace initialization syntax everywhere.

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

76

My understanding is that in case of fread, if buf is 0 then that is an undefined behavior.

In the section in C11 describing the use of library functions (§7.1.4) it is stated that:

If an argument to a function has an invalid value (such as a value outside the domain of the function, or a pointer outside the address space of the program, or a null pointer, or a pointer to non-modifiable storage when the corresponding parameter is not const-qualified) or a type (after promotion) not expected by a function with variable number of arguments, the behavior is undefined. If a function argument is described as being an array, the pointer actually passed to the function shall have a value such that all address computations and accesses to objects (that would be valid if the pointer did point to the first element of such an array) are in fact valid.

I've found more details in this StackOverflow post:
https://stackoverflow.com/questions/51779960/is-it-safe-to-fread-0-bytes-into-a-null-pointer

Also note that a sane libc implementation would not dereference buf if size is 0, but we may not rely on these implementation details, that is why this is declared as undefined behavior in the standard.

[Szelethus]

LGTM! Please address the nits before commiting, but there is no need for another round of reviews. :)

edit: from my end, that is.

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

188

Forgot my eyes in the office. Woops. I would still prefer a line of comment here :)

930–931

Not super relevant to this specific revision, but shouldn't we leave these to StreamChecker?

[martong]

• Use report/bugpath verify prefixes in test
• Address review nits

[martong]

In D75063#1923780, @NoQ wrote:

This is basically a shorthand for "outside [0, 0]", right? I don't mind ^.^

Yeah, and my first attempt was exactly to implement this with ranges. However, it failed when I realized that we cannot cast a pointer to NonLoc, so the already written RangeConstraint::apply* functions could not work (I would have to add another branch for handling Loc kind of SVals for the pointer case).

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

188

Ok, I added a comment about its role.

719

Finally I ended up with the parens. :)

930–931

Well, yeah we could remove fread and fwrite from the summaries entirely at some point, but that will require changing the test files here.

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

ComparisonConstraint(ArgNo ArgN, BinaryOperator::Opcode Opcode,
                          ArgNo OtherArgN)
         : ValueConstraint(ArgN), Opcode(Opcode), OtherArgN(OtherArgN) {}
     ArgNo getOtherArgNo() const { return OtherArgN; }
     BinaryOperator::Opcode getOpcode() const { return Opcode; }
     ProgramStateRef apply(ProgramStateRef State, const CallEvent &Call,
                           const Summary &Summary) const override;
   };
 
+  class NotNullConstraint : public ValueConstraint {
+    using ValueConstraint::ValueConstraint;
+    // This variable has a role when we negate the constraint.
+    bool CannotBeNull = true;
+
+  public:
+    ProgramStateRef apply(ProgramStateRef State, const CallEvent &Call,
+                          const Summary &Summary) const override {
+      SVal V = getArgSVal(Call, getArgNo());
+      DefinedOrUnknownSVal L = V.castAs<DefinedOrUnknownSVal>();
+      if (!L.getAs<Loc>())
+        return State;
+
+      return State->assume(L, CannotBeNull);
+    }
+
+    ValueConstraintPtr negate() const override {
+      NotNullConstraint Tmp(*this);
+      Tmp.CannotBeNull = !this->CannotBeNull;
+      return std::make_shared<NotNullConstraint>(Tmp);
+    }
+  };
+
   /// The complete list of constraints that defines a single branch.
   typedef std::vector<ValueConstraintPtr> ConstraintSet;
 
   using ArgTypes = std::vector<QualType>;
   using Cases = std::vector<ConstraintSet>;
 
   /// Includes information about
   ///   * function prototype (which is necessary to
struct Summary {
     }
 
   private:
     static void assertTypeSuitableForSummary(QualType T) {
       assert(!T->isVoidType() &&
              "We should have had no significant void types in the spec");
       assert(T.isCanonical() &&
              "We should only have canonical types in the spec");
-      // FIXME: lift this assert (but not the ones above!)
-      assert(T->isIntegralOrEnumerationType() &&
-             "We only support integral ranges in the spec");
     }
 
   public:
     QualType getArgType(ArgNo ArgN) const {
       QualType T = (ArgN == Ret) ? RetTy : ArgTys[ArgN];
       assertTypeSuitableForSummary(T);
       return T;
     }
void StdLibraryFunctionsChecker::initFunctionSummaries(
   // or long long, so three summary variants would be enough).
   // Of course, function variants are also useful for C++ overloads.
   const QualType
       Irrelevant{}; // A placeholder, whenever we do not care about the type.
   const QualType IntTy = ACtx.IntTy;
   const QualType LongTy = ACtx.LongTy;
   const QualType LongLongTy = ACtx.LongLongTy;
   const QualType SizeTy = ACtx.getSizeType();
+  const QualType VoidPtrTy = ACtx.VoidPtrTy; // void *T
+  const QualType ConstVoidPtrTy =
+      ACtx.getPointerType(ACtx.VoidTy.withConst()); // const void *T
 
   const RangeInt IntMax = BVF.getMaxValue(IntTy).getLimitedValue();
   const RangeInt LongMax = BVF.getMaxValue(LongTy).getLimitedValue();
   const RangeInt LongLongMax = BVF.getMaxValue(LongLongTy).getLimitedValue();
 
   // Set UCharRangeMax to min of int or uchar maximum value.
   // The C standard states that the arguments of functions like isalpha must
   // be representable as an unsigned char. Their type is 'int', so the max
void StdLibraryFunctionsChecker::initFunctionSummaries(
   } ReturnValueCondition;
   auto Range = [](RangeInt b, RangeInt e) {
     return IntRangeVector{std::pair<RangeInt, RangeInt>{b, e}};
   };
   auto SingleValue = [](RangeInt v) {
     return IntRangeVector{std::pair<RangeInt, RangeInt>{v, v}};
   };
   auto LessThanOrEq = BO_LE;
+  auto NotNull = [&](ArgNo ArgN) {
+    return std::make_shared<NotNullConstraint>(ArgN);
+  };
 
   using RetType = QualType;
   // Templates for summaries that are reused by many functions.
   auto Getc = [&]() {
     return Summary(ArgTypes{Irrelevant}, RetType{IntTy}, NoEvalCall)
         .Case({ReturnValueCondition(WithinRange,
                                     {{EOFv, EOFv}, {0, UCharRangeMax}})});
   };
   auto Read = [&](RetType R, RangeInt Max) {
     return Summary(ArgTypes{Irrelevant, Irrelevant, SizeTy}, RetType{R},
                    NoEvalCall)
         .Case({ReturnValueCondition(LessThanOrEq, ArgNo(2)),
                ReturnValueCondition(WithinRange, Range(-1, Max))});
   };
   auto Fread = [&]() {
-    return Summary(ArgTypes{Irrelevant, Irrelevant, SizeTy, Irrelevant},
+    return Summary(ArgTypes{VoidPtrTy, Irrelevant, SizeTy, Irrelevant},
+                   RetType{SizeTy}, NoEvalCall)
+        .Case({
+            ReturnValueCondition(LessThanOrEq, ArgNo(2)),
+        })
+        .ArgConstraint(NotNull(ArgNo(0)));
+  };
+  auto Fwrite = [&]() {
+    return Summary(ArgTypes{ConstVoidPtrTy, Irrelevant, SizeTy, Irrelevant},
                    RetType{SizeTy}, NoEvalCall)
         .Case({
             ReturnValueCondition(LessThanOrEq, ArgNo(2)),
-        });
+        })
+        .ArgConstraint(NotNull(ArgNo(0)));
   };
   auto Getline = [&](RetType R, RangeInt Max) {
     return Summary(ArgTypes{Irrelevant, Irrelevant, Irrelevant}, RetType{R},
                    NoEvalCall)
         .Case({ReturnValueCondition(WithinRange, {{-1, -1}, {1, Max}})});
   };
 
   FunctionSummaryMap = {
FunctionSummaryMap = {
 
       // read()-like functions that never return more than buffer size.
       // We are not sure how ssize_t is defined on every platform, so we
       // provide three variants that should cover common cases.
       {"read", Summaries{Read(IntTy, IntMax), Read(LongTy, LongMax),
                          Read(LongLongTy, LongLongMax)}},
       {"write", Summaries{Read(IntTy, IntMax), Read(LongTy, LongMax),
                           Read(LongLongTy, LongLongMax)}},
       {"fread", Summaries{Fread()}},
-      {"fwrite", Summaries{Fread()}},
+      {"fwrite", Summaries{Fwrite()}},
       // getline()-like functions either fail or read at least the delimiter.
       {"getline", Summaries{Getline(IntTy, IntMax), Getline(LongTy, LongMax),
                             Getline(LongLongTy, LongLongMax)}},
       {"getdelim", Summaries{Getline(IntTy, IntMax), Getline(LongTy, LongMax),
                              Getline(LongLongTy, LongLongMax)}},
   };
 }
 

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

if (x > 255) { // \
     int ret = isalnum(x); // \
     // report-warning{{Function argument constraint is not satisfied}} \
     // bugpath-warning{{Function argument constraint is not satisfied}} \
     // bugpath-note{{Function argument constraint is not satisfied}}
 
     (void)ret;
   }
 }
+
+typedef struct FILE FILE;
+typedef typeof(sizeof(int)) size_t;
+size_t fread(void *, size_t, size_t, FILE *);
+void test_notnull_concrete(FILE *fp) {
+  fread(0, sizeof(int), 10, fp); // \
+  // report-warning{{Function argument constraint is not satisfied}} \
+  // bugpath-warning{{Function argument constraint is not satisfied}} \
+  // bugpath-note{{Function argument constraint is not satisfied}}
+}
+void test_notnull_symbolic(FILE *fp, int *buf) {
+  fread(buf, sizeof(int), 10, fp);
+  clang_analyzer_eval(buf != 0); // \
+  // report-warning{{TRUE}} \
+  // bugpath-warning{{TRUE}} \
+  // bugpath-note{{TRUE}} \
+  // bugpath-note{{'buf' is not equal to null}}
+}
+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{{Function argument constraint is not satisfied}} \
+    // bugpath-warning{{Function argument constraint is not satisfied}} \
+    // bugpath-note{{Function argument constraint is not satisfied}}
+}

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

void test_read_write(int fd, char *buf) {
   } else {
     clang_analyzer_eval(x == -1); // expected-warning{{TRUE}}
   }
 }
 
 size_t fread(void *, size_t, size_t, FILE *);
 size_t fwrite(const void *restrict, size_t, size_t, FILE *restrict);
 void test_fread_fwrite(FILE *fp, int *buf) {
+
   size_t x = fwrite(buf, sizeof(int), 10, fp);
   clang_analyzer_eval(x <= 10); // expected-warning{{TRUE}}
+
   size_t y = fread(buf, sizeof(int), 10, fp);
   clang_analyzer_eval(y <= 10); // expected-warning{{TRUE}}
+
   size_t z = fwrite(buf, sizeof(int), y, fp);
   clang_analyzer_eval(z <= y); // expected-warning{{TRUE}}
 }
 
 ssize_t getline(char **, size_t *, FILE *);
 void test_getline(FILE *fp) {
   char *line = 0;
   size_t n = 0;