[clang][analyzer] Support `fgets` in the SteamChecker (#73638)

4 files changed, 89 insertions, 36 deletions

diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index eccb206..a4799b5 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -252,7 +252,10 @@ private:
         std::bind(&StreamChecker::evalFreadFwrite, _1, _2, _3, _4, false), 3}},
       {{{"fgetc"}, 1},
        {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, true),
-        std::bind(&StreamChecker::evalFgetc, _1, _2, _3, _4), 0}},
+        std::bind(&StreamChecker::evalFgetx, _1, _2, _3, _4, true), 0}},
+      {{{"fgets"}, 3},
+       {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, true),
+        std::bind(&StreamChecker::evalFgetx, _1, _2, _3, _4, false), 2}},
       {{{"fputc"}, 2},
        {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, false),
         std::bind(&StreamChecker::evalFputx, _1, _2, _3, _4, true), 1}},
@@ -320,8 +323,8 @@ private:
   void evalFreadFwrite(const FnDescription *Desc, const CallEvent &Call,
                        CheckerContext &C, bool IsFread) const;
 
-  void evalFgetc(const FnDescription *Desc, const CallEvent &Call,
-                 CheckerContext &C) const;
+  void evalFgetx(const FnDescription *Desc, const CallEvent &Call,
+                 CheckerContext &C, bool SingleChar) const;
 
   void evalFputx(const FnDescription *Desc, const CallEvent &Call,
                  CheckerContext &C, bool IsSingleChar) const;
@@ -760,8 +763,8 @@ void StreamChecker::evalFreadFwrite(const FnDescription *Desc,
     C.addTransition(StateFailed);
 }
 
-void StreamChecker::evalFgetc(const FnDescription *Desc, const CallEvent &Call,
-                              CheckerContext &C) const {
+void StreamChecker::evalFgetx(const FnDescription *Desc, const CallEvent &Call,
+                              CheckerContext &C, bool SingleChar) const {
   ProgramStateRef State = C.getState();
   SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol();
   if (!StreamSym)
@@ -778,42 +781,61 @@ void StreamChecker::evalFgetc(const FnDescription *Desc, const CallEvent &Call,
   assertStreamStateOpened(OldSS);
 
   // `fgetc` returns the read character on success, otherwise returns EOF.
+  // `fgets` returns the read buffer address on success, otherwise returns NULL.
 
-  // 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.
   if (OldSS->ErrorState != ErrorFEof) {
-    NonLoc RetVal = makeRetVal(C, CE).castAs<NonLoc>();
-    ProgramStateRef StateNotFailed =
-        State->BindExpr(CE, C.getLocationContext(), RetVal);
-    SValBuilder &SVB = C.getSValBuilder();
-    ASTContext &ASTC = C.getASTContext();
-    // The returned 'unsigned char' of `fgetc` is converted to 'int',
-    // so we need to check if it is in range [0, 255].
-    auto CondLow =
-        SVB.evalBinOp(State, BO_GE, RetVal, SVB.makeZeroVal(ASTC.IntTy),
-                      SVB.getConditionType())
-            .getAs<DefinedOrUnknownSVal>();
-    auto CondHigh =
-        SVB.evalBinOp(State, BO_LE, RetVal,
-                      SVB.makeIntVal(SVB.getBasicValueFactory()
-                                         .getMaxValue(ASTC.UnsignedCharTy)
-                                         .getLimitedValue(),
-                                     ASTC.IntTy),
-                      SVB.getConditionType())
-            .getAs<DefinedOrUnknownSVal>();
-    if (!CondLow || !CondHigh)
-      return;
-    StateNotFailed = StateNotFailed->assume(*CondLow, true);
-    if (!StateNotFailed)
-      return;
-    StateNotFailed = StateNotFailed->assume(*CondHigh, true);
-    if (!StateNotFailed)
-      return;
-    C.addTransition(StateNotFailed);
+    if (SingleChar) {
+      // Generate a transition for the success state of `fgetc`.
+      NonLoc RetVal = makeRetVal(C, CE).castAs<NonLoc>();
+      ProgramStateRef StateNotFailed =
+          State->BindExpr(CE, C.getLocationContext(), RetVal);
+      SValBuilder &SVB = C.getSValBuilder();
+      ASTContext &ASTC = C.getASTContext();
+      // The returned 'unsigned char' of `fgetc` is converted to 'int',
+      // so we need to check if it is in range [0, 255].
+      auto CondLow =
+          SVB.evalBinOp(State, BO_GE, RetVal, SVB.makeZeroVal(ASTC.IntTy),
+                        SVB.getConditionType())
+              .getAs<DefinedOrUnknownSVal>();
+      auto CondHigh =
+          SVB.evalBinOp(State, BO_LE, RetVal,
+                        SVB.makeIntVal(SVB.getBasicValueFactory()
+                                           .getMaxValue(ASTC.UnsignedCharTy)
+                                           .getLimitedValue(),
+                                       ASTC.IntTy),
+                        SVB.getConditionType())
+              .getAs<DefinedOrUnknownSVal>();
+      if (!CondLow || !CondHigh)
+        return;
+      StateNotFailed = StateNotFailed->assume(*CondLow, true);
+      if (!StateNotFailed)
+        return;
+      StateNotFailed = StateNotFailed->assume(*CondHigh, true);
+      if (!StateNotFailed)
+        return;
+      C.addTransition(StateNotFailed);
+    } else {
+      // Generate a transition for the success state of `fgets`.
+      std::optional<DefinedSVal> GetBuf =
+          Call.getArgSVal(0).getAs<DefinedSVal>();
+      if (!GetBuf)
+        return;
+      ProgramStateRef StateNotFailed =
+          State->BindExpr(CE, C.getLocationContext(), *GetBuf);
+      StateNotFailed = StateNotFailed->set<StreamMap>(
+          StreamSym, StreamState::getOpened(Desc));
+      C.addTransition(StateNotFailed);
+    }
   }
 
   // Add transition for the failed state.
-  ProgramStateRef StateFailed = bindInt(*EofVal, State, C, CE);
+  ProgramStateRef StateFailed;
+  if (SingleChar)
+    StateFailed = bindInt(*EofVal, State, C, CE);
+  else
+    StateFailed =
+        State->BindExpr(CE, C.getLocationContext(),
+                        C.getSValBuilder().makeNullWithType(CE->getType()));
 
   // If a (non-EOF) error occurs, the resulting value of the file position
   // indicator for the stream is indeterminate.
diff --git a/clang/test/Analysis/Inputs/system-header-simulator.h b/clang/test/Analysis/Inputs/system-header-simulator.h
index 7ef5f29..7089bd8 100644
--- a/clang/test/Analysis/Inputs/system-header-simulator.h
+++ b/clang/test/Analysis/Inputs/system-header-simulator.h
@@ -49,6 +49,7 @@ 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);
+char *fgets(char *restrict str, int count, FILE *restrict stream);
 int fputc(int ch, FILE *stream);
 int fputs(const char *restrict s, FILE *restrict stream);
 int fseek(FILE *__stream, long int __off, int __whence);
diff --git a/clang/test/Analysis/stream-error.c b/clang/test/Analysis/stream-error.c
index c90d0cd..c8332bc 100644
--- a/clang/test/Analysis/stream-error.c
+++ b/clang/test/Analysis/stream-error.c
@@ -123,6 +123,29 @@ void error_fgetc(void) {
   fgetc(F); // expected-warning {{Stream might be already closed}}
 }
 
+void error_fgets(void) {
+  FILE *F = tmpfile();
+  char Buf[256];
+  if (!F)
+    return;
+  char *Ret = fgets(Buf, sizeof(Buf), F);
+  if (Ret == Buf) {
+    clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}}
+  } else {
+    clang_analyzer_eval(Ret == NULL);          // expected-warning {{TRUE}}
+    clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{TRUE}}
+    if (feof(F)) {
+      clang_analyzer_eval(ferror(F)); // expected-warning {{FALSE}}
+      fgets(Buf, sizeof(Buf), F);     // expected-warning {{Read function called when stream is in EOF state}}
+    } else {
+      clang_analyzer_eval(ferror(F)); // expected-warning {{TRUE}}
+      fgets(Buf, sizeof(Buf), F);     // expected-warning {{might be 'indeterminate'}}
+    }
+  }
+  fclose(F);
+  fgets(Buf, sizeof(Buf), F);         // expected-warning {{Stream might be already closed}}
+}
+
 void error_fputc(void) {
   FILE *F = tmpfile();
   if (!F)
diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c
index b0f39af8..0b655db 100644
--- a/clang/test/Analysis/stream.c
+++ b/clang/test/Analysis/stream.c
@@ -20,6 +20,13 @@ void check_fgetc(void) {
   fclose(fp);
 }
 
+void check_fgets(void) {
+  FILE *fp = tmpfile();
+  char buf[256];
+  fgets(buf, sizeof(buf), 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}}