[clang][analyzer] Fix StreamChecker `ftell` and `fgetpos` at indeterminate file position. #84191

balazske · 2024-03-06T15:59:30Z

These functions should not be allowed if the file position is indeterminate (they return the file position).
This condition is now checked, and tests are improved to check it.

llvmbot · 2024-03-06T16:00:05Z

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

@llvm/pr-subscribers-clang

Author: Balázs Kéri (balazske)

Changes

These functions should not be allowed if the file position is indeterminate (they return the file position).
This condition is now checked, and tests are improved to check it.

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

2 Files Affected:

(modified) clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp (+53-30)
(modified) clang/test/Analysis/stream-error.c (+23-4)

diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 2ec47bf55df76b..346d4ae8db2dfe 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -307,64 +307,64 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
       {{{"fclose"}, 1},
        {&StreamChecker::preDefault, &StreamChecker::evalFclose, 0}},
       {{{"fread"}, 4},
-       {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, true),
+       {&StreamChecker::preRead,
         std::bind(&StreamChecker::evalFreadFwrite, _1, _2, _3, _4, true), 3}},
       {{{"fwrite"}, 4},
-       {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, false),
+       {&StreamChecker::preWrite,
         std::bind(&StreamChecker::evalFreadFwrite, _1, _2, _3, _4, false), 3}},
       {{{"fgetc"}, 1},
-       {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, true),
+       {&StreamChecker::preRead,
         std::bind(&StreamChecker::evalFgetx, _1, _2, _3, _4, true), 0}},
       {{{"fgets"}, 3},
-       {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, true),
+       {&StreamChecker::preRead,
         std::bind(&StreamChecker::evalFgetx, _1, _2, _3, _4, false), 2}},
       {{{"getc"}, 1},
        {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, true),
         std::bind(&StreamChecker::evalFgetx, _1, _2, _3, _4, true), 0}},
       {{{"fputc"}, 2},
-       {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, false),
+       {&StreamChecker::preWrite,
         std::bind(&StreamChecker::evalFputx, _1, _2, _3, _4, true), 1}},
       {{{"fputs"}, 2},
-       {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, false),
+       {&StreamChecker::preWrite,
         std::bind(&StreamChecker::evalFputx, _1, _2, _3, _4, false), 1}},
       {{{"putc"}, 2},
        {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, false),
         std::bind(&StreamChecker::evalFputx, _1, _2, _3, _4, true), 1}},
       {{{"fprintf"}},
-       {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, false),
+       {&StreamChecker::preWrite,
         std::bind(&StreamChecker::evalFprintf, _1, _2, _3, _4), 0}},
       {{{"vfprintf"}, 3},
        {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, false),
         std::bind(&StreamChecker::evalFprintf, _1, _2, _3, _4), 0}},
       {{{"fscanf"}},
-       {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, true),
+       {&StreamChecker::preRead,
         std::bind(&StreamChecker::evalFscanf, _1, _2, _3, _4), 0}},
       {{{"vfscanf"}, 3},
        {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, true),
         std::bind(&StreamChecker::evalFscanf, _1, _2, _3, _4), 0}},
       {{{"ungetc"}, 2},
-       {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, false),
+       {&StreamChecker::preWrite,
         std::bind(&StreamChecker::evalUngetc, _1, _2, _3, _4), 1}},
       {{{"getdelim"}, 4},
-       {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, true),
+       {&StreamChecker::preRead,
         std::bind(&StreamChecker::evalGetdelim, _1, _2, _3, _4), 3}},
       {{{"getline"}, 3},
-       {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, true),
+       {&StreamChecker::preRead,
         std::bind(&StreamChecker::evalGetdelim, _1, _2, _3, _4), 2}},
       {{{"fseek"}, 3},
        {&StreamChecker::preFseek, &StreamChecker::evalFseek, 0}},
       {{{"fseeko"}, 3},
        {&StreamChecker::preFseek, &StreamChecker::evalFseek, 0}},
       {{{"ftell"}, 1},
-       {&StreamChecker::preDefault, &StreamChecker::evalFtell, 0}},
+       {&StreamChecker::preWrite, &StreamChecker::evalFtell, 0}},
       {{{"ftello"}, 1},
-       {&StreamChecker::preDefault, &StreamChecker::evalFtell, 0}},
+       {&StreamChecker::preWrite, &StreamChecker::evalFtell, 0}},
       {{{"fflush"}, 1},
        {&StreamChecker::preFflush, &StreamChecker::evalFflush, 0}},
       {{{"rewind"}, 1},
        {&StreamChecker::preDefault, &StreamChecker::evalRewind, 0}},
       {{{"fgetpos"}, 2},
-       {&StreamChecker::preDefault, &StreamChecker::evalFgetpos, 0}},
+       {&StreamChecker::preWrite, &StreamChecker::evalFgetpos, 0}},
       {{{"fsetpos"}, 2},
        {&StreamChecker::preDefault, &StreamChecker::evalFsetpos, 0}},
       {{{"clearerr"}, 1},
@@ -384,12 +384,18 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
   CallDescriptionMap<FnDescription> FnTestDescriptions = {
       {{{"StreamTesterChecker_make_feof_stream"}, 1},
        {nullptr,
-        std::bind(&StreamChecker::evalSetFeofFerror, _1, _2, _3, _4, ErrorFEof),
+        std::bind(&StreamChecker::evalSetFeofFerror, _1, _2, _3, _4, ErrorFEof,
+                  false),
         0}},
       {{{"StreamTesterChecker_make_ferror_stream"}, 1},
        {nullptr,
         std::bind(&StreamChecker::evalSetFeofFerror, _1, _2, _3, _4,
-                  ErrorFError),
+                  ErrorFError, false),
+        0}},
+      {{{"StreamTesterChecker_make_ferror_indeterminate_stream"}, 1},
+       {nullptr,
+        std::bind(&StreamChecker::evalSetFeofFerror, _1, _2, _3, _4,
+                  ErrorFError, true),
         0}},
   };
 
@@ -415,8 +421,11 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
   void evalFclose(const FnDescription *Desc, const CallEvent &Call,
                   CheckerContext &C) const;
 
-  void preReadWrite(const FnDescription *Desc, const CallEvent &Call,
-                    CheckerContext &C, bool IsRead) const;
+  void preRead(const FnDescription *Desc, const CallEvent &Call,
+               CheckerContext &C) const;
+
+  void preWrite(const FnDescription *Desc, const CallEvent &Call,
+                CheckerContext &C) const;
 
   void evalFreadFwrite(const FnDescription *Desc, const CallEvent &Call,
                        CheckerContext &C, bool IsFread) const;
@@ -467,8 +476,8 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
                       const StreamErrorState &ErrorKind) const;
 
   void evalSetFeofFerror(const FnDescription *Desc, const CallEvent &Call,
-                         CheckerContext &C,
-                         const StreamErrorState &ErrorKind) const;
+                         CheckerContext &C, const StreamErrorState &ErrorKind,
+                         bool Indeterminate) const;
 
   void preFflush(const FnDescription *Desc, const CallEvent &Call,
                  CheckerContext &C) const;
@@ -849,9 +858,8 @@ void StreamChecker::evalFclose(const FnDescription *Desc, const CallEvent &Call,
   C.addTransition(E.bindReturnValue(State, C, *EofVal));
 }
 
-void StreamChecker::preReadWrite(const FnDescription *Desc,
-                                 const CallEvent &Call, CheckerContext &C,
-                                 bool IsRead) const {
+void StreamChecker::preRead(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,
@@ -865,11 +873,6 @@ void StreamChecker::preReadWrite(const FnDescription *Desc,
   if (!State)
     return;
 
-  if (!IsRead) {
-    C.addTransition(State);
-    return;
-  }
-
   SymbolRef Sym = StreamVal.getAsSymbol();
   if (Sym && State->get<StreamMap>(Sym)) {
     const StreamState *SS = State->get<StreamMap>(Sym);
@@ -880,6 +883,24 @@ void StreamChecker::preReadWrite(const FnDescription *Desc,
   }
 }
 
+void StreamChecker::preWrite(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;
+  State = ensureNoFilePositionIndeterminate(StreamVal, C, State);
+  if (!State)
+    return;
+
+  C.addTransition(State);
+}
+
 void StreamChecker::evalFreadFwrite(const FnDescription *Desc,
                                     const CallEvent &Call, CheckerContext &C,
                                     bool IsFread) const {
@@ -1496,14 +1517,16 @@ void StreamChecker::preDefault(const FnDescription *Desc, const CallEvent &Call,
 
 void StreamChecker::evalSetFeofFerror(const FnDescription *Desc,
                                       const CallEvent &Call, CheckerContext &C,
-                                      const StreamErrorState &ErrorKind) const {
+                                      const StreamErrorState &ErrorKind,
+                                      bool Indeterminate) 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));
+      StreamSym,
+      StreamState::getOpened(SS->LastOperation, ErrorKind, Indeterminate));
   C.addTransition(State);
 }
 
diff --git a/clang/test/Analysis/stream-error.c b/clang/test/Analysis/stream-error.c
index ac31083bfc691f..88f7de4234ffb4 100644
--- a/clang/test/Analysis/stream-error.c
+++ b/clang/test/Analysis/stream-error.c
@@ -11,6 +11,7 @@ void clang_analyzer_dump(int);
 void clang_analyzer_warnIfReached(void);
 void StreamTesterChecker_make_feof_stream(FILE *);
 void StreamTesterChecker_make_ferror_stream(FILE *);
+void StreamTesterChecker_make_ferror_indeterminate_stream(FILE *);
 
 void error_fopen(void) {
   FILE *F = fopen("file", "r");
@@ -52,6 +53,8 @@ void stream_error_feof(void) {
   clearerr(F);
   clang_analyzer_eval(feof(F));   // expected-warning {{FALSE}}
   clang_analyzer_eval(ferror(F)); // expected-warning {{FALSE}}
+  StreamTesterChecker_make_ferror_indeterminate_stream(F);
+  clang_analyzer_eval(feof(F));   // expected-warning {{FALSE}}
   fclose(F);
 }
 
@@ -65,6 +68,8 @@ void stream_error_ferror(void) {
   clearerr(F);
   clang_analyzer_eval(feof(F));   // expected-warning {{FALSE}}
   clang_analyzer_eval(ferror(F)); // expected-warning {{FALSE}}
+  StreamTesterChecker_make_ferror_indeterminate_stream(F);
+  clang_analyzer_eval(ferror(F)); // expected-warning {{TRUE}}
   fclose(F);
 }
 
@@ -233,7 +238,7 @@ void error_fscanf(int *A) {
   fscanf(F, "ccc"); // expected-warning {{Stream might be already closed}}
 }
 
-void error_ungetc() {
+void error_ungetc(int TestIndeterminate) {
   FILE *F = tmpfile();
   if (!F)
     return;
@@ -245,8 +250,12 @@ void error_ungetc() {
     clang_analyzer_eval(Ret == 'X');         // expected-warning {{TRUE}}
   }
   fputc('Y', F);                             // no-warning
+  if (TestIndeterminate) {
+    StreamTesterChecker_make_ferror_indeterminate_stream(F);
+    ungetc('X', F);                          // expected-warning {{might be 'indeterminate'}}
+  }
   fclose(F);
-  ungetc('A', F); // expected-warning {{Stream might be already closed}}
+  ungetc('A', F);                            // expected-warning {{Stream might be already closed}}
 }
 
 void error_getdelim(char *P, size_t Sz) {
@@ -449,7 +458,7 @@ void error_fseeko_0(void) {
   fclose(F);
 }
 
-void error_ftell(void) {
+void error_ftell(int TestIndeterminate) {
   FILE *F = fopen("file", "r");
   if (!F)
     return;
@@ -467,10 +476,14 @@ void error_ftell(void) {
   rc = ftell(F);
   clang_analyzer_eval(feof(F));              // expected-warning {{FALSE}}
   clang_analyzer_eval(ferror(F));            // expected-warning {{TRUE}}
+  if (TestIndeterminate) {
+    StreamTesterChecker_make_ferror_indeterminate_stream(F);
+    ftell(F);                                // expected-warning {{might be 'indeterminate'}}
+  }
   fclose(F);
 }
 
-void error_ftello(void) {
+void error_ftello(int TestIndeterminate) {
   FILE *F = fopen("file", "r");
   if (!F)
     return;
@@ -488,6 +501,10 @@ void error_ftello(void) {
   rc = ftello(F);
   clang_analyzer_eval(feof(F));              // expected-warning {{FALSE}}
   clang_analyzer_eval(ferror(F));            // expected-warning {{TRUE}}
+  if (TestIndeterminate) {
+    StreamTesterChecker_make_ferror_indeterminate_stream(F);
+    ftell(F);                                // expected-warning {{might be 'indeterminate'}}
+  }
   fclose(F);
 }
 
@@ -506,6 +523,8 @@ void error_fileno(void) {
   N = fileno(F);
   clang_analyzer_eval(feof(F));              // expected-warning {{FALSE}}
   clang_analyzer_eval(ferror(F));            // expected-warning {{TRUE}}
+  StreamTesterChecker_make_ferror_indeterminate_stream(F);
+  fileno(F);                                 // no warning
   fclose(F);
 }

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

…inate file position. These functions should not be allowed if the file position is indeterminate (they return the file position). This condition is now checked, and tests are improved to check it.

steakhal

I haven't gone deep into this PR, but it looks good to me.
Please wait for someone else to sign it too.

benshi001

After eliminating std::bind, I hope there can be further solutions to reduce duplications.

balazske requested review from benshi001 and NagyDonat March 6, 2024 15:59

llvmbot added clang Clang issues not falling into any other category clang:static analyzer labels Mar 6, 2024

benshi001 reviewed Mar 7, 2024

View reviewed changes

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp Show resolved Hide resolved

benshi001 self-requested a review March 7, 2024 01:23

[clang][analyzer] Fix StreamChecker ftell and fgetpos at indeterm…

dbaf334

…inate file position. These functions should not be allowed if the file position is indeterminate (they return the file position). This condition is now checked, and tests are improved to check it.

balazske force-pushed the streamchecker_indet branch from f5e2ba8 to dbaf334 Compare March 7, 2024 08:50

steakhal approved these changes Mar 7, 2024

View reviewed changes

benshi001 approved these changes Mar 8, 2024

View reviewed changes

balazske merged commit d72b7f9 into llvm:main Mar 8, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang][analyzer] Fix StreamChecker `ftell` and `fgetpos` at indeterminate file position. #84191

[clang][analyzer] Fix StreamChecker `ftell` and `fgetpos` at indeterminate file position. #84191

balazske commented Mar 6, 2024

llvmbot commented Mar 6, 2024 •

edited

steakhal left a comment

benshi001 left a comment

[clang][analyzer] Fix StreamChecker ftell and fgetpos at indeterminate file position. #84191

[clang][analyzer] Fix StreamChecker ftell and fgetpos at indeterminate file position. #84191

Conversation

balazske commented Mar 6, 2024

llvmbot commented Mar 6, 2024 • edited

steakhal left a comment

Choose a reason for hiding this comment

benshi001 left a comment

Choose a reason for hiding this comment

[clang][analyzer] Fix StreamChecker `ftell` and `fgetpos` at indeterminate file position. #84191

[clang][analyzer] Fix StreamChecker `ftell` and `fgetpos` at indeterminate file position. #84191

llvmbot commented Mar 6, 2024 •

edited