[clang] [clang][analyzer] Support `fputs` in the StreamChecker (PR #73335)

Ben Shi via cfe-commits cfe-commits at lists.llvm.org
Tue Nov 28 01:29:20 PST 2023


https://github.com/benshi001 updated https://github.com/llvm/llvm-project/pull/73335

>From ff1ab48c5760302dc55ff889b5b3dfd0b884aa33 Mon Sep 17 00:00:00 2001
From: Ben Shi <bennshi at tencent.com>
Date: Fri, 24 Nov 2023 22:51:27 +0800
Subject: [PATCH 1/2] [clang][analyzer] Support `fputs` in the StreamChecker

---
 .../StaticAnalyzer/Checkers/StreamChecker.cpp | 105 +++++++++++++-----
 .../Analysis/Inputs/system-header-simulator.h |   1 +
 clang/test/Analysis/stream-error.c            |  30 ++++-
 clang/test/Analysis/stream.c                  |   6 +
 4 files changed, 109 insertions(+), 33 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 8eca989d7bcdea4..eccb2063fad10d0 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -252,10 +252,13 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
         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}},
+        std::bind(&StreamChecker::evalFgetc, _1, _2, _3, _4), 0}},
       {{{"fputc"}, 2},
        {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, false),
-        std::bind(&StreamChecker::evalFgetcFputc, _1, _2, _3, _4, false), 1}},
+        std::bind(&StreamChecker::evalFputx, _1, _2, _3, _4, true), 1}},
+      {{{"fputs"}, 2},
+       {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, false),
+        std::bind(&StreamChecker::evalFputx, _1, _2, _3, _4, false), 1}},
       {{{"fseek"}, 3},
        {&StreamChecker::preFseek, &StreamChecker::evalFseek, 0}},
       {{{"ftell"}, 1},
@@ -317,8 +320,11 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
   void evalFreadFwrite(const FnDescription *Desc, const CallEvent &Call,
                        CheckerContext &C, bool IsFread) const;
 
-  void evalFgetcFputc(const FnDescription *Desc, const CallEvent &Call,
-                      CheckerContext &C, bool IsRead) const;
+  void evalFgetc(const FnDescription *Desc, const CallEvent &Call,
+                 CheckerContext &C) const;
+
+  void evalFputx(const FnDescription *Desc, const CallEvent &Call,
+                 CheckerContext &C, bool IsSingleChar) const;
 
   void preFseek(const FnDescription *Desc, const CallEvent &Call,
                 CheckerContext &C) const;
@@ -754,9 +760,8 @@ void StreamChecker::evalFreadFwrite(const FnDescription *Desc,
     C.addTransition(StateFailed);
 }
 
-void StreamChecker::evalFgetcFputc(const FnDescription *Desc,
-                                   const CallEvent &Call, CheckerContext &C,
-                                   bool IsRead) const {
+void StreamChecker::evalFgetc(const FnDescription *Desc, const CallEvent &Call,
+                              CheckerContext &C) const {
   ProgramStateRef State = C.getState();
   SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol();
   if (!StreamSym)
@@ -773,27 +778,15 @@ void StreamChecker::evalFgetcFputc(const FnDescription *Desc,
   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 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.
+  // 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) {
+  if (OldSS->ErrorState != ErrorFEof) {
     NonLoc RetVal = makeRetVal(C, CE).castAs<NonLoc>();
     ProgramStateRef StateNotFailed =
         State->BindExpr(CE, C.getLocationContext(), RetVal);
     SValBuilder &SVB = C.getSValBuilder();
-    auto &ASTC = C.getASTContext();
+    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 =
@@ -824,20 +817,74 @@ void StreamChecker::evalFgetcFputc(const FnDescription *Desc,
 
   // If a (non-EOF) error occurs, the resulting value of the file position
   // indicator for the stream is indeterminate.
-  StreamErrorState NewES;
-  if (IsRead)
-    NewES =
-        OldSS->ErrorState == ErrorFEof ? ErrorFEof : ErrorFEof | ErrorFError;
-  else
-    NewES = ErrorFError;
+  StreamErrorState NewES =
+      OldSS->ErrorState == ErrorFEof ? ErrorFEof : ErrorFEof | ErrorFError;
   StreamState NewSS = StreamState::getOpened(Desc, NewES, !NewES.isFEof());
   StateFailed = StateFailed->set<StreamMap>(StreamSym, NewSS);
-  if (IsRead && OldSS->ErrorState != ErrorFEof)
+  if (OldSS->ErrorState != ErrorFEof)
     C.addTransition(StateFailed, constructSetEofNoteTag(C, StreamSym));
   else
     C.addTransition(StateFailed);
 }
 
+void StreamChecker::evalFputx(const FnDescription *Desc, const CallEvent &Call,
+                              CheckerContext &C, bool IsSingleChar) const {
+  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 *OldSS = State->get<StreamMap>(StreamSym);
+  if (!OldSS)
+    return;
+
+  assertStreamStateOpened(OldSS);
+
+  // `fputc` returns the written character on success, otherwise returns EOF.
+  // `fputs` returns a non negative value on sucecess, otherwise returns EOF.
+
+  if (IsSingleChar) {
+    // Generate a transition for the success state of `fputc`.
+    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);
+  } else {
+    // Generate a transition for the success state of `fputs`.
+    NonLoc RetVal = makeRetVal(C, CE).castAs<NonLoc>();
+    ProgramStateRef StateNotFailed =
+        State->BindExpr(CE, C.getLocationContext(), RetVal);
+    SValBuilder &SVB = C.getSValBuilder();
+    auto &ASTC = C.getASTContext();
+    auto Cond = SVB.evalBinOp(State, BO_GE, RetVal, SVB.makeZeroVal(ASTC.IntTy),
+                              SVB.getConditionType())
+                    .getAs<DefinedOrUnknownSVal>();
+    if (!Cond)
+      return;
+    StateNotFailed = StateNotFailed->assume(*Cond, true);
+    if (!StateNotFailed)
+      return;
+    StateNotFailed =
+        StateNotFailed->set<StreamMap>(StreamSym, StreamState::getOpened(Desc));
+    C.addTransition(StateNotFailed);
+  }
+
+  // Add transition for the failed state. 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, true);
+  StateFailed = StateFailed->set<StreamMap>(StreamSym, NewSS);
+  C.addTransition(StateFailed);
+}
+
 void StreamChecker::preFseek(const FnDescription *Desc, const CallEvent &Call,
                              CheckerContext &C) const {
   ProgramStateRef State = C.getState();
diff --git a/clang/test/Analysis/Inputs/system-header-simulator.h b/clang/test/Analysis/Inputs/system-header-simulator.h
index fc57e8bdc3d30c3..7ef5f29fbf42cb1 100644
--- a/clang/test/Analysis/Inputs/system-header-simulator.h
+++ b/clang/test/Analysis/Inputs/system-header-simulator.h
@@ -50,6 +50,7 @@ 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 fputs(const char *restrict s, FILE *restrict stream);
 int fseek(FILE *__stream, long int __off, int __whence);
 long int ftell(FILE *__stream);
 void rewind(FILE *__stream);
diff --git a/clang/test/Analysis/stream-error.c b/clang/test/Analysis/stream-error.c
index 38e6b77b9bb5053..8ad29f48ed49514 100644
--- a/clang/test/Analysis/stream-error.c
+++ b/clang/test/Analysis/stream-error.c
@@ -131,16 +131,38 @@ void error_fputc(void) {
   if (Ret == EOF) {
     clang_analyzer_eval(ferror(F)); // expected-warning {{TRUE}}
     clang_analyzer_eval(feof(F));   // expected-warning {{FALSE}}
-    fputc('Y', F); // expected-warning {{might be 'indeterminate'}}
+    fputc('Y', F);                  // expected-warning {{might be 'indeterminate'}}
   } else {
-    clang_analyzer_eval(Ret == 'X'); // expected-warning {{TRUE}}
-    clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}}
-    fputc('Y', F); // no-warning
+    clang_analyzer_eval(Ret == 'X');             // expected-warning {{TRUE}}
+    clang_analyzer_eval(feof(F) || ferror(F));   // expected-warning {{FALSE}}
+    StreamTesterChecker_make_feof_stream(F);
+    if (fputc('Y', F) == 'Y')                    // no-warning
+      clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}}
   }
   fclose(F);
   fputc('A', F); // expected-warning {{Stream might be already closed}}
 }
 
+void error_fputs(void) {
+  FILE *F = tmpfile();
+  if (!F)
+    return;
+  int Ret = fputs("XYZ", F);
+  if (Ret >= 0) {
+    clang_analyzer_eval(feof(F) || ferror(F));   // expected-warning {{FALSE}}
+    StreamTesterChecker_make_feof_stream(F);
+    if (fputs("QWD", F) >= 0)                    // no-warning
+      clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}}
+  } else {
+    clang_analyzer_eval(Ret == EOF); // expected-warning {{TRUE}}
+    clang_analyzer_eval(ferror(F));  // expected-warning {{TRUE}}
+    clang_analyzer_eval(feof(F));    // expected-warning {{FALSE}}
+    fputs("QWD", F);                 // expected-warning {{might be 'indeterminate'}}
+  }
+  fclose(F);
+  fputs("ABC", F); // expected-warning {{Stream might be already closed}}
+}
+
 void freadwrite_zerosize(FILE *F) {
   size_t Ret;
   Ret = fwrite(0, 1, 0, F);
diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c
index a7ee9982478bb96..b0f39af8eb2f3a3 100644
--- a/clang/test/Analysis/stream.c
+++ b/clang/test/Analysis/stream.c
@@ -26,6 +26,12 @@ void check_fputc(void) {
   fclose(fp);
 }
 
+void check_fputs(void) {
+  FILE *fp = tmpfile();
+  fputs("ABC", fp); // expected-warning {{Stream pointer might be NULL}}
+  fclose(fp);
+}
+
 void check_fseek(void) {
   FILE *fp = tmpfile();
   fseek(fp, 0, 0); // expected-warning {{Stream pointer might be NULL}}

>From 893cbbe626ae3935e4589731c24d230fdc2e6155 Mon Sep 17 00:00:00 2001
From: Ben Shi <bennshi at tencent.com>
Date: Tue, 28 Nov 2023 17:28:16 +0800
Subject: [PATCH 2/2] [clang][analyzer][NFC] Improve tests for the
 StreamChecker

---
 clang/test/Analysis/stream-error.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/clang/test/Analysis/stream-error.c b/clang/test/Analysis/stream-error.c
index 8ad29f48ed49514..c90d0cdc5742d37 100644
--- a/clang/test/Analysis/stream-error.c
+++ b/clang/test/Analysis/stream-error.c
@@ -135,9 +135,7 @@ void error_fputc(void) {
   } else {
     clang_analyzer_eval(Ret == 'X');             // expected-warning {{TRUE}}
     clang_analyzer_eval(feof(F) || ferror(F));   // expected-warning {{FALSE}}
-    StreamTesterChecker_make_feof_stream(F);
-    if (fputc('Y', F) == 'Y')                    // no-warning
-      clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}}
+    fputc('Y', F);                               // no-warning
   }
   fclose(F);
   fputc('A', F); // expected-warning {{Stream might be already closed}}
@@ -150,9 +148,7 @@ void error_fputs(void) {
   int Ret = fputs("XYZ", F);
   if (Ret >= 0) {
     clang_analyzer_eval(feof(F) || ferror(F));   // expected-warning {{FALSE}}
-    StreamTesterChecker_make_feof_stream(F);
-    if (fputs("QWD", F) >= 0)                    // no-warning
-      clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}}
+    fputs("QWD", F);                             // no-warning
   } else {
     clang_analyzer_eval(Ret == EOF); // expected-warning {{TRUE}}
     clang_analyzer_eval(ferror(F));  // expected-warning {{TRUE}}
@@ -163,6 +159,22 @@ void error_fputs(void) {
   fputs("ABC", F); // expected-warning {{Stream might be already closed}}
 }
 
+void write_after_eof_is_allowed(void) {
+  FILE *F = tmpfile();
+  if (!F)
+    return;
+  StreamTesterChecker_make_feof_stream(F);
+  if (fputs("QWD", F) >= 0)                    // no-warning
+    clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}}
+  StreamTesterChecker_make_feof_stream(F);
+  if (fputc('Q', F) == 'Q')                    // no-warning
+    clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}}
+  StreamTesterChecker_make_feof_stream(F);
+  if (fwrite("012345678", 1, 10, F) == 10)     // no-warning
+    clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}}
+  fclose(F);
+}
+
 void freadwrite_zerosize(FILE *F) {
   size_t Ret;
   Ret = fwrite(0, 1, 0, F);



More information about the cfe-commits mailing list