[clang][analyzer] Support fgetc in StreamChecker

[benshi001]

No description provided.

[llvmbot]

@llvm/pr-subscribers-clang

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

Author: Ben Shi (benshi001)

Changes

---

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

4 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp (+52-18)
• (modified) clang/test/Analysis/Inputs/system-header-simulator.h (+1)
• (modified) clang/test/Analysis/stream-error.c (+72-3)
• (modified) clang/test/Analysis/stream.c (+6)

diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 1d53e59ca067c27..3acf23f090cb397 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -250,9 +250,12 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
       {{{"fwrite"}, 4},
        {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, false),
         std::bind(&StreamChecker::evalFreadFwrite, _1, _2, _3, _4, false), 3}},
+      {{{"fgetc"}, 1},
+       {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, true),
+        std::bind(&StreamChecker::evalFgetcFputc, _1, _2, _3, _4, true), 0}},
       {{{"fputc"}, 2},
        {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, false),
-        &StreamChecker::evalFputc, 1}},
+        std::bind(&StreamChecker::evalFgetcFputc, _1, _2, _3, _4, false), 1}},
       {{{"fseek"}, 3},
        {&StreamChecker::preFseek, &StreamChecker::evalFseek, 0}},
       {{{"ftell"}, 1},
@@ -314,8 +317,8 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
   void evalFreadFwrite(const FnDescription *Desc, const CallEvent &Call,
                        CheckerContext &C, bool IsFread) const;
 
-  void evalFputc(const FnDescription *Desc, const CallEvent &Call,
-                 CheckerContext &C) const;
+  void evalFgetcFputc(const FnDescription *Desc, const CallEvent &Call,
+                      CheckerContext &C, bool IsRead) const;
 
   void preFseek(const FnDescription *Desc, const CallEvent &Call,
                 CheckerContext &C) const;
@@ -751,8 +754,9 @@ void StreamChecker::evalFreadFwrite(const FnDescription *Desc,
     C.addTransition(StateFailed);
 }
 
-void StreamChecker::evalFputc(const FnDescription *Desc, const CallEvent &Call,
-                              CheckerContext &C) const {
+void StreamChecker::evalFgetcFputc(const FnDescription *Desc,
+                                   const CallEvent &Call, CheckerContext &C,
+                                   bool IsRead) const {
   ProgramStateRef State = C.getState();
   SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol();
   if (!StreamSym)
@@ -768,26 +772,56 @@ void StreamChecker::evalFputc(const FnDescription *Desc, const CallEvent &Call,
 
   assertStreamStateOpened(OldSS);
 
+  // `fgetc` returns the read character on success, otherwise returns EOF.
   // `fputc` returns the written character on success, otherwise returns EOF.
 
-  // Generate a transition for the success state.
-  std::optional<NonLoc> PutVal = Call.getArgSVal(0).getAs<NonLoc>();
-  if (!PutVal)
-    return;
-  ProgramStateRef StateNotFailed =
-      State->BindExpr(CE, C.getLocationContext(), *PutVal);
-  StateNotFailed =
-      StateNotFailed->set<StreamMap>(StreamSym, StreamState::getOpened(Desc));
-  C.addTransition(StateNotFailed);
+  // Generate a transition for the success state of fputc.
+  if (!IsRead) {
+    std::optional<NonLoc> PutVal = Call.getArgSVal(0).getAs<NonLoc>();
+    if (!PutVal)
+      return;
+    ProgramStateRef StateNotFailed =
+        State->BindExpr(CE, C.getLocationContext(), *PutVal);
+    StateNotFailed =
+        StateNotFailed->set<StreamMap>(StreamSym, StreamState::getOpened(Desc));
+    C.addTransition(StateNotFailed);
+  }
+  // Generate a transition for the success state of fgetc.
+  // If we know the state to be FEOF at fgetc, do not add a success state.
+  else if (OldSS->ErrorState != ErrorFEof) {
+    NonLoc RetVal = makeRetVal(C, CE).castAs<NonLoc>();
+    ProgramStateRef StateNotFailed =
+        State->BindExpr(CE, C.getLocationContext(), RetVal);
+    SValBuilder &SVB = C.getSValBuilder();
+    auto Cond = SVB.evalBinOp(State, BO_GE, RetVal,
+                              SVB.makeZeroVal(C.getASTContext().IntTy),
+                              SVB.getConditionType())
+                    .getAs<DefinedOrUnknownSVal>();
+    if (!Cond)
+      return;
+    StateNotFailed = StateNotFailed->assume(*Cond, true);
+    if (!StateNotFailed)
+      return;
+    C.addTransition(StateNotFailed);
+  }
 
   // Add transition for the failed state.
+  ProgramStateRef StateFailed = bindInt(*EofVal, State, C, CE);
+
   // If a (non-EOF) error occurs, the resulting value of the file position
   // indicator for the stream is indeterminate.
-  ProgramStateRef StateFailed = bindInt(*EofVal, State, C, CE);
-  StreamState NewSS = StreamState::getOpened(
-      Desc, ErrorFError, /*IsFilePositionIndeterminate*/ true);
+  StreamErrorState NewES;
+  if (IsRead)
+    NewES =
+        OldSS->ErrorState == ErrorFEof ? ErrorFEof : ErrorFEof | ErrorFError;
+  else
+    NewES = ErrorFError;
+  StreamState NewSS = StreamState::getOpened(Desc, NewES, !NewES.isFEof());
   StateFailed = StateFailed->set<StreamMap>(StreamSym, NewSS);
-  C.addTransition(StateFailed);
+  if (IsRead && OldSS->ErrorState != ErrorFEof)
+    C.addTransition(StateFailed, constructSetEofNoteTag(C, StreamSym));
+  else
+    C.addTransition(StateFailed);
 }
 
 void StreamChecker::preFseek(const FnDescription *Desc, const CallEvent &Call,
diff --git a/clang/test/Analysis/Inputs/system-header-simulator.h b/clang/test/Analysis/Inputs/system-header-simulator.h
index 8924103f5046ea2..fc57e8bdc3d30c3 100644
--- a/clang/test/Analysis/Inputs/system-header-simulator.h
+++ b/clang/test/Analysis/Inputs/system-header-simulator.h
@@ -48,6 +48,7 @@ FILE *freopen(const char *restrict pathname, const char *restrict mode, FILE *re
 int fclose(FILE *fp);
 size_t fread(void *restrict, size_t, size_t, FILE *restrict);
 size_t fwrite(const void *restrict, size_t, size_t, FILE *restrict);
+int fgetc(FILE *stream);
 int fputc(int ch, FILE *stream);
 int fseek(FILE *__stream, long int __off, int __whence);
 long int ftell(FILE *__stream);
diff --git a/clang/test/Analysis/stream-error.c b/clang/test/Analysis/stream-error.c
index 5ebdc32bb1b92ff..521bdb15f706bc3 100644
--- a/clang/test/Analysis/stream-error.c
+++ b/clang/test/Analysis/stream-error.c
@@ -101,6 +101,30 @@ void error_fwrite(void) {
   Ret = fwrite(0, 1, 10, F); // expected-warning {{Stream might be already closed}}
 }
 
+void error_fgetc(void) {
+  FILE *F = tmpfile();
+  if (!F)
+    return;
+  int Ret = fgetc(F);
+  if (Ret >= 0) {
+    clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}}
+  } else {
+    clang_analyzer_eval(Ret == EOF);  // expected-warning {{TRUE}}
+    if (feof(F)) {
+      clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+      fgetc(F);                       // expected-warning {{Read function called when stream is in EOF state}}
+      clang_analyzer_eval(feof(F));   // expected-warning {{TRUE}}
+      clang_analyzer_eval(ferror(F)); // expected-warning {{FALSE}}
+    }
+    if (ferror(F)) {
+      clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+      fgetc(F);                       // expected-warning {{might be 'indeterminate'}}
+    }
+  }
+  fclose(F);
+  fgetc(F); // expected-warning {{Stream might be already closed}}
+}
+
 void error_fputc(void) {
   FILE *F = tmpfile();
   if (!F)
@@ -259,6 +283,24 @@ void error_indeterminate_clearerr(void) {
   fclose(F);
 }
 
+void error_indeterminate_fgetc(void) {
+  FILE *F = fopen("file", "r+");
+  if (!F)
+    return;
+  int rc = fseek(F, 0, SEEK_SET);
+  if (rc) {
+    if (feof(F)) {
+      clang_analyzer_warnIfReached(); // no warning
+      fgetc(F);                       // no warning
+    } else if (ferror(F)) {
+      fgetc(F);                       // expected-warning {{might be 'indeterminate'}}
+    } else {
+      fgetc(F);                       // expected-warning {{might be 'indeterminate'}}
+    }
+  }
+  fclose(F);
+}
+
 void error_indeterminate_fputc(void) {
   FILE *F = fopen("file", "r+");
   if (!F)
@@ -266,11 +308,12 @@ void error_indeterminate_fputc(void) {
   int rc = fseek(F, 0, SEEK_SET);
   if (rc) {
     if (feof(F)) {
-      fputc('X', F); // no warning
+      clang_analyzer_warnIfReached(); // no warning
+      fputc('X', F);                  // no warning
     } else if (ferror(F)) {
-      fputc('C', F); // expected-warning {{might be 'indeterminate'}}
+      fputc('C', F);                  // expected-warning {{might be 'indeterminate'}}
     } else {
-      fputc('E', F); // expected-warning {{might be 'indeterminate'}}
+      fputc('E', F);                  // expected-warning {{might be 'indeterminate'}}
     }
   }
   fclose(F);
@@ -303,3 +346,29 @@ void error_indeterminate_feof2(void) {
   }
   fclose(F);
 }
+
+void error_indeterminate_feof3(void) {
+  FILE *F = fopen("file", "r+");
+  if (!F)
+    return;
+  if (fgetc(F) == EOF) {
+    if (feof(F)) {
+      // error is feof, should be non-indeterminate
+      fputc('A', F); // no warning
+    }
+  }
+  fclose(F);
+}
+
+void error_indeterminate_feof4(void) {
+  FILE *F = fopen("file", "r+");
+  if (!F)
+    return;
+  if (fgetc(F) == EOF) {
+    if (ferror(F) == 0) {
+      // error is feof, should be non-indeterminate
+      fputc('B', F); // no warning
+    }
+  }
+  fclose(F);
+}
diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c
index 9b6a31738d6e0b8..a7ee9982478bb96 100644
--- a/clang/test/Analysis/stream.c
+++ b/clang/test/Analysis/stream.c
@@ -14,6 +14,12 @@ void check_fwrite(void) {
   fclose(fp);
 }
 
+void check_fgetc(void) {
+  FILE *fp = tmpfile();
+  fgetc(fp); // expected-warning {{Stream pointer might be NULL}}
+  fclose(fp);
+}
+
 void check_fputc(void) {
   FILE *fp = tmpfile();
   fputc('A', fp); // expected-warning {{Stream pointer might be NULL}}