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

[balazske]

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" (theoretically correct bug normally "should not happen" cases) reports. Because this, the function is now assumed to not fail. The change affects StdCLibraryFunctionsChecker and StreamChecker.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Balázs Kéri (balazske)

Changes

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" (theoretically correct bug normally "should not happen" cases) reports. Because this, the function is now assumed to not fail. The change affects StdCLibraryFunctionsChecker and StreamChecker.

---

Full diff: https://github.com/llvm/llvm-project/pull/81842.diff

7 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp (+6-3)
• (modified) clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp (+124-79)
• (modified) clang/test/Analysis/std-c-library-functions-path-notes.c (+10-12)
• (modified) clang/test/Analysis/stream-errno-note.c (+2-10)
• (modified) clang/test/Analysis/stream-errno.c (+2-14)
• (modified) clang/test/Analysis/stream-error.c (+18)
• (modified) clang/test/Analysis/stream-noopen.c (+10)

diff --git a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
index 6b8ac2629453d4..6cc88679458140 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 07727b339d967a..4dda09c4817d7b 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/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,
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 4df00fe1e60646..6449b71928fa7c 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 2531e26e200385..2411a2d9a00a72 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 fab6a58b3275a8..5f0a58032fa263 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 4bab07577ccd53..ac31083bfc691f 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 8bd01a90cf8596..8c13f3c36597bb 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}}

[balazske]

Additionally code of fflush is moved in StreamChecker to a better place.

[NagyDonat]

LGTM with some low priority bikeshedding in a comment.

By the way it would be good to rebase this change onto your other commit #79312 (that was merged a few hours ago) and use the "toolbox" introduced there if that's possible.

[NagyDonat]

Suggested change

	// 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.)
	// Here we assume that the operation does not fail, because 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 vaild streams.
	// The stream error states are not modified by 'fileno', and 'errno' is also
	// left unchanged (so this evalCall does not invalidate it).

I felt that this comment is a bit difficult to understand and composed a reworded alternative. Of course, this is a very subjective matter and English isn't my first language, so feel free to bikeshed this and/or override my suggestions.

[NagyDonat]

It's a nice trick that you're creating the zero in the type of the other side to avoid surprising type conversion.

There's a chance that I can borrow this solution to handle tricky issues in ArrayBoundCheckerV2 😄

[NagyDonat]

LGTM, thanks for the update!

It's nice that you introduced the function getZeroVal(); it's yet another good "tool" in your toolbox.

[benshi001]

Shall we change to StreamOperationEvaluator which is introduced by your previous simplification patch ?

[benshi001]

It looks better to making the comment lines begin from the same column.