[clang][analyzer] Change modeling of 'fileno' in checkers. (#81842)

Function 'fileno' fails only if invalid pointer is passed, this is a
case that is often ignored in source code. The failure case leads to
many "false positive" reports when `fileno` returns -1 and this is not
checked in the program. Because this, the function is now assumed
to not fail (this is assumption that the passed file pointer is correct).
The change affects `StdCLibraryFunctionsChecker` and
`StreamChecker`.

7 files changed, 159 insertions, 121 deletions

diff --git a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
index 6b8ac26..6cc8867 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -2388,12 +2388,15 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
             .ArgConstraint(NotNull(ArgNo(0))));
 
     // int fileno(FILE *stream);
+    // According to POSIX 'fileno' may fail and set 'errno'.
+    // But in Linux it may fail only if the specified file pointer is invalid.
+    // At many places 'fileno' is used without check for failure and a failure
+    // case here would produce a large amount of likely false positive warnings.
+    // To avoid this, we assume here that it does not fail.
     addToFunctionSummaryMap(
         "fileno", Signature(ArgTypes{FilePtrTy}, RetType{IntTy}),
         Summary(NoEvalCall)
-            .Case(ReturnsValidFileDescriptor, ErrnoMustNotBeChecked,
-                  GenericSuccessMsg)
-            .Case(ReturnsMinusOne, ErrnoNEZeroIrrelevant, GenericFailureMsg)
+            .Case(ReturnsValidFileDescriptor, ErrnoUnchanged, GenericSuccessMsg)
             .ArgConstraint(NotNull(ArgNo(0))));
 
     // void rewind(FILE *stream);
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 7e7e3f0..a070f45 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -249,6 +249,10 @@ struct StreamOperationEvaluator {
 
   bool isStreamEof() const { return SS->ErrorState == ErrorFEof; }
 
+  NonLoc getZeroVal(const CallEvent &Call) {
+    return *SVB.makeZeroVal(Call.getResultType()).getAs<NonLoc>();
+  }
+
   ProgramStateRef setStreamState(ProgramStateRef State,
                                  const StreamState &NewSS) {
     return State->set<StreamMap>(StreamSym, NewSS);
@@ -390,7 +394,8 @@ private:
        {&StreamChecker::preDefault,
         std::bind(&StreamChecker::evalFeofFerror, _1, _2, _3, _4, ErrorFError),
         0}},
-      {{{"fileno"}, 1}, {&StreamChecker::preDefault, nullptr, 0}},
+      {{{"fileno"}, 1},
+       {&StreamChecker::preDefault, &StreamChecker::evalFileno, 0}},
   };
 
   CallDescriptionMap<FnDescription> FnTestDescriptions = {
@@ -486,6 +491,9 @@ private:
   void evalFflush(const FnDescription *Desc, const CallEvent &Call,
                   CheckerContext &C) const;
 
+  void evalFileno(const FnDescription *Desc, const CallEvent &Call,
+                  CheckerContext &C) const;
+
   /// Check that the stream (in StreamVal) is not NULL.
   /// 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
@@ -929,8 +937,7 @@ void StreamChecker::evalFputx(const FnDescription *Desc, const CallEvent &Call,
     ProgramStateRef StateNotFailed =
         State->BindExpr(E.CE, C.getLocationContext(), RetVal);
     StateNotFailed =
-        E.assumeBinOpNN(StateNotFailed, BO_GE, RetVal,
-                        *E.SVB.makeZeroVal(E.ACtx.IntTy).getAs<NonLoc>());
+        E.assumeBinOpNN(StateNotFailed, BO_GE, RetVal, E.getZeroVal(Call));
     if (!StateNotFailed)
       return;
     StateNotFailed =
@@ -1003,8 +1010,7 @@ void StreamChecker::evalFscanf(const FnDescription *Desc, const CallEvent &Call,
     ProgramStateRef StateNotFailed =
         State->BindExpr(E.CE, C.getLocationContext(), RetVal);
     StateNotFailed =
-        E.assumeBinOpNN(StateNotFailed, BO_GE, RetVal,
-                        *E.SVB.makeZeroVal(E.ACtx.IntTy).getAs<NonLoc>());
+        E.assumeBinOpNN(StateNotFailed, BO_GE, RetVal, E.getZeroVal(Call));
     if (StateNotFailed)
       C.addTransition(StateNotFailed);
   }
@@ -1073,8 +1079,7 @@ void StreamChecker::evalGetdelim(const FnDescription *Desc,
     ProgramStateRef StateNotFailed =
         State->BindExpr(E.CE, C.getLocationContext(), RetVal);
     StateNotFailed =
-        E.assumeBinOpNN(StateNotFailed, BO_GE, RetVal,
-                        *E.SVB.makeZeroVal(E.CE->getType()).getAs<NonLoc>());
+        E.assumeBinOpNN(StateNotFailed, BO_GE, RetVal, E.getZeroVal(Call));
     if (!StateNotFailed)
       return;
     C.addTransition(StateNotFailed);
@@ -1200,8 +1205,7 @@ void StreamChecker::evalFtell(const FnDescription *Desc, const CallEvent &Call,
   ProgramStateRef StateNotFailed =
       State->BindExpr(E.CE, C.getLocationContext(), RetVal);
   StateNotFailed =
-      E.assumeBinOpNN(StateNotFailed, BO_GE, RetVal,
-                      *E.SVB.makeZeroVal(Call.getResultType()).getAs<NonLoc>());
+      E.assumeBinOpNN(StateNotFailed, BO_GE, RetVal, E.getZeroVal(Call));
   if (!StateNotFailed)
     return;
 
@@ -1226,79 +1230,6 @@ void StreamChecker::evalRewind(const FnDescription *Desc, const CallEvent &Call,
   C.addTransition(State);
 }
 
-void StreamChecker::evalClearerr(const FnDescription *Desc,
-                                 const CallEvent &Call,
-                                 CheckerContext &C) const {
-  ProgramStateRef State = C.getState();
-  StreamOperationEvaluator E(C);
-  if (!E.Init(Desc, Call, C, State))
-    return;
-
-  // FilePositionIndeterminate is not cleared.
-  State = E.setStreamState(
-      State,
-      StreamState::getOpened(Desc, ErrorNone, E.SS->FilePositionIndeterminate));
-  C.addTransition(State);
-}
-
-void StreamChecker::evalFeofFerror(const FnDescription *Desc,
-                                   const CallEvent &Call, CheckerContext &C,
-                                   const StreamErrorState &ErrorKind) const {
-  ProgramStateRef State = C.getState();
-  StreamOperationEvaluator E(C);
-  if (!E.Init(Desc, Call, C, State))
-    return;
-
-  if (E.SS->ErrorState & ErrorKind) {
-    // Execution path with error of ErrorKind.
-    // Function returns true.
-    // From now on it is the only one error state.
-    ProgramStateRef TrueState = bindAndAssumeTrue(State, C, E.CE);
-    C.addTransition(E.setStreamState(
-        TrueState, StreamState::getOpened(Desc, ErrorKind,
-                                          E.SS->FilePositionIndeterminate &&
-                                              !ErrorKind.isFEof())));
-  }
-  if (StreamErrorState NewES = E.SS->ErrorState & (~ErrorKind)) {
-    // Execution path(s) with ErrorKind not set.
-    // Function returns false.
-    // New error state is everything before minus ErrorKind.
-    ProgramStateRef FalseState = E.bindReturnValue(State, C, 0);
-    C.addTransition(E.setStreamState(
-        FalseState,
-        StreamState::getOpened(
-            Desc, NewES, E.SS->FilePositionIndeterminate && !NewES.isFEof())));
-  }
-}
-
-void StreamChecker::preDefault(const FnDescription *Desc, const CallEvent &Call,
-                               CheckerContext &C) const {
-  ProgramStateRef State = C.getState();
-  SVal StreamVal = getStreamArg(Desc, Call);
-  State = ensureStreamNonNull(StreamVal, Call.getArgExpr(Desc->StreamArgNo), C,
-                              State);
-  if (!State)
-    return;
-  State = ensureStreamOpened(StreamVal, C, State);
-  if (!State)
-    return;
-
-  C.addTransition(State);
-}
-
-void StreamChecker::evalSetFeofFerror(const FnDescription *Desc,
-                                      const CallEvent &Call, CheckerContext &C,
-                                      const StreamErrorState &ErrorKind) const {
-  ProgramStateRef State = C.getState();
-  SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol();
-  assert(StreamSym && "Operation not permitted on non-symbolic stream value.");
-  const StreamState *SS = State->get<StreamMap>(StreamSym);
-  assert(SS && "Stream should be tracked by the checker.");
-  State = State->set<StreamMap>(
-      StreamSym, StreamState::getOpened(SS->LastOperation, ErrorKind));
-  C.addTransition(State);
-}
-
 void StreamChecker::preFflush(const FnDescription *Desc, const CallEvent &Call,
                               CheckerContext &C) const {
   ProgramStateRef State = C.getState();
@@ -1377,6 +1308,104 @@ void StreamChecker::evalFflush(const FnDescription *Desc, const CallEvent &Call,
   C.addTransition(StateFailed);
 }
 
+void StreamChecker::evalClearerr(const FnDescription *Desc,
+                                 const CallEvent &Call,
+                                 CheckerContext &C) const {
+  ProgramStateRef State = C.getState();
+  StreamOperationEvaluator E(C);
+  if (!E.Init(Desc, Call, C, State))
+    return;
+
+  // FilePositionIndeterminate is not cleared.
+  State = E.setStreamState(
+      State,
+      StreamState::getOpened(Desc, ErrorNone, E.SS->FilePositionIndeterminate));
+  C.addTransition(State);
+}
+
+void StreamChecker::evalFeofFerror(const FnDescription *Desc,
+                                   const CallEvent &Call, CheckerContext &C,
+                                   const StreamErrorState &ErrorKind) const {
+  ProgramStateRef State = C.getState();
+  StreamOperationEvaluator E(C);
+  if (!E.Init(Desc, Call, C, State))
+    return;
+
+  if (E.SS->ErrorState & ErrorKind) {
+    // Execution path with error of ErrorKind.
+    // Function returns true.
+    // From now on it is the only one error state.
+    ProgramStateRef TrueState = bindAndAssumeTrue(State, C, E.CE);
+    C.addTransition(E.setStreamState(
+        TrueState, StreamState::getOpened(Desc, ErrorKind,
+                                          E.SS->FilePositionIndeterminate &&
+                                              !ErrorKind.isFEof())));
+  }
+  if (StreamErrorState NewES = E.SS->ErrorState & (~ErrorKind)) {
+    // Execution path(s) with ErrorKind not set.
+    // Function returns false.
+    // New error state is everything before minus ErrorKind.
+    ProgramStateRef FalseState = E.bindReturnValue(State, C, 0);
+    C.addTransition(E.setStreamState(
+        FalseState,
+        StreamState::getOpened(
+            Desc, NewES, E.SS->FilePositionIndeterminate && !NewES.isFEof())));
+  }
+}
+
+void StreamChecker::evalFileno(const FnDescription *Desc, const CallEvent &Call,
+                               CheckerContext &C) const {
+  // Fileno should fail only if the passed pointer is invalid.
+  // Some of the preconditions are checked already in preDefault.
+  // Here we can assume that the operation does not fail, because if we
+  // introduced a separate branch where fileno() returns -1, then it would cause
+  // many unexpected and unwanted warnings in situations where fileno() is
+  // called on valid streams.
+  // The stream error states are not modified by 'fileno', and 'errno' is also
+  // left unchanged (so this evalCall does not invalidate it, but we have a
+  // custom evalCall instead of the default that would invalidate it).
+  ProgramStateRef State = C.getState();
+  StreamOperationEvaluator E(C);
+  if (!E.Init(Desc, Call, C, State))
+    return;
+
+  NonLoc RetVal = makeRetVal(C, E.CE).castAs<NonLoc>();
+  State = State->BindExpr(E.CE, C.getLocationContext(), RetVal);
+  State = E.assumeBinOpNN(State, BO_GE, RetVal, E.getZeroVal(Call));
+  if (!State)
+    return;
+
+  C.addTransition(State);
+}
+
+void StreamChecker::preDefault(const FnDescription *Desc, const CallEvent &Call,
+                               CheckerContext &C) const {
+  ProgramStateRef State = C.getState();
+  SVal StreamVal = getStreamArg(Desc, Call);
+  State = ensureStreamNonNull(StreamVal, Call.getArgExpr(Desc->StreamArgNo), C,
+                              State);
+  if (!State)
+    return;
+  State = ensureStreamOpened(StreamVal, C, State);
+  if (!State)
+    return;
+
+  C.addTransition(State);
+}
+
+void StreamChecker::evalSetFeofFerror(const FnDescription *Desc,
+                                      const CallEvent &Call, CheckerContext &C,
+                                      const StreamErrorState &ErrorKind) const {
+  ProgramStateRef State = C.getState();
+  SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol();
+  assert(StreamSym && "Operation not permitted on non-symbolic stream value.");
+  const StreamState *SS = State->get<StreamMap>(StreamSym);
+  assert(SS && "Stream should be tracked by the checker.");
+  State = State->set<StreamMap>(
+      StreamSym, StreamState::getOpened(SS->LastOperation, ErrorKind));
+  C.addTransition(State);
+}
+
 ProgramStateRef
 StreamChecker::ensureStreamNonNull(SVal StreamVal, const Expr *StreamE,
                                    CheckerContext &C,
diff --git a/clang/test/Analysis/std-c-library-functions-path-notes.c b/clang/test/Analysis/std-c-library-functions-path-notes.c
index 4df00fe..6449b71 100644
--- a/clang/test/Analysis/std-c-library-functions-path-notes.c
+++ b/clang/test/Analysis/std-c-library-functions-path-notes.c
@@ -61,24 +61,22 @@ int test_islower(int *x) {
 }
 
 int test_bugpath_notes(FILE *f1, char c, FILE *f2) {
-  int f = fileno(f2);
-  if (f == -1) // \
+  // This test has the purpose of checking that notes appear at correct place.
+  long a = ftell(f2); // no note
+  if (a == -1) // \
     // expected-note{{Taking false branch}}
-    return 0;
-  int l = islower(c);
-  f = fileno(f1); // \
-  // expected-note{{Value assigned to 'f'}} \
-  // expected-note{{Assuming that 'fileno' fails}}
-  return dup(f); // \
+    return -1;
+  int l = islower(c); // no note
+  a = ftell(f1); // \
+  // expected-note{{Value assigned to 'a'}} \
+  // expected-note{{Assuming that 'ftell' fails}}
+  return dup(a); // \
   // expected-warning{{The 1st argument to 'dup' is -1 but should be >= 0}} \
   // expected-note{{The 1st argument to 'dup' is -1 but should be >= 0}}
 }
 
 int test_fileno_arg_note(FILE *f1) {
-  return dup(fileno(f1)); // \
-  // expected-warning{{The 1st argument to 'dup' is < 0 but should be >= 0}} \
-  // expected-note{{The 1st argument to 'dup' is < 0 but should be >= 0}} \
-  // expected-note{{Assuming that 'fileno' fails}}
+  return dup(fileno(f1)); // no warning
 }
 
 int test_readlink_bufsize_zero(char *Buf, size_t Bufsize) {
diff --git a/clang/test/Analysis/stream-errno-note.c b/clang/test/Analysis/stream-errno-note.c
index 2531e26..2411a2d 100644
--- a/clang/test/Analysis/stream-errno-note.c
+++ b/clang/test/Analysis/stream-errno-note.c
@@ -141,16 +141,8 @@ void check_rewind_errnocheck(void) {
 }
 
 void check_fileno(void) {
-  FILE *F = tmpfile();
-  // expected-note@+2{{'F' is non-null}}
-  // expected-note@+1{{Taking false branch}}
-  if (!F)
-    return;
-  fileno(F);
-  // expected-note@-1{{Assuming that 'fileno' is successful; 'errno' becomes undefined after the call}}
-  if (errno) {} // expected-warning{{An undefined value may be read from 'errno'}}
-  // expected-note@-1{{An undefined value may be read from 'errno'}}
-  (void)fclose(F);
+  // nothing to check: checker assumes that 'fileno' is always successful
+  // (and does not change 'errno')
 }
 
 void check_fwrite_zeroarg(size_t Siz) {
diff --git a/clang/test/Analysis/stream-errno.c b/clang/test/Analysis/stream-errno.c
index fab6a58..5f0a580 100644
--- a/clang/test/Analysis/stream-errno.c
+++ b/clang/test/Analysis/stream-errno.c
@@ -173,6 +173,8 @@ void check_no_errno_change(void) {
   if (errno) {} // no-warning
   ferror(F);
   if (errno) {} // no-warning
+  fileno(F);
+  if (errno) {} // no-warning
   clang_analyzer_eval(errno == 1); // expected-warning{{TRUE}}
   fclose(F);
 }
@@ -250,20 +252,6 @@ void check_rewind(void) {
   fclose(F);
 }
 
-void check_fileno(void) {
-  FILE *F = tmpfile();
-  if (!F)
-    return;
-  int N = fileno(F);
-  if (N == -1) {
-    clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}}
-    if (errno) {} // no-warning
-    fclose(F);
-    return;
-  }
-  if (errno) {} // expected-warning{{An undefined value may be read from 'errno'}}
-}
-
 void check_fflush_opened_file(void) {
   FILE *F = tmpfile();
   if (!F)
diff --git a/clang/test/Analysis/stream-error.c b/clang/test/Analysis/stream-error.c
index 4bab075..ac31083b 100644
--- a/clang/test/Analysis/stream-error.c
+++ b/clang/test/Analysis/stream-error.c
@@ -491,6 +491,24 @@ void error_ftello(void) {
   fclose(F);
 }
 
+void error_fileno(void) {
+  FILE *F = fopen("file", "r");
+  if (!F)
+    return;
+  int N = fileno(F);
+  clang_analyzer_eval(N >= 0); // expected-warning {{TRUE}}
+  clang_analyzer_eval(feof(F) && ferror(F)); // expected-warning {{FALSE}}
+  StreamTesterChecker_make_feof_stream(F);
+  N = fileno(F);
+  clang_analyzer_eval(feof(F));              // expected-warning {{TRUE}}
+  clang_analyzer_eval(ferror(F));            // expected-warning {{FALSE}}
+  StreamTesterChecker_make_ferror_stream(F);
+  N = fileno(F);
+  clang_analyzer_eval(feof(F));              // expected-warning {{FALSE}}
+  clang_analyzer_eval(ferror(F));            // expected-warning {{TRUE}}
+  fclose(F);
+}
+
 void error_fflush_on_non_null_stream_clear_error_states(void) {
   FILE *F0 = tmpfile(), *F1 = tmpfile();
   // `fflush` clears a non-EOF stream's error state.
diff --git a/clang/test/Analysis/stream-noopen.c b/clang/test/Analysis/stream-noopen.c
index 8bd01a9..644c699 100644
--- a/clang/test/Analysis/stream-noopen.c
+++ b/clang/test/Analysis/stream-noopen.c
@@ -268,6 +268,16 @@ void test_clearerr(FILE *F) {
                                    // expected-warning@-1{{FALSE}}
 }
 
+void test_fileno(FILE *F) {
+  errno = 0;
+  int A = fileno(F);
+  clang_analyzer_eval(F != NULL);  // expected-warning{{TRUE}}
+  clang_analyzer_eval(A >= 0);     // expected-warning{{TRUE}}
+  if (errno) {}                    // no-warning
+  clang_analyzer_eval(errno == 0); // expected-warning{{TRUE}}
+                                   // expected-warning@-1{{FALSE}}
+}
+
 void freadwrite_zerosize(FILE *F) {
   fwrite(WBuf, 1, 0, F);
   clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}