[analyzer] Fix crash of StreamChecker when eval calling 'fopen' (#100990)

Actually, on the failure branch of `fopen`, the resulting pointer could
alias with `stdout` iff `stdout` is already known to be null.
We crashed in this case as the implementation assumed that the
state-split for creating the success and failure branches both should be
viable; thus dereferenced both of those states - leading to the crash.

To fix this, let's just only add this no-alias property for the success
path, and that's it :)

Fixes #100901

Author: steakhal

clang/docs/analyzer/checkers.rst

@@ -1703,7 +1703,13 @@ are detected:
 * Invalid 3rd ("``whence``") argument to ``fseek``.
 
 The stream operations are by this checker usually split into two cases, a success
-and a failure case. However, in the case of write operations (like ``fwrite``,
+and a failure case.
+On the success case it also assumes that the current value of ``stdout``,
+``stderr``, or ``stdin`` can't be equal to the file pointer returned by ``fopen``.
+Operations performed on ``stdout``, ``stderr``, or ``stdin`` are not checked by
+this checker in contrast to the streams opened by ``fopen``.
+
+In the case of write operations (like ``fwrite``,
 ``fprintf`` and even ``fsetpos``) this behavior could produce a large amount of
 unwanted reports on projects that don't have error checks around the write
 operations, so by default the checker assumes that write operations always succeed.
@@ -1769,9 +1775,7 @@ are assumed to succeed.)
 **Limitations**
 
 The checker does not track the correspondence between integer file descriptors
-and ``FILE *`` pointers. Operations on standard streams like ``stdin`` are not
-treated specially and are therefore often not recognized (because these streams
-are usually not opened explicitly by the program, and are global variables).
+and ``FILE *`` pointers.
 
 .. _osx-checkers:
 

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

@@ -615,30 +615,22 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
     });
   }
 
-  void initMacroValues(CheckerContext &C) const {
+  void initMacroValues(const Preprocessor &PP) const {
     if (EofVal)
       return;
 
-    if (const std::optional<int> OptInt =
-            tryExpandAsInteger("EOF", C.getPreprocessor()))
+    if (const std::optional<int> OptInt = tryExpandAsInteger("EOF", PP))
       EofVal = *OptInt;
     else
       EofVal = -1;
-    if (const std::optional<int> OptInt =
-            tryExpandAsInteger("SEEK_SET", C.getPreprocessor()))
+    if (const std::optional<int> OptInt = tryExpandAsInteger("SEEK_SET", PP))
       SeekSetVal = *OptInt;
-    if (const std::optional<int> OptInt =
-            tryExpandAsInteger("SEEK_END", C.getPreprocessor()))
+    if (const std::optional<int> OptInt = tryExpandAsInteger("SEEK_END", PP))
       SeekEndVal = *OptInt;
-    if (const std::optional<int> OptInt =
-            tryExpandAsInteger("SEEK_CUR", C.getPreprocessor()))
+    if (const std::optional<int> OptInt = tryExpandAsInteger("SEEK_CUR", PP))
       SeekCurVal = *OptInt;
   }
 
-  void initVaListType(CheckerContext &C) const {
-    VaListType = C.getASTContext().getBuiltinVaListType().getCanonicalType();
-  }
-
   /// Searches for the ExplodedNode where the file descriptor was acquired for
   /// StreamSym.
   static const ExplodedNode *getAcquisitionSite(const ExplodedNode *N,
@@ -880,9 +872,6 @@ static ProgramStateRef escapeArgs(ProgramStateRef State, CheckerContext &C,
 
 void StreamChecker::checkPreCall(const CallEvent &Call,
                                  CheckerContext &C) const {
-  initMacroValues(C);
-  initVaListType(C);
-
   const FnDescription *Desc = lookupFn(Call);
   if (!Desc || !Desc->PreFn)
     return;
@@ -938,7 +927,6 @@ void StreamChecker::evalFopen(const FnDescription *Desc, const CallEvent &Call,
   assert(RetSym && "RetVal must be a symbol here.");
 
   State = State->BindExpr(CE, C.getLocationContext(), RetVal);
-  State = assumeNoAliasingWithStdStreams(State, RetVal, C);
 
   // Bifurcate the state into two: one with a valid FILE* pointer, the other
   // with a NULL.
@@ -951,6 +939,8 @@ void StreamChecker::evalFopen(const FnDescription *Desc, const CallEvent &Call,
   StateNull =
       StateNull->set<StreamMap>(RetSym, StreamState::getOpenFailed(Desc));
 
+  StateNotNull = assumeNoAliasingWithStdStreams(StateNotNull, RetVal, C);
+
   C.addTransition(StateNotNull,
                   constructLeakNoteTag(C, RetSym, "Stream opened here"));
   C.addTransition(StateNull);
@@ -2081,10 +2071,12 @@ getGlobalStreamPointerByName(const TranslationUnitDecl *TU, StringRef VarName) {
 }
 
 void StreamChecker::checkASTDecl(const TranslationUnitDecl *TU,
-                                 AnalysisManager &, BugReporter &) const {
+                                 AnalysisManager &Mgr, BugReporter &) const {
   StdinDecl = getGlobalStreamPointerByName(TU, "stdin");
   StdoutDecl = getGlobalStreamPointerByName(TU, "stdout");
   StderrDecl = getGlobalStreamPointerByName(TU, "stderr");
+  VaListType = TU->getASTContext().getBuiltinVaListType().getCanonicalType();
+  initMacroValues(Mgr.getPreprocessor());
 }
 
 //===----------------------------------------------------------------------===//

clang/test/Analysis/stream.c

@@ -1,11 +1,11 @@
 // RUN: %clang_analyze_cc1 -triple=x86_64-pc-linux-gnu -analyzer-checker=core,unix.Stream,debug.ExprInspection \
-// RUN:   -analyzer-config unix.Stream:Pedantic=true -verify %s
+// RUN:   -analyzer-config eagerly-assume=false,unix.Stream:Pedantic=true -verify %s
 // RUN: %clang_analyze_cc1 -triple=armv8-none-linux-eabi -analyzer-checker=core,unix.Stream,debug.ExprInspection \
-// RUN:   -analyzer-config unix.Stream:Pedantic=true -verify %s
+// RUN:   -analyzer-config eagerly-assume=false,unix.Stream:Pedantic=true -verify %s
 // RUN: %clang_analyze_cc1 -triple=aarch64-linux-gnu -analyzer-checker=core,unix.Stream,debug.ExprInspection \
-// RUN:   -analyzer-config unix.Stream:Pedantic=true -verify %s
+// RUN:   -analyzer-config eagerly-assume=false,unix.Stream:Pedantic=true -verify %s
 // RUN: %clang_analyze_cc1 -triple=hexagon -analyzer-checker=core,unix.Stream,debug.ExprInspection \
-// RUN:   -analyzer-config unix.Stream:Pedantic=true -verify %s
+// RUN:   -analyzer-config eagerly-assume=false,unix.Stream:Pedantic=true -verify %s
 
 #include "Inputs/system-header-simulator.h"
 #include "Inputs/system-header-simulator-for-malloc.h"
@@ -499,14 +499,34 @@ void gh_93408_regression_ZeroSized(struct ZeroSized *buffer) {
   fclose(f);
 }
 
-extern FILE *stdout_like_ptr;
-void no_aliasing(void) {
+extern FILE *non_standard_stream_ptr;
+void test_fopen_does_not_alias_with_standard_streams(void) {
   FILE *f = fopen("file", "r");
-  clang_analyzer_eval(f == stdin);           // expected-warning {{FALSE}} no-TRUE
-  clang_analyzer_eval(f == stdout);          // expected-warning {{FALSE}} no-TRUE
-  clang_analyzer_eval(f == stderr);          // expected-warning {{FALSE}} no-TRUE
-  clang_analyzer_eval(f == stdout_like_ptr); // expected-warning {{FALSE}} expected-warning {{TRUE}}
-  if (f && f != stdout) {
+  if (!f) return;
+  clang_analyzer_eval(f == stdin);  // expected-warning {{FALSE}} no-TRUE
+  clang_analyzer_eval(f == stdout); // expected-warning {{FALSE}} no-TRUE
+  clang_analyzer_eval(f == stderr); // expected-warning {{FALSE}} no-TRUE
+  clang_analyzer_eval(f == non_standard_stream_ptr); // expected-warning {{UNKNOWN}}
+  if (f != stdout) {
     fclose(f);
   }
 } // no-leak: 'fclose()' is always called because 'f' cannot be 'stdout'.
+
+void reopen_std_stream(void) {
+  FILE *oldStdout = stdout;
+  fclose(stdout);
+  FILE *fp = fopen("blah", "w");
+  if (!fp) return;
+
+  stdout = fp; // Let's make them alias.
+  clang_analyzer_eval(fp == oldStdout);     // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(fp == stdout);        // expected-warning {{TRUE}} no-FALSE
+  clang_analyzer_eval(oldStdout == stdout); // expected-warning {{UNKNOWN}}
+}
+
+void only_success_path_does_not_alias_with_stdout(void) {
+  if (stdout) return;
+  FILE *f = fopen("/tmp/foof", "r"); // no-crash
+  if (!f) return;
+  fclose(f);
+}