[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

@@ -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 @@ 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 = {
@@ -486,6 +491,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
@@ -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,

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.