[PATCH] D153363: [clang][analyzer] No end-of-file when seek to file begin.

[Balázs Kéri via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG2eefd19613b8: [clang][analyzer] No end-of-file when seek to 
file begin. (authored by balazske).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153363/new/

https://reviews.llvm.org/D153363

Files:
  clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
  clang/test/Analysis/stream-error.c

Index: clang/test/Analysis/stream-error.c
===
--- clang/test/Analysis/stream-error.c
+++ clang/test/Analysis/stream-error.c
@@ -146,7 +146,7 @@
   FILE *F = fopen("file", "r");
   if (!F)
 return;
-  int rc = fseek(F, 0, SEEK_SET);
+  int rc = fseek(F, 1, SEEK_SET);
   if (rc) {
 int IsFEof = feof(F), IsFError = ferror(F);
 // Get feof or ferror or no error.
@@ -173,6 +173,35 @@
   fclose(F);
 }
 
+void error_fseek_0(void) {
+  FILE *F = fopen("file", "r");
+  if (!F)
+return;
+  int rc = fseek(F, 0, SEEK_SET);
+  if (rc) {
+int IsFEof = feof(F), IsFError = ferror(F);
+// Get ferror or no error, but not feof.
+clang_analyzer_eval(IsFError);
+// expected-warning@-1 {{FALSE}}
+// expected-warning@-2 {{TRUE}}
+clang_analyzer_eval(IsFEof);
+// expected-warning@-1 {{FALSE}}
+// Error flags should not change.
+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}}
+  } else {
+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}}
+  }
+  fclose(F);
+}
+
 void error_indeterminate(void) {
   FILE *F = fopen("file", "r+");
   if (!F)
Index: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -285,7 +285,14 @@
 0}},
   };
 
+  /// Expanded value of EOF, empty before initialization.
   mutable std::optional EofVal;
+  /// Expanded value of SEEK_SET, 0 if not found.
+  mutable int SeekSetVal = 0;
+  /// Expanded value of SEEK_CUR, 1 if not found.
+  mutable int SeekCurVal = 1;
+  /// Expanded value of SEEK_END, 2 if not found.
+  mutable int SeekEndVal = 2;
 
   void evalFopen(const FnDescription *Desc, const CallEvent &Call,
  CheckerContext &C) const;
@@ -432,7 +439,7 @@
 });
   }
 
-  void initEof(CheckerContext &C) const {
+  void initMacroValues(CheckerContext &C) const {
 if (EofVal)
   return;
 
@@ -441,6 +448,15 @@
   EofVal = *OptInt;
 else
   EofVal = -1;
+if (const std::optional OptInt =
+tryExpandAsInteger("SEEK_SET", C.getPreprocessor()))
+  SeekSetVal = *OptInt;
+if (const std::optional OptInt =
+tryExpandAsInteger("SEEK_END", C.getPreprocessor()))
+  SeekEndVal = *OptInt;
+if (const std::optional OptInt =
+tryExpandAsInteger("SEEK_CUR", C.getPreprocessor()))
+  SeekCurVal = *OptInt;
   }
 
   /// Searches for the ExplodedNode where the file descriptor was acquired for
@@ -488,7 +504,7 @@
 
 void StreamChecker::checkPreCall(const CallEvent &Call,
  CheckerContext &C) const {
-  initEof(C);
+  initMacroValues(C);
 
   const FnDescription *Desc = lookupFn(Call);
   if (!Desc || !Desc->PreFn)
@@ -786,6 +802,11 @@
   if (!State->get(StreamSym))
 return;
 
+  const llvm::APSInt *PosV =
+  C.getSValBuilder().getKnownValue(State, Call.getArgSVal(1));
+  const llvm::APSInt *WhenceV =
+  C.getSValBuilder().getKnownValue(State, Call.getArgSVal(2));
+
   DefinedSVal RetVal = makeRetVal(C, CE);
 
   // Make expression result.
@@ -804,9 +825,12 @@
   // 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 = StateFailed->set(
-  StreamSym,
-  StreamState::getOpened(Desc, ErrorNone | ErrorFEof | ErrorFError, true));
+  StreamSym, StreamState::getOpened(Desc, NewErrS, true));
 
   C.addTransition(StateNotFailed);
   C.addTransition(StateFailed, constructSetEofNoteTag(C, StreamSym));
@@ -1153,7 +1177,7 @@
 return State;
 
   int64_t X = CI->getValue().getSExtValue();
-  if (X >= 0 && X <= 2)
+  if (X == SeekSetVal || X == SeekCurVal || X == SeekEndVal)
 return State;
 
   if (Exp

[PATCH] D153363: [clang][analyzer] No end-of-file when seek to file begin.

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy accepted this revision.
donat.nagy added a comment.
This revision is now accepted and ready to land.

LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153363/new/

https://reviews.llvm.org/D153363

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153363: [clang][analyzer] No end-of-file when seek to file begin.

[Balázs Kéri via Phabricator via cfe-commits]

balazske added a comment.

This case 

 is an example of what can be fixed by the change.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153363/new/

https://reviews.llvm.org/D153363

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153363: [clang][analyzer] No end-of-file when seek to file begin.

[Balázs Kéri via Phabricator via cfe-commits]

balazske created this revision.
Herald added subscribers: steakhal, manas, ASDenysPetrov, martong, gamesh411, 
dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, szepet, 
baloghadamsoftware, xazax.hun.
Herald added a reviewer: Szelethus.
Herald added a reviewer: NoQ.
Herald added a project: All.
balazske requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

If `fseek` is used with 0 position and SEEK_SET it sets the position
to the start of the file. This should not cause FEOF (end of file) error.
The case of an empty file is not handled for simplification.
It is not exactly defined in what cases `fseek` produces the different
error states. Normally feof should not happen at all because it is
possible to set the position after the end of file, but previous tests
showed that still feof (and any other error cases) can happen.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D153363

Files:
  clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
  clang/test/Analysis/stream-error.c

Index: clang/test/Analysis/stream-error.c
===
--- clang/test/Analysis/stream-error.c
+++ clang/test/Analysis/stream-error.c
@@ -146,7 +146,7 @@
   FILE *F = fopen("file", "r");
   if (!F)
 return;
-  int rc = fseek(F, 0, SEEK_SET);
+  int rc = fseek(F, 1, SEEK_SET);
   if (rc) {
 int IsFEof = feof(F), IsFError = ferror(F);
 // Get feof or ferror or no error.
@@ -173,6 +173,35 @@
   fclose(F);
 }
 
+void error_fseek_0(void) {
+  FILE *F = fopen("file", "r");
+  if (!F)
+return;
+  int rc = fseek(F, 0, SEEK_SET);
+  if (rc) {
+int IsFEof = feof(F), IsFError = ferror(F);
+// Get ferror or no error, but not feof.
+clang_analyzer_eval(IsFError);
+// expected-warning@-1 {{FALSE}}
+// expected-warning@-2 {{TRUE}}
+clang_analyzer_eval(IsFEof);
+// expected-warning@-1 {{FALSE}}
+// Error flags should not change.
+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}}
+  } else {
+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}}
+  }
+  fclose(F);
+}
+
 void error_indeterminate(void) {
   FILE *F = fopen("file", "r+");
   if (!F)
Index: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -285,7 +285,14 @@
 0}},
   };
 
+  /// Expanded value of EOF, empty before initialization.
   mutable std::optional EofVal;
+  /// Expanded value of SEEK_SET, 0 if not found.
+  mutable int SeekSetVal = 0;
+  /// Expanded value of SEEK_CUR, 1 if not found.
+  mutable int SeekCurVal = 1;
+  /// Expanded value of SEEK_END, 2 if not found.
+  mutable int SeekEndVal = 2;
 
   void evalFopen(const FnDescription *Desc, const CallEvent &Call,
  CheckerContext &C) const;
@@ -432,7 +439,7 @@
 });
   }
 
-  void initEof(CheckerContext &C) const {
+  void initMacroValues(CheckerContext &C) const {
 if (EofVal)
   return;
 
@@ -441,6 +448,15 @@
   EofVal = *OptInt;
 else
   EofVal = -1;
+if (const std::optional OptInt =
+tryExpandAsInteger("SEEK_SET", C.getPreprocessor()))
+  SeekSetVal = *OptInt;
+if (const std::optional OptInt =
+tryExpandAsInteger("SEEK_END", C.getPreprocessor()))
+  SeekEndVal = *OptInt;
+if (const std::optional OptInt =
+tryExpandAsInteger("SEEK_CUR", C.getPreprocessor()))
+  SeekCurVal = *OptInt;
   }
 
   /// Searches for the ExplodedNode where the file descriptor was acquired for
@@ -488,7 +504,7 @@
 
 void StreamChecker::checkPreCall(const CallEvent &Call,
  CheckerContext &C) const {
-  initEof(C);
+  initMacroValues(C);
 
   const FnDescription *Desc = lookupFn(Call);
   if (!Desc || !Desc->PreFn)
@@ -786,6 +802,11 @@
   if (!State->get(StreamSym))
 return;
 
+  const llvm::APSInt *PosV =
+  C.getSValBuilder().getKnownValue(State, Call.getArgSVal(1));
+  const llvm::APSInt *WhenceV =
+  C.getSValBuilder().getKnownValue(State, Call.getArgSVal(2));
+
   DefinedSVal RetVal = makeRetVal(C, CE);
 
   // Make expression result.
@@ -804,9 +825,12 @@
   // 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 ne