[clang][analyzer] Change modeling of 'fileno' in checkers.

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);

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

@@ -304,7 +304,8 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
        {&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 = {
@@ -400,6 +401,9 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
   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
@@ -1343,6 +1347,84 @@ void StreamChecker::evalRewind(const FnDescription *Desc, const CallEvent &Call,
   C.addTransition(State);
 }
 
+void StreamChecker::preFflush(const FnDescription *Desc, const CallEvent &Call,
+                              CheckerContext &C) const {
+  ProgramStateRef State = C.getState();
+  SVal StreamVal = getStreamArg(Desc, Call);
+  std::optional<DefinedSVal> Stream = StreamVal.getAs<DefinedSVal>();
+  if (!Stream)
+    return;
+
+  ProgramStateRef StateNotNull, StateNull;
+  std::tie(StateNotNull, StateNull) =
+      C.getConstraintManager().assumeDual(State, *Stream);
+  if (StateNotNull && !StateNull)
+    ensureStreamOpened(StreamVal, C, StateNotNull);
+}
+
+void StreamChecker::evalFflush(const FnDescription *Desc, const CallEvent &Call,
+                               CheckerContext &C) const {
+  ProgramStateRef State = C.getState();
+  SVal StreamVal = getStreamArg(Desc, Call);
+  std::optional<DefinedSVal> Stream = StreamVal.getAs<DefinedSVal>();
+  if (!Stream)
+    return;
+
+  // Skip if the stream can be both NULL and non-NULL.
+  ProgramStateRef StateNotNull, StateNull;
+  std::tie(StateNotNull, StateNull) =
+      C.getConstraintManager().assumeDual(State, *Stream);
+  if (StateNotNull && StateNull)
+    return;
+  if (StateNotNull && !StateNull)
+    State = StateNotNull;
+  else
+    State = StateNull;
+
+  const CallExpr *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
+  if (!CE)
+    return;
+
+  // `fflush` returns EOF on failure, otherwise returns 0.
+  ProgramStateRef StateFailed = bindInt(*EofVal, State, C, CE);
+  ProgramStateRef StateNotFailed = bindInt(0, State, C, CE);
+
+  // Clear error states if `fflush` returns 0, but retain their EOF flags.
+  auto ClearErrorInNotFailed = [&StateNotFailed, Desc](SymbolRef Sym,
+                                                       const StreamState *SS) {
+    if (SS->ErrorState & ErrorFError) {
+      StreamErrorState NewES =
+          (SS->ErrorState & ErrorFEof) ? ErrorFEof : ErrorNone;
+      StreamState NewSS = StreamState::getOpened(Desc, NewES, false);
+      StateNotFailed = StateNotFailed->set<StreamMap>(Sym, NewSS);
+    }
+  };
+
+  if (StateNotNull && !StateNull) {
+    // Skip if the input stream's state is unknown, open-failed or closed.
+    if (SymbolRef StreamSym = StreamVal.getAsSymbol()) {
+      const StreamState *SS = State->get<StreamMap>(StreamSym);
+      if (SS) {
+        assert(SS->isOpened() && "Stream is expected to be opened");
+        ClearErrorInNotFailed(StreamSym, SS);
+      } else
+        return;
+    }
+  } else {
+    // Clear error states for all streams.
+    const StreamMapTy &Map = StateNotFailed->get<StreamMap>();
+    for (const auto &I : Map) {
+      SymbolRef Sym = I.first;
+      const StreamState &SS = I.second;
+      if (SS.isOpened())
+        ClearErrorInNotFailed(Sym, &SS);
+    }
+  }
+
+  C.addTransition(StateNotFailed);
+  C.addTransition(StateFailed);
+}
+
 void StreamChecker::evalClearerr(const FnDescription *Desc,
                                  const CallEvent &Call,
                                  CheckerContext &C) const {
@@ -1404,6 +1486,47 @@ void StreamChecker::evalFeofFerror(const FnDescription *Desc,
   }
 }
 
+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.
+  // An added failure case causes many unexpected warnings because a file number
+  // becomes -1 that is not expected by the program.
+  // The stream error states are not modified by 'fileno', and not the 'errno'.
+  // (To ensure that errno is not changed, this evalCall is needed to not
+  // invalidate 'errno' like in a default case.)
+  ProgramStateRef State = C.getState();
+  SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol();
+  if (!StreamSym)
+    return;
+
+  const CallExpr *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
+  if (!CE)
+    return;
+
+  const StreamState *SS = State->get<StreamMap>(StreamSym);
+  if (!SS)
+    return;
+
+  assertStreamStateOpened(SS);
+
+  SValBuilder &SVB = C.getSValBuilder();
+  NonLoc RetVal = makeRetVal(C, CE).castAs<NonLoc>();
+  State = State->BindExpr(CE, C.getLocationContext(), RetVal);
+  auto Cond =
+      SVB.evalBinOp(State, BO_GE, RetVal, SVB.makeZeroVal(Call.getResultType()),
+                    SVB.getConditionType())
+          .getAs<DefinedOrUnknownSVal>();
+  if (!Cond)
+    return;
+  State = State->assume(*Cond, true);
+  if (!State)
+    return;
+
+  C.addTransition(State);
+}
+
 void StreamChecker::preDefault(const FnDescription *Desc, const CallEvent &Call,
                                CheckerContext &C) const {
   ProgramStateRef State = C.getState();
@@ -1432,84 +1555,6 @@ void StreamChecker::evalSetFeofFerror(const FnDescription *Desc,
   C.addTransition(State);
 }
 
-void StreamChecker::preFflush(const FnDescription *Desc, const CallEvent &Call,
-                              CheckerContext &C) const {
-  ProgramStateRef State = C.getState();
-  SVal StreamVal = getStreamArg(Desc, Call);
-  std::optional<DefinedSVal> Stream = StreamVal.getAs<DefinedSVal>();
-  if (!Stream)
-    return;
-
-  ProgramStateRef StateNotNull, StateNull;
-  std::tie(StateNotNull, StateNull) =
-      C.getConstraintManager().assumeDual(State, *Stream);
-  if (StateNotNull && !StateNull)
-    ensureStreamOpened(StreamVal, C, StateNotNull);
-}
-
-void StreamChecker::evalFflush(const FnDescription *Desc, const CallEvent &Call,
-                               CheckerContext &C) const {
-  ProgramStateRef State = C.getState();
-  SVal StreamVal = getStreamArg(Desc, Call);
-  std::optional<DefinedSVal> Stream = StreamVal.getAs<DefinedSVal>();
-  if (!Stream)
-    return;
-
-  // Skip if the stream can be both NULL and non-NULL.
-  ProgramStateRef StateNotNull, StateNull;
-  std::tie(StateNotNull, StateNull) =
-      C.getConstraintManager().assumeDual(State, *Stream);
-  if (StateNotNull && StateNull)
-    return;
-  if (StateNotNull && !StateNull)
-    State = StateNotNull;
-  else
-    State = StateNull;
-
-  const CallExpr *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
-  if (!CE)
-    return;
-
-  // `fflush` returns EOF on failure, otherwise returns 0.
-  ProgramStateRef StateFailed = bindInt(*EofVal, State, C, CE);
-  ProgramStateRef StateNotFailed = bindInt(0, State, C, CE);
-
-  // Clear error states if `fflush` returns 0, but retain their EOF flags.
-  auto ClearErrorInNotFailed = [&StateNotFailed, Desc](SymbolRef Sym,
-                                                       const StreamState *SS) {
-    if (SS->ErrorState & ErrorFError) {
-      StreamErrorState NewES =
-          (SS->ErrorState & ErrorFEof) ? ErrorFEof : ErrorNone;
-      StreamState NewSS = StreamState::getOpened(Desc, NewES, false);
-      StateNotFailed = StateNotFailed->set<StreamMap>(Sym, NewSS);
-    }
-  };
-
-  if (StateNotNull && !StateNull) {
-    // Skip if the input stream's state is unknown, open-failed or closed.
-    if (SymbolRef StreamSym = StreamVal.getAsSymbol()) {
-      const StreamState *SS = State->get<StreamMap>(StreamSym);
-      if (SS) {
-        assert(SS->isOpened() && "Stream is expected to be opened");
-        ClearErrorInNotFailed(StreamSym, SS);
-      } else
-        return;
-    }
-  } else {
-    // Clear error states for all streams.
-    const StreamMapTy &Map = StateNotFailed->get<StreamMap>();
-    for (const auto &I : Map) {
-      SymbolRef Sym = I.first;
-      const StreamState &SS = I.second;
-      if (SS.isOpened())
-        ClearErrorInNotFailed(Sym, &SS);
-    }
-  }
-
-  C.addTransition(StateNotFailed);
-  C.addTransition(StateFailed);
-}
-
 ProgramStateRef
 StreamChecker::ensureStreamNonNull(SVal StreamVal, const Expr *StreamE,
                                    CheckerContext &C,

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) {

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) {

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)

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.

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}}