[clang][analyzer] Add report of NULL stream to StreamChecker.

[balazske]

Summary

The report of NULL stream was removed in commit 570bf97.
The old reason is not actual any more because the checker dependencies are changed.
It is not good to eliminate a failure state (where the stream is NULL) without
generating a bug report because other checkers are not able to find it later.
The checker did this with the NULL stream pointer, and because this checker
runs now before other checkers that can detect NULL pointers, the null pointer
bug was not found at all.

[steakhal]

Ah yes, finally this sorts itself out.
We also had to revert 570bf972f5adf05438c7e08d693bf4b96bfd510a because we use the Stream checker but not the StdLibFunc checker, so in our case after a rebase suddenly some important reports "disappeared", thus we reverted the cause.
As more and more patches came to the stdlibfunc checker, I worried about maintaining that revert. Finally, this patch will resolve it, so we don't have to do it ourselves.

Thanks!

[steakhal]

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

553

What's the purpose of this hunk?

[balazske]

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

553

The intent was that NonNullParamChecker should find a null pointer problem before StreamChecker and have a fixed priority of the warnings. But NonNullParamChecker probably does not apply to the stream functions because there are no nonnull attributes (this may be possible?) and no references. No tests fail if this line is removed.

[steakhal]

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

553

Okay, I see. Choose as you want. I don't mind.

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

 
 def PthreadLockChecker : Checker<"PthreadLock">,
   HelpText<"Simple lock -> unlock checker">,
   Dependencies<[PthreadLockBase]>,
   Documentation<HasDocumentation>;
 
 def StreamChecker : Checker<"Stream">,
   HelpText<"Check stream handling functions">,
+  WeakDependencies<[NonNullParamChecker]>,
   Documentation<HasDocumentation>;
 
 def SimpleStreamChecker : Checker<"SimpleStream">,
   HelpText<"Check for misuses of stream APIs">,
   Documentation<HasDocumentation>;
 
 def BlockInCriticalSectionChecker : Checker<"BlockInCriticalSection">,
   HelpText<"Check for calls to blocking functions inside a critical section">,
CmdLineOption<Boolean,
                   Hide>,
     CmdLineOption<Boolean,
                   "ModelPOSIX",
                   "If set to true, the checker models additional functions "
                   "from the POSIX standard.",
                   "false",
                   InAlpha>
   ]>,
-  WeakDependencies<[CallAndMessageChecker, NonNullParamChecker]>,
+  WeakDependencies<[CallAndMessageChecker, NonNullParamChecker, StreamChecker]>,
   Documentation<HasDocumentation>;
 
 } // end "alpha.unix"
 
 //===----------------------------------------------------------------------===//
 // C++ checkers.
 //===----------------------------------------------------------------------===//
 

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

 //===-- StreamChecker.cpp -----------------------------------------*- C++ -*--//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
ProgramStateRef bindInt(uint64_t Value, ProgramStateRef State,
                         CheckerContext &C, const CallExpr *CE) {
   State = State->BindExpr(CE, C.getLocationContext(),
                           C.getSValBuilder().makeIntVal(Value, CE->getType()));
   return State;
 }
 
 class StreamChecker : public Checker<check::PreCall, eval::Call,
                                      check::DeadSymbols, check::PointerEscape> {
+  BugType BT_FileNull{this, "NULL stream pointer", "Stream handling error"};
   BugType BT_UseAfterClose{this, "Closed stream", "Stream handling error"};
   BugType BT_UseAfterOpenFailed{this, "Invalid stream",
                                 "Stream handling error"};
   BugType BT_IndeterminatePosition{this, "Invalid stream state",
                                    "Stream handling error"};
   BugType BT_IllegalWhence{this, "Illegal whence argument",
                            "Stream handling error"};
   BugType BT_StreamEof{this, "Stream already in EOF", "Stream handling error"};
void evalFeofFerror(const FnDescription *Desc, const CallEvent &Call,
                       CheckerContext &C,
                       const StreamErrorState &ErrorKind) const;
 
   void evalSetFeofFerror(const FnDescription *Desc, const CallEvent &Call,
                          CheckerContext &C,
                          const StreamErrorState &ErrorKind) const;
 
   /// Check that the stream (in StreamVal) is not NULL.
-  /// If it can only be NULL a sink node is generated and nullptr returned.
+  /// If it can only be NULL a fatal error is emitted and nullptr returned.
   /// Otherwise the return value is a new state where the stream is constrained
   /// to be non-null.
   ProgramStateRef ensureStreamNonNull(SVal StreamVal, const Expr *StreamE,
                                       CheckerContext &C,
                                       ProgramStateRef State) const;
 
   /// Check that the stream is the opened state.
   /// If the stream is known to be not opened an error is generated
if (!Stream)
     return State;
 
   ConstraintManager &CM = C.getConstraintManager();
 
   ProgramStateRef StateNotNull, StateNull;
   std::tie(StateNotNull, StateNull) = CM.assumeDual(C.getState(), *Stream);
 
   if (!StateNotNull && StateNull) {
-    // Stream argument is NULL, stop analysis on this path.
-    // This case should occur only if StdLibraryFunctionsChecker (or ModelPOSIX
-    // option of it) is not turned on, otherwise that checker ensures non-null
-    // argument.
-    C.generateSink(StateNull, C.getPredecessor());
+    if (ExplodedNode *N = C.generateErrorNode(StateNull)) {
+      auto R = std::make_unique<PathSensitiveBugReport>(
+          BT_FileNull, "Stream pointer might be NULL.", N);
+      if (StreamE)
+        bugreporter::trackExpressionValue(N, StreamE, *R);
+      C.emitReport(std::move(R));
+    }
     return nullptr;
   }
 
   return StateNotNull;
 }
 
 ProgramStateRef StreamChecker::ensureStreamOpened(SVal StreamVal,
                                                   CheckerContext &C,

clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c

 // RUN:   2>&1 | FileCheck %s --implicit-check-not=ANALYZE \
 // RUN:                       --implicit-check-not=\.
 
 // CHECK:      OVERVIEW: Clang Static Analyzer Enabled Checkers List
 // CHECK-EMPTY:
 // CHECK-NEXT: core.CallAndMessageModeling
 // CHECK-NEXT: core.CallAndMessage
 // CHECK-NEXT: core.NonNullParamChecker
-// CHECK-NEXT: alpha.unix.StdCLibraryFunctions
 // CHECK-NEXT: alpha.unix.Stream
+// CHECK-NEXT: alpha.unix.StdCLibraryFunctions
 // CHECK-NEXT: apiModeling.Errno
 // CHECK-NEXT: apiModeling.TrustNonnull
 // CHECK-NEXT: apiModeling.TrustReturnsNonnull
 // CHECK-NEXT: apiModeling.llvm.CastValue
 // CHECK-NEXT: apiModeling.llvm.ReturnValue
 // CHECK-NEXT: core.DivideZero
 // CHECK-NEXT: core.DynamicTypePropagation
 // CHECK-NEXT: core.NonnilStringConstants

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

 // Check that the more specific checkers report and not the generic
 // StdCLibraryFunctions checker.
 
 // RUN: %clang_analyze_cc1 %s \
 // RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=alpha.unix.Stream \
 // RUN:   -analyzer-checker=alpha.unix.StdCLibraryFunctions \
 // RUN:   -analyzer-config alpha.unix.StdCLibraryFunctions:ModelPOSIX=true \
 // RUN:   -triple x86_64-unknown-linux-gnu \
 // RUN:   -verify
 
 
 // Make sure that all used functions have their summary loaded.
 
void test_uninit_arg(void) {
   (void)r;
 }
 
 void test_notnull_arg(FILE *F) {
   int *p = 0;
   fread(p, sizeof(int), 5, F); // \
   expected-warning{{Null pointer passed to 1st parameter expecting 'nonnull' [core.NonNullParamChecker]}}
 }
+
+void test_notnull_stream_arg(void) {
+  fileno(0); // \
+  // expected-warning{{Stream pointer might be NULL [alpha.unix.Stream]}}
+}

clang/test/Analysis/stream-note.c

void check_note_leak_2(int c) {
   // expected-warning@-3 {{Opened stream never closed. Potential resource leak}}
   // expected-note@-4 {{Opened stream never closed. Potential resource leak}}
   fclose(F1);
   fclose(F2);
 }
 
 void check_track_null(void) {
   FILE *F;
-  F = fopen("foo1.c", "r"); // stdargs-note {{Value assigned to 'F'}} stdargs-note {{Assuming pointer value is null}}
-  if (F != NULL) {          // stdargs-note {{Taking false branch}} stdargs-note {{'F' is equal to NULL}}
+  F = fopen("foo1.c", "r"); // expected-note {{Value assigned to 'F'}} expected-note {{Assuming pointer value is null}}
+  if (F != NULL) {          // expected-note {{Taking false branch}} expected-note {{'F' is equal to NULL}}
     fclose(F);
     return;
   }
-  fclose(F); // stdargs-warning {{The 1st argument to 'fclose' is NULL but should not be NULL}}
-             // stdargs-note@-1 {{The 1st argument to 'fclose' is NULL but should not be NULL}}
+  fclose(F); // expected-warning {{Stream pointer might be NULL}}
+             // expected-note@-1 {{Stream pointer might be NULL}}
 }
 
 void check_eof_notes_feof_after_feof(void) {
   FILE *F;
   char Buf[10];
   F = fopen("foo1.c", "r");
   if (F == NULL) { // expected-note {{Taking false branch}} expected-note {{'F' is not equal to NULL}}
     return;

clang/test/Analysis/stream-stdlibraryfunctionargs.c

 // RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.unix.Stream,alpha.unix.StdCLibraryFunctions,debug.ExprInspection \
-// RUN:   -analyzer-config alpha.unix.StdCLibraryFunctions:ModelPOSIX=true -verify=stdargs,any %s
+// RUN:   -analyzer-config alpha.unix.StdCLibraryFunctions:ModelPOSIX=true -verify=stream,any %s
 
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.unix.Stream,debug.ExprInspection \
-// RUN:   -analyzer-config alpha.unix.StdCLibraryFunctions:ModelPOSIX=true -verify=any %s
+// RUN:   -analyzer-config alpha.unix.StdCLibraryFunctions:ModelPOSIX=true -verify=stream,any %s
 
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.unix.StdCLibraryFunctions,debug.ExprInspection \
-// RUN:   -analyzer-config alpha.unix.StdCLibraryFunctions:ModelPOSIX=true -verify=stdargs,any %s
+// RUN:   -analyzer-config alpha.unix.StdCLibraryFunctions:ModelPOSIX=true -verify=stdfunc,any %s
 
 #include "Inputs/system-header-simulator.h"
 
 extern void clang_analyzer_eval(int);
 
 void *buf;
 size_t size;
 size_t n;
 
 void test_fopen(void) {
   FILE *fp = fopen("path", "r");
   clang_analyzer_eval(fp != NULL); // any-warning{{TRUE}} any-warning{{FALSE}}
-  fclose(fp); // stdargs-warning{{should not be NULL}}
+  fclose(fp); // \
+  // stream-warning{{Stream pointer might be NULL}} \
+  // stdfunc-warning{{should not be NULL}}
 }
 
 void test_tmpfile(void) {
   FILE *fp = tmpfile();
   clang_analyzer_eval(fp != NULL); // any-warning{{TRUE}} any-warning{{FALSE}}
-  fclose(fp); // stdargs-warning{{should not be NULL}}
+  fclose(fp); // \
+  // stream-warning{{Stream pointer might be NULL}} \
+  // stdfunc-warning{{should not be NULL}}
 }
 
 void test_fclose(void) {
   FILE *fp = tmpfile();
-  fclose(fp); // stdargs-warning{{should not be NULL}}
+  fclose(fp); // \
+  // stream-warning{{Stream pointer might be NULL}} \
+  // stdfunc-warning{{should not be NULL}}
   clang_analyzer_eval(fp != NULL); // any-warning{{TRUE}}
 }
 
 void test_freopen(void) {
   FILE *fp = tmpfile();
-  fp = freopen("file", "w", fp); // stdargs-warning{{should not be NULL}}
-  fclose(fp); // stdargs-warning{{should not be NULL}}
+  fp = freopen("file", "w", fp); // \
+  // stream-warning{{Stream pointer might be NULL}} \
+  // stdfunc-warning{{should not be NULL}}
+  fclose(fp); // \
+  // stream-warning{{Stream pointer might be NULL}} \
+  // stdfunc-warning{{should not be NULL}}
   clang_analyzer_eval(fp != NULL); // any-warning{{TRUE}}
 }
 
 void test_fread(void) {
   FILE *fp = tmpfile();
-  size_t ret = fread(buf, size, n, fp); // stdargs-warning{{The 4th argument to 'fread' is NULL but should not be NULL}}
+  size_t ret = fread(buf, size, n, fp); // \
+  // stream-warning{{Stream pointer might be NULL}} \
+  // stdfunc-warning{{The 4th argument to 'fread' is NULL but should not be NULL}}
   clang_analyzer_eval(fp != NULL); // any-warning{{TRUE}}
   clang_analyzer_eval(ret <= n); // any-warning{{TRUE}}
   clang_analyzer_eval(ret == n); // any-warning{{TRUE}} any-warning{{FALSE}}
 
   fclose(fp);
 }
 
 void test_fwrite(void) {
   FILE *fp = tmpfile();
-  size_t ret = fwrite(buf, size, n, fp); // stdargs-warning{{The 4th argument to 'fwrite' is NULL but should not be NULL}}
+  size_t ret = fwrite(buf, size, n, fp); // \
+  // stream-warning{{Stream pointer might be NULL}} \
+  // stdfunc-warning{{The 4th argument to 'fwrite' is NULL but should not be NULL}}
   clang_analyzer_eval(fp != NULL); // any-warning{{TRUE}}
   clang_analyzer_eval(ret <= n); // any-warning{{TRUE}}
   clang_analyzer_eval(ret == n); // any-warning{{TRUE}} any-warning{{FALSE}}
 
   fclose(fp);
 }
 
 void test_fseek(void) {
   FILE *fp = tmpfile();
-  fseek(fp, 0, 0); // stdargs-warning{{should not be NULL}}
+  fseek(fp, 0, 0); // \
+  // stream-warning{{Stream pointer might be NULL}} \
+  // stdfunc-warning{{should not be NULL}}
   clang_analyzer_eval(fp != NULL); // any-warning{{TRUE}}
   fclose(fp);
 }
 
 void test_ftell(void) {
   FILE *fp = tmpfile();
-  ftell(fp); // stdargs-warning{{should not be NULL}}
+  ftell(fp); // \
+  // stream-warning{{Stream pointer might be NULL}} \
+  // stdfunc-warning{{should not be NULL}}
   clang_analyzer_eval(fp != NULL); // any-warning{{TRUE}}
   fclose(fp);
 }
 
 void test_rewind(void) {
   FILE *fp = tmpfile();
-  rewind(fp); // stdargs-warning{{should not be NULL}}
+  rewind(fp); // \
+  // stream-warning{{Stream pointer might be NULL}} \
+  // stdfunc-warning{{should not be NULL}}
   clang_analyzer_eval(fp != NULL); // any-warning{{TRUE}}
   fclose(fp);
 }
 
 void test_fgetpos(void) {
   FILE *fp = tmpfile();
   fpos_t pos;
-  fgetpos(fp, &pos); // stdargs-warning{{should not be NULL}}
+  fgetpos(fp, &pos); // \
+  // stream-warning{{Stream pointer might be NULL}} \
+  // stdfunc-warning{{should not be NULL}}
   clang_analyzer_eval(fp != NULL); // any-warning{{TRUE}}
   fclose(fp);
 }
 
 void test_fsetpos(void) {
   FILE *fp = tmpfile();
   fpos_t pos;
-  fsetpos(fp, &pos); // stdargs-warning{{should not be NULL}}
+  fsetpos(fp, &pos); // \
+  // stream-warning{{Stream pointer might be NULL}} \
+  // stdfunc-warning{{should not be NULL}}
   clang_analyzer_eval(fp != NULL); // any-warning{{TRUE}}
   fclose(fp);
 }
 
 void test_clearerr(void) {
   FILE *fp = tmpfile();
-  clearerr(fp); // stdargs-warning{{should not be NULL}}
+  clearerr(fp); // \
+  // stream-warning{{Stream pointer might be NULL}} \
+  // stdfunc-warning{{should not be NULL}}
   clang_analyzer_eval(fp != NULL); // any-warning{{TRUE}}
   fclose(fp);
 }
 
 void test_feof(void) {
   FILE *fp = tmpfile();
-  feof(fp); // stdargs-warning{{should not be NULL}}
+  feof(fp); // \
+  // stream-warning{{Stream pointer might be NULL}} \
+  // stdfunc-warning{{should not be NULL}}
   clang_analyzer_eval(fp != NULL); // any-warning{{TRUE}}
   fclose(fp);
 }
 
 void test_ferror(void) {
   FILE *fp = tmpfile();
-  ferror(fp); // stdargs-warning{{should not be NULL}}
+  ferror(fp); // \
+  // stream-warning{{Stream pointer might be NULL}} \
+  // stdfunc-warning{{should not be NULL}}
   clang_analyzer_eval(fp != NULL); // any-warning{{TRUE}}
   fclose(fp);
 }
 
 void test_fileno(void) {
   FILE *fp = tmpfile();
-  fileno(fp); // stdargs-warning{{should not be NULL}}
+  fileno(fp); // \
+  // stream-warning{{Stream pointer might be NULL}} \
+  // stdfunc-warning{{should not be NULL}}
   clang_analyzer_eval(fp != NULL); // any-warning{{TRUE}}
   fclose(fp);
 }

clang/test/Analysis/stream.c

 // RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.unix.Stream -verify %s
 
 #include "Inputs/system-header-simulator.h"
 
+void check_fread(void) {
+  FILE *fp = tmpfile();
+  fread(0, 0, 0, fp); // expected-warning {{Stream pointer might be NULL}}
+  fclose(fp);
+}
+
+void check_fwrite(void) {
+  FILE *fp = tmpfile();
+  fwrite(0, 0, 0, fp); // expected-warning {{Stream pointer might be NULL}}
+  fclose(fp);
+}
+
+void check_fseek(void) {
+  FILE *fp = tmpfile();
+  fseek(fp, 0, 0); // expected-warning {{Stream pointer might be NULL}}
+  fclose(fp);
+}
+
+void check_ftell(void) {
+  FILE *fp = tmpfile();
+  ftell(fp); // expected-warning {{Stream pointer might be NULL}}
+  fclose(fp);
+}
+
+void check_rewind(void) {
+  FILE *fp = tmpfile();
+  rewind(fp); // expected-warning {{Stream pointer might be NULL}}
+  fclose(fp);
+}
+
+void check_fgetpos(void) {
+  FILE *fp = tmpfile();
+  fpos_t pos;
+  fgetpos(fp, &pos); // expected-warning {{Stream pointer might be NULL}}
+  fclose(fp);
+}
+
+void check_fsetpos(void) {
+  FILE *fp = tmpfile();
+  fpos_t pos;
+  fsetpos(fp, &pos); // expected-warning {{Stream pointer might be NULL}}
+  fclose(fp);
+}
+
+void check_clearerr(void) {
+  FILE *fp = tmpfile();
+  clearerr(fp); // expected-warning {{Stream pointer might be NULL}}
+  fclose(fp);
+}
+
+void check_feof(void) {
+  FILE *fp = tmpfile();
+  feof(fp); // expected-warning {{Stream pointer might be NULL}}
+  fclose(fp);
+}
+
+void check_ferror(void) {
+  FILE *fp = tmpfile();
+  ferror(fp); // expected-warning {{Stream pointer might be NULL}}
+  fclose(fp);
+}
+
+void check_fileno(void) {
+  FILE *fp = tmpfile();
+  fileno(fp); // expected-warning {{Stream pointer might be NULL}}
+  fclose(fp);
+}
+
+void f_open(void) {
+  FILE *p = fopen("foo", "r");
+  char buf[1024];
+  fread(buf, 1, 1, p); // expected-warning {{Stream pointer might be NULL}}
+  fclose(p);
+}
+
 void f_seek(void) {
   FILE *p = fopen("foo", "r");
   if (!p)
     return;
   fseek(p, 1, SEEK_SET); // no-warning
   fseek(p, 1, 3); // expected-warning {{The whence argument to fseek() should be SEEK_SET, SEEK_END, or SEEK_CUR}}
   fclose(p);
 }

 }
 
 // PR 8081 - null pointer crash when 'whence' is not an integer constant
 void pr8081(FILE *stream, long offset, int whence) {
   fseek(stream, offset, whence);
 }
 
 void check_freopen_1(void) {
-  FILE *f1 = freopen("foo.c", "r", (FILE *)0); // Not reported by the stream checker.
+  FILE *f1 = freopen("foo.c", "r", (FILE *)0); // expected-warning {{Stream pointer might be NULL}}
   f1 = freopen(0, "w", (FILE *)0x123456);      // Do not report this as error.
 }
 
 void check_freopen_2(void) {
   FILE *f1 = fopen("foo.c", "r");
   if (f1) {
     FILE *f2 = freopen(0, "w", f1);
     if (f2) {