[analyzer] Fix StdLibraryFunctionsChecker NotNull Constraint Check

[vabridgers]

Summary

This check was causing a crash in a test case where the 0th argument was
uninitialized ('Assertion `T::isKind(*this)' at line SVals.h:104). This
was happening since the argument was actually undefined, but the castAs
assumes the value is DefinedOrUnknownSVal.

The fix appears to be simply to check for an undefined value and skip
the check allowing the uninitalized value checker to detect the error.

I included a test case that I verified to produce the negative case
prior to the fix, and passes with the fix.

[NoQ]

Thanks!

@Szelethus can we make this checker depend on undefined value checker (probably CallAndMessage) so that uninitialized arguments were handled first? In fact, can we make a silent assumption that everything depends on core? If so we could eliminate all checks for undefined values in PreStmt and PreCall.

[vabridgers]

fix pre-merge lint checks

[vabridgers]

fix pre-merge lint checks

[martong]

LGTM and thanks!

In D77012#1948550, @NoQ wrote:

Thanks!

@Szelethus can we make this checker depend on undefined value checker (probably CallAndMessage) so that uninitialized arguments were handled first? In fact, can we make a silent assumption that everything depends on core? If so we could eliminate all checks for undefined values in PreStmt and PreCall.

+1, we should really do that.

[Szelethus]

In D77012#1948550, @NoQ wrote:

@Szelethus can we make this checker depend on undefined value checker (probably CallAndMessage) so that uninitialized arguments were handled first?

I'll drop some pointers to previous discussions: D67336#1928925, D69662#1891124. I took a look, the evaluation order indeed depends on the order of registration, and the order of registration can be controlled with checker dependencies.

In fact, can we make a silent assumption that everything depends on core? If so we could eliminate all checks for undefined values in PreStmt and PreCall.

I wouldn't be comfortable doing that before we can separate the modeling portions from the diagnostics.

[cfe-dev] [analyzer][RFC] Our stance on checker dependencies and disabling core checkers: http://lists.llvm.org/pipermail/cfe-dev/2019-August/063070.html

[Szelethus]

Oh, yea, LGTM! Thanks!

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

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());
+      if (V.isUndef())
+        return State;
+
       DefinedOrUnknownSVal L = V.castAs<DefinedOrUnknownSVal>();
       if (!L.getAs<Loc>())
         return State;
 
       return State->assume(L, CannotBeNull);
     }
 
     ValueConstraintPtr negate() const override {

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

void test_fread_fwrite(FILE *fp, int *buf) {
 
   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}}
 }
 
+void test_fread_uninitialized(void) {
+  void *ptr;
+  size_t sz;
+  size_t nmem;
+  FILE *fp;
+  (void)fread(ptr, sz, nmem, fp); // expected-warning {{1st function call argument is an uninitialized value}}
+}
+
 ssize_t getline(char **, size_t *, FILE *);
 void test_getline(FILE *fp) {
   char *line = 0;
   size_t n = 0;
   ssize_t len;
   while ((len = getline(&line, &n, fp)) != -1) {
     clang_analyzer_eval(len == 0); // expected-warning{{FALSE}}
   }