[clang][analyzer] Change modeling of fseek in StreamChecker.

[balazske]

Until now function fseek returned nonzero on error, this is changed to -1 only. And it does not produce EOF error any more.
This complies better with the POSIX standard.

[llvmbot]

@llvm/pr-subscribers-clang

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

Author: Balázs Kéri (balazske)

Changes

Until now function fseek returned nonzero on error, this is changed to -1 only. And it does not produce EOF error any more.
This complies better with the POSIX standard.

---

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

3 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp (+7-14)
• (modified) clang/test/Analysis/stream-error.c (+18-25)
• (modified) clang/test/Analysis/stream-note.c (+30-1)

diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 902c42a2799be4..069e3a633c1214 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -1264,15 +1264,10 @@ void StreamChecker::evalFseek(const FnDescription *Desc, const CallEvent &Call,
   if (!E.Init(Desc, Call, C, State))
     return;
 
-  const llvm::APSInt *PosV =
-      C.getSValBuilder().getKnownValue(State, Call.getArgSVal(1));
-  const llvm::APSInt *WhenceV =
-      C.getSValBuilder().getKnownValue(State, Call.getArgSVal(2));
-
   // Bifurcate the state into failed and non-failed.
-  // Return zero on success, nonzero on error.
-  ProgramStateRef StateNotFailed, StateFailed;
-  std::tie(StateFailed, StateNotFailed) = E.makeRetValAndAssumeDual(State, C);
+  // Return zero on success, -1 on error.
+  ProgramStateRef StateNotFailed = E.bindReturnValue(State, C, 0);
+  ProgramStateRef StateFailed = E.bindReturnValue(State, C, -1);
 
   // No failure: Reset the state to opened with no error.
   StateNotFailed =
@@ -1282,12 +1277,10 @@ void StreamChecker::evalFseek(const FnDescription *Desc, const CallEvent &Call,
   // At error it is possible that fseek fails but sets none of the error flags.
   // If fseek failed, assume that the file position becomes indeterminate in any
   // case.
-  StreamErrorState NewErrS = ErrorNone | ErrorFError;
-  // Setting the position to start of file never produces EOF error.
-  if (!(PosV && *PosV == 0 && WhenceV && *WhenceV == SeekSetVal))
-    NewErrS = NewErrS | ErrorFEof;
-  StateFailed = E.setStreamState(StateFailed,
-                                 StreamState::getOpened(Desc, NewErrS, true));
+  // It is allowed to set the position beyond the end of the file. EOF error
+  // should not occur.
+  StateFailed = E.setStreamState(
+      StateFailed, StreamState::getOpened(Desc, ErrorNone | ErrorFError, true));
   C.addTransition(StateFailed, E.getFailureNoteTag(this, C));
 }
 
diff --git a/clang/test/Analysis/stream-error.c b/clang/test/Analysis/stream-error.c
index 88f7de4234ffb4..7f9116ff401445 100644
--- a/clang/test/Analysis/stream-error.c
+++ b/clang/test/Analysis/stream-error.c
@@ -365,27 +365,22 @@ void error_fseek(void) {
     return;
   int rc = fseek(F, 1, SEEK_SET);
   if (rc) {
+    clang_analyzer_eval(rc == -1);     // expected-warning {{TRUE}}
     int IsFEof = feof(F), IsFError = ferror(F);
-    // Get feof or ferror or no error.
-    clang_analyzer_eval(IsFEof || IsFError);
-    // expected-warning@-1 {{FALSE}}
-    // expected-warning@-2 {{TRUE}}
-    clang_analyzer_eval(IsFEof && IsFError); // expected-warning {{FALSE}}
+    // Get ferror or no error.
+    clang_analyzer_eval(IsFError);     // expected-warning {{FALSE}} \
+                                       // expected-warning {{TRUE}}
+    clang_analyzer_eval(IsFEof);       // expected-warning {{FALSE}}
     // Error flags should not change.
-    if (IsFEof)
-      clang_analyzer_eval(feof(F)); // expected-warning {{TRUE}}
-    else
-      clang_analyzer_eval(feof(F)); // expected-warning {{FALSE}}
+    clang_analyzer_eval(feof(F));      // expected-warning {{FALSE}}
     if (IsFError)
-      clang_analyzer_eval(ferror(F)); // expected-warning {{TRUE}}
-    else
-      clang_analyzer_eval(ferror(F)); // expected-warning {{FALSE}}
+      clang_analyzer_eval(ferror(F));  // expected-warning {{TRUE}}
   } else {
-    clang_analyzer_eval(feof(F));   // expected-warning {{FALSE}}
-    clang_analyzer_eval(ferror(F)); // expected-warning {{FALSE}}
+    clang_analyzer_eval(feof(F));      // expected-warning {{FALSE}}
+    clang_analyzer_eval(ferror(F));    // expected-warning {{FALSE}}
     // Error flags should not change.
-    clang_analyzer_eval(feof(F));   // expected-warning {{FALSE}}
-    clang_analyzer_eval(ferror(F)); // expected-warning {{FALSE}}
+    clang_analyzer_eval(feof(F));      // expected-warning {{FALSE}}
+    clang_analyzer_eval(ferror(F));    // expected-warning {{FALSE}}
   }
   fclose(F);
 }
@@ -396,15 +391,13 @@ void error_fseeko(void) {
     return;
   int rc = fseeko(F, 1, SEEK_SET);
   if (rc) {
-    int IsFEof = feof(F), IsFError = ferror(F);
-    // Get feof or ferror or no error.
-    clang_analyzer_eval(IsFEof || IsFError);
-    // expected-warning@-1 {{FALSE}}
-    // expected-warning@-2 {{TRUE}}
-    clang_analyzer_eval(IsFEof && IsFError); // expected-warning {{FALSE}}
+    // Get ferror or no error.
+    clang_analyzer_eval(ferror(F));  // expected-warning {{FALSE}} \
+                                     // expected-warning {{TRUE}}
+    clang_analyzer_eval(feof(F));    // expected-warning {{FALSE}}
   } else {
-    clang_analyzer_eval(feof(F));   // expected-warning {{FALSE}}
-    clang_analyzer_eval(ferror(F)); // expected-warning {{FALSE}}
+    clang_analyzer_eval(feof(F));    // expected-warning {{FALSE}}
+    clang_analyzer_eval(ferror(F));  // expected-warning {{FALSE}}
   }
   fclose(F);
 }
@@ -414,7 +407,7 @@ void error_fseek_0(void) {
   if (!F)
     return;
   int rc = fseek(F, 0, SEEK_SET);
-  if (rc) {
+  if (rc == -1) {
     int IsFEof = feof(F), IsFError = ferror(F);
     // Get ferror or no error, but not feof.
     clang_analyzer_eval(IsFError);
diff --git a/clang/test/Analysis/stream-note.c b/clang/test/Analysis/stream-note.c
index f77cd4aa62841d..54ea699f46674e 100644
--- a/clang/test/Analysis/stream-note.c
+++ b/clang/test/Analysis/stream-note.c
@@ -226,10 +226,39 @@ void check_indeterminate_fseek(void) {
     return;
   int Ret = fseek(F, 1, SEEK_SET);  // expected-note {{Assuming this stream operation fails}}
   if (Ret) {                        // expected-note {{Taking true branch}} \
-                                    // expected-note {{'Ret' is not equal to 0}}
+                                    // expected-note {{'Ret' is -1}}
     char Buf[2];
     fwrite(Buf, 1, 2, F);           // expected-warning {{might be 'indeterminate'}} \
                                     // expected-note {{might be 'indeterminate'}}
   }
   fclose(F);
 }
+
+void error_fseek_ftell(void) {
+  FILE *F = fopen("file", "r");
+  if (!F)                 // expected-note {{Taking false branch}} \
+                          // expected-note {{'F' is non-null}}
+    return;
+  fseek(F, 0, SEEK_END);  // expected-note {{Assuming this stream operation fails}}
+  long size = ftell(F);   // expected-warning {{might be 'indeterminate'}} \
+                          // expected-note {{might be 'indeterminate'}}
+  if (size == -1) {
+    fclose(F);
+    return;
+  }
+  if (size == 1)
+    fprintf(F, "abcd");
+  fclose(F);
+}
+
+void error_fseek_read_eof(void) {
+  FILE *F = fopen("file", "r");
+  if (!F)
+    return;
+  if (fseek(F, 22, SEEK_SET) == -1) {
+    fclose(F);
+    return;
+  }
+  fgetc(F); // no warning
+  fclose(F);
+}

[benshi001]

LGTM