[clang][analyzer] Add "pedantic" mode to StreamChecker. (#87322)

The checker may create failure branches for all stream write operations
only if the new option "pedantic" is set to true.
Result of the write operations is often not checked in typical code. If
failure branches are created the checker will warn for unchecked write
operations and generate a lot of "false positives" (these are valid
warnings but the programmer does not care about this problem).

Author: balazske

clang/docs/analyzer/checkers.rst

@@ -3138,10 +3138,16 @@ are detected:
   allowed in this state.
 * Invalid 3rd ("``whence``") argument to ``fseek``.
 
-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).
+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``,
+``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.
+This behavior can be controlled by the ``Pedantic`` flag: With
+``-analyzer-config alpha.unix.Stream:Pedantic=true`` the checker will model the
+cases where a write operation fails and report situations where this leads to
+erroneous behavior. (The default is ``Pedantic=false``, where write operations
+are assumed to succeed.)
 
 .. code-block:: c
 
@@ -3196,6 +3202,13 @@ are usually not opened explicitly by the program, and are global variables).
    fclose(p);
  }
 
+**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).
+
 .. _alpha-unix-cstring-BufferOverlap:
 
 alpha.unix.cstring.BufferOverlap (C)

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

@@ -604,6 +604,15 @@ def PthreadLockChecker : Checker<"PthreadLock">,
 def StreamChecker : Checker<"Stream">,
   HelpText<"Check stream handling functions">,
   WeakDependencies<[NonNullParamChecker]>,
+  CheckerOptions<[
+    CmdLineOption<Boolean,
+                  "Pedantic",
+                  "If false, assume that stream operations which are often not "
+                  "checked for error do not fail."
+                  "fail.",
+                  "false",
+                  InAlpha>
+  ]>,
   Documentation<HasDocumentation>;
 
 def SimpleStreamChecker : Checker<"SimpleStream">,

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

@@ -297,6 +297,9 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
   /// If true, evaluate special testing stream functions.
   bool TestMode = false;
 
+  /// If true, generate failure branches for cases that are often not checked.
+  bool PedanticMode = false;
+
 private:
   CallDescriptionMap<FnDescription> FnDescriptions = {
       {{{"fopen"}, 2}, {nullptr, &StreamChecker::evalFopen, ArgNone}},
@@ -945,6 +948,10 @@ void StreamChecker::evalFreadFwrite(const FnDescription *Desc,
   }
 
   // Add transition for the failed state.
+  // At write, add failure case only if "pedantic mode" is on.
+  if (!IsFread && !PedanticMode)
+    return;
+
   NonLoc RetVal = makeRetVal(C, E.CE).castAs<NonLoc>();
   ProgramStateRef StateFailed =
       State->BindExpr(E.CE, C.getLocationContext(), RetVal);
@@ -1057,6 +1064,9 @@ void StreamChecker::evalFputx(const FnDescription *Desc, const CallEvent &Call,
     C.addTransition(StateNotFailed);
   }
 
+  if (!PedanticMode)
+    return;
+
   // Add transition for the failed state. The resulting value of the file
   // position indicator for the stream is indeterminate.
   ProgramStateRef StateFailed = E.bindReturnValue(State, C, *EofVal);
@@ -1092,6 +1102,9 @@ void StreamChecker::evalFprintf(const FnDescription *Desc,
       E.setStreamState(StateNotFailed, StreamState::getOpened(Desc));
   C.addTransition(StateNotFailed);
 
+  if (!PedanticMode)
+    return;
+
   // Add transition for the failed state. The resulting value of the file
   // position indicator for the stream is indeterminate.
   StateFailed = E.setStreamState(
@@ -1264,21 +1277,23 @@ void StreamChecker::evalFseek(const FnDescription *Desc, const CallEvent &Call,
   if (!E.Init(Desc, Call, C, State))
     return;
 
-  // Bifurcate the state into failed and non-failed.
-  // Return zero on success, -1 on error.
+  // Add success state.
   ProgramStateRef StateNotFailed = E.bindReturnValue(State, C, 0);
-  ProgramStateRef StateFailed = E.bindReturnValue(State, C, -1);
-
   // No failure: Reset the state to opened with no error.
   StateNotFailed =
       E.setStreamState(StateNotFailed, StreamState::getOpened(Desc));
   C.addTransition(StateNotFailed);
 
+  if (!PedanticMode)
+    return;
+
+  // Add failure state.
   // At error it is possible that fseek fails but sets none of the error flags.
   // If fseek failed, assume that the file position becomes indeterminate in any
   // case.
   // It is allowed to set the position beyond the end of the file. EOF error
   // should not occur.
+  ProgramStateRef StateFailed = E.bindReturnValue(State, C, -1);
   StateFailed = E.setStreamState(
       StateFailed, StreamState::getOpened(Desc, ErrorNone | ErrorFError, true));
   C.addTransition(StateFailed, E.getFailureNoteTag(this, C));
@@ -1316,6 +1331,10 @@ void StreamChecker::evalFsetpos(const FnDescription *Desc,
 
   StateNotFailed = E.setStreamState(
       StateNotFailed, StreamState::getOpened(Desc, ErrorNone, false));
+  C.addTransition(StateNotFailed);
+
+  if (!PedanticMode)
+    return;
 
   // At failure ferror could be set.
   // The standards do not tell what happens with the file position at failure.
@@ -1324,7 +1343,6 @@ void StreamChecker::evalFsetpos(const FnDescription *Desc,
   StateFailed = E.setStreamState(
       StateFailed, StreamState::getOpened(Desc, ErrorNone | ErrorFError, true));
 
-  C.addTransition(StateNotFailed);
   C.addTransition(StateFailed, E.getFailureNoteTag(this, C));
 }
 
@@ -1794,7 +1812,9 @@ ProgramStateRef StreamChecker::checkPointerEscape(
 //===----------------------------------------------------------------------===//
 
 void ento::registerStreamChecker(CheckerManager &Mgr) {
-  Mgr.registerChecker<StreamChecker>();
+  auto *Checker = Mgr.registerChecker<StreamChecker>();
+  Checker->PedanticMode =
+      Mgr.getAnalyzerOptions().getCheckerBooleanOption(Checker, "Pedantic");
 }
 
 bool ento::shouldRegisterStreamChecker(const CheckerManager &Mgr) {

clang/test/Analysis/analyzer-config.c

@@ -12,6 +12,7 @@
 // CHECK-NEXT: alpha.security.MmapWriteExec:MmapProtExec = 0x04
 // CHECK-NEXT: alpha.security.MmapWriteExec:MmapProtRead = 0x01
 // CHECK-NEXT: alpha.security.taint.TaintPropagation:Config = ""
+// CHECK-NEXT: alpha.unix.Stream:Pedantic = false
 // CHECK-NEXT: apply-fixits = false
 // CHECK-NEXT: assume-controlled-environment = false
 // CHECK-NEXT: avoid-suppressing-null-argument-paths = false

clang/test/Analysis/std-c-library-functions-vs-stream-checker.c

@@ -1,6 +1,7 @@
 // Check the case when only the StreamChecker is enabled.
 // RUN: %clang_analyze_cc1 %s \
 // RUN:   -analyzer-checker=core,alpha.unix.Stream \
+// RUN:   -analyzer-config alpha.unix.Stream:Pedantic=true \
 // RUN:   -analyzer-checker=debug.ExprInspection \
 // RUN:   -analyzer-config eagerly-assume=false \
 // RUN:   -triple x86_64-unknown-linux \
@@ -19,6 +20,7 @@
 // StdLibraryFunctionsChecker are enabled.
 // RUN: %clang_analyze_cc1 %s \
 // RUN:   -analyzer-checker=core,alpha.unix.Stream \
+// RUN:   -analyzer-config alpha.unix.Stream:Pedantic=true \
 // RUN:   -analyzer-checker=unix.StdCLibraryFunctions \
 // RUN:   -analyzer-config unix.StdCLibraryFunctions:DisplayLoadedSummaries=true \
 // RUN:   -analyzer-checker=debug.ExprInspection \

clang/test/Analysis/stream-errno-note.c

@@ -1,5 +1,6 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=core \
 // RUN:   -analyzer-checker=alpha.unix.Stream \
+// RUN:   -analyzer-config alpha.unix.Stream:Pedantic=true \
 // RUN:   -analyzer-checker=unix.Errno \
 // RUN:   -analyzer-checker=unix.StdCLibraryFunctions \
 // RUN:   -analyzer-config unix.StdCLibraryFunctions:ModelPOSIX=true \

clang/test/Analysis/stream-errno.c

@@ -1,4 +1,5 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.unix.Stream,unix.Errno,unix.StdCLibraryFunctions,debug.ExprInspection \
+// RUN:   -analyzer-config alpha.unix.Stream:Pedantic=true \
 // RUN:   -analyzer-config unix.StdCLibraryFunctions:ModelPOSIX=true -verify %s
 
 #include "Inputs/system-header-simulator.h"

clang/test/Analysis/stream-error.c

@@ -1,6 +1,7 @@
 // RUN: %clang_analyze_cc1 -verify %s \
 // RUN: -analyzer-checker=core \
 // RUN: -analyzer-checker=alpha.unix.Stream \
+// RUN: -analyzer-config alpha.unix.Stream:Pedantic=true \
 // RUN: -analyzer-checker=debug.StreamTester \
 // RUN: -analyzer-checker=debug.ExprInspection
 

clang/test/Analysis/stream-note.c

@@ -1,6 +1,8 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.unix.Stream -analyzer-output text \
+// RUN:   -analyzer-config alpha.unix.Stream:Pedantic=true \
 // RUN:   -verify %s
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.unix.Stream,unix.StdCLibraryFunctions -analyzer-output text \
+// RUN:   -analyzer-config alpha.unix.Stream:Pedantic=true \
 // RUN:   -analyzer-config unix.StdCLibraryFunctions:ModelPOSIX=true -verify=expected,stdargs %s
 
 #include "Inputs/system-header-simulator.h"

clang/test/Analysis/stream-pedantic.c

@@ -0,0 +1,95 @@
+// RUN: %clang_analyze_cc1 -triple=x86_64-pc-linux-gnu -analyzer-checker=core,alpha.unix.Stream,debug.ExprInspection \
+// RUN:   -analyzer-config alpha.unix.Stream:Pedantic=false -verify=nopedantic %s
+
+// RUN: %clang_analyze_cc1 -triple=x86_64-pc-linux-gnu -analyzer-checker=core,alpha.unix.Stream,debug.ExprInspection \
+// RUN:   -analyzer-config alpha.unix.Stream:Pedantic=true -verify=pedantic %s
+
+#include "Inputs/system-header-simulator.h"
+
+void clang_analyzer_eval(int);
+
+void check_fwrite(void) {
+  char *Buf = "123456789";
+  FILE *Fp = tmpfile();
+  if (!Fp)
+    return;
+  size_t Ret = fwrite(Buf, 1, 10, Fp);
+  clang_analyzer_eval(Ret == 0); // nopedantic-warning {{FALSE}} \
+                                 // pedantic-warning {{FALSE}} \
+                                 // pedantic-warning {{TRUE}}
+  fputc('A', Fp); // pedantic-warning {{might be 'indeterminate'}}
+  fclose(Fp);
+}
+
+void check_fputc(void) {
+  FILE *Fp = tmpfile();
+  if (!Fp)
+    return;
+  int Ret = fputc('A', Fp);
+  clang_analyzer_eval(Ret == EOF); // nopedantic-warning {{FALSE}} \
+                                   // pedantic-warning {{FALSE}} \
+                                   // pedantic-warning {{TRUE}}
+  fputc('A', Fp); // pedantic-warning {{might be 'indeterminate'}}
+  fclose(Fp);
+}
+
+void check_fputs(void) {
+  FILE *Fp = tmpfile();
+  if (!Fp)
+    return;
+  int Ret = fputs("ABC", Fp);
+  clang_analyzer_eval(Ret == EOF); // nopedantic-warning {{FALSE}} \
+                                   // pedantic-warning {{FALSE}} \
+                                   // pedantic-warning {{TRUE}}
+  fputc('A', Fp); // pedantic-warning {{might be 'indeterminate'}}
+  fclose(Fp);
+}
+
+void check_fprintf(void) {
+  FILE *Fp = tmpfile();
+  if (!Fp)
+    return;
+  int Ret = fprintf(Fp, "ABC");
+  clang_analyzer_eval(Ret < 0); // nopedantic-warning {{FALSE}} \
+                                // pedantic-warning {{FALSE}} \
+                                // pedantic-warning {{TRUE}}
+  fputc('A', Fp); // pedantic-warning {{might be 'indeterminate'}}
+  fclose(Fp);
+}
+
+void check_fseek(void) {
+  FILE *Fp = tmpfile();
+  if (!Fp)
+    return;
+  int Ret = fseek(Fp, 0, 0);
+  clang_analyzer_eval(Ret == -1); // nopedantic-warning {{FALSE}} \
+                                  // pedantic-warning {{FALSE}} \
+                                  // pedantic-warning {{TRUE}}
+  fputc('A', Fp); // pedantic-warning {{might be 'indeterminate'}}
+  fclose(Fp);
+}
+
+void check_fseeko(void) {
+  FILE *Fp = tmpfile();
+  if (!Fp)
+    return;
+  int Ret = fseeko(Fp, 0, 0);
+  clang_analyzer_eval(Ret == -1); // nopedantic-warning {{FALSE}} \
+                                  // pedantic-warning {{FALSE}} \
+                                  // pedantic-warning {{TRUE}}
+  fputc('A', Fp); // pedantic-warning {{might be 'indeterminate'}}
+  fclose(Fp);
+}
+
+void check_fsetpos(void) {
+  FILE *Fp = tmpfile();
+  if (!Fp)
+    return;
+  fpos_t Pos;
+  int Ret = fsetpos(Fp, &Pos);
+  clang_analyzer_eval(Ret); // nopedantic-warning {{FALSE}} \
+                            // pedantic-warning {{FALSE}} \
+                            // pedantic-warning {{TRUE}}
+  fputc('A', Fp); // pedantic-warning {{might be 'indeterminate'}}
+  fclose(Fp);
+}