[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

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

balazske abandoned this revision.
balazske added a comment.

Yes, closing it. (Will change `StreamErrorState` probably to bitfield later.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78374



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

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

We can close this now, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78374



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

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

> Is there a significant gain behind doing that instead of doing a state split 
> immediately? It feels unnatural to delay the split. It doesn't accurately 
> represent how the code would run. Are we supposed to do this at every 
> precall? What about other checkers that may rely on this one?

The actual gain is that we somewhat reduce the exponential growth of the 
exploded graph. With such a Schrödinger's cat pattern we delay it to the point 
where it is actually checked, it checked at all.




Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:39
+  /// There is an execution path with none of the error flags set.
+  bool NoError = true;
+  /// There is an execution path with EOF indicator set.

`bool NoError:1 = true` etc. Why not use bit fields for a struct of boolans?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78374



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

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

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

balazske added a comment.

Created new revisions for parts of this change and a bit improved: D80009 
 and others in "Stack".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78374



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

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

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

balazske added a comment.

We can assume that size of a file does not change by external (to the analyzed 
program) events so the **EOF** state can not disappear. (Is this true for 
`stdin`, or it has never an **EOF**?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78374



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

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

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

balazske added a comment.

Adding a state chart may help something but does not tell the full picture 
because conditions on the transitions are not trivial to indicate and follow. 
And the state chart is too complex to draw it in ASCII-art format.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78374



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

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

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

balazske added a comment.

The indeterminate position is mentioned only at fread and fwrite. I do not know 
if it is reasonable to make the indeterminate position in other cases. Th 
indeterminate position is separate from error flags because of "clearerr" that 
clears the error flag but not the indeterminate position (if this is the right 
way of how it works), and because even **ferror** does not imply an 
indeterminate position always like after a write to a file opened in read mode. 
Then it is a detail question after what functions to set indeterminate position 
and before what functions check for it, the support for it is still needed. The 
current way of its handling seems correct to me, indeterminate position is not 
allowed before read or write but no problem before fseek (at least with an 
absolute seek value, this is not checked now).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78374



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

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

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

balazske added a comment.

The problem with small changes is that still we (at least I who writes the 
code) should know the final goal (and this can be hard if I have multiple not 
related problems to work on and everything goes forward by little pieces spread 
out in a long time). (Also the reviewer needs to know why a change is made if 
it is not obvious because it is part of a bigger to-be-added "logic".) 
Otherwise something already done can turn out to be wrong later, as is the case 
already now with these changes. My goal was to add the file handling functions 
one by one and adjust the existing code to work with the current implemented 
functions (probably something already added must be changed to adapt to the new 
conditions).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78374



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

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D78374#2033826 , @balazske wrote:

> I think this is a expected way of how it works, failure of a stream operation 
> does not necessarily depend on result of a previous operation. So any 
> operation can fail or not, independent of the previous error state (the 
> "ferror" may happen because a temporary disk error and it is worth to retry 
> the read, at least a non-`fread` function where the "indeterminate file 
> position" problem does not happen). This would make things more simple. Only 
> the EOF is probably exception, in EOF state any read operation fails with EOF.


Right. But, speaking about generality, would it be reasonably to assume that 
all (or almost all) operations may also result in an indeterminate file 
indicator? Because if so, we could, and totally should strip those changes from 
the fread/fwrite ones and make a new revision.

That is by the way true for a lot of other changes, such as the delayed state 
split or the new error state struct. I mean not to burden you by micromanaging 
a lot of small patches, but it makes reviewing far faster for me.

> (But I do not know if it is possible that a stream in EOF state becomes 
> non-EOF because new data appears at the end, if other program can write into 
> it or it is some form of "pipe" or special file. If this is true even EOF 
> needs no special handling.)

That is a great question, but I strongly believe that it cannot without 
`freopen`. I mean, if I were to create an io library, I would set a pointer to 
the beginning and the end of the file, and know we hit eof is the current 
pointer hits the end point. Besides, how could anyone change the `FILE *` 
object in a non-analyzable way?

In any case, its a reasonable to assume it won't change.




Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:937-939
+new BuiltinBug(this, "Stream is in error state",
+   "Function called when stream has already error state "
+   "flag (EOF or error) set."));

Since this isn't a bug but rather a code smell, a note would be a nice 
compliment.
"Use clearerr() to clear the error flag"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78374



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

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

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

balazske marked 2 inline comments as done.
balazske added a comment.

The intent is to model the fread-fwrite function failure by returning the error 
value and set the stream into error state. The error state is a composite of 
**ferror** and **feof**. The questions are now, at what case do these functions 
fail and with what error type?




Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:107
+  /// This value applies to all execution paths in ErrorState except the FEOF
+  /// case. In more detail, an EOF+indeterminate state is the same as EOF 
state.
+  bool FilePositionIndeterminate = false;

Szelethus wrote:
> The standard suggests that this happens on FERROR, not FEOF.
> 
> > If an error occurs, the resulting value of the file position indicator for 
> > the stream is indeterminate. If a partial element is read, its value is 
> > indeterminate.
> 
> It would be wild if the stream wouldn't set the FEOF flag after reaching the 
> end of the file. Also, I would imagine FEOF being usually implemented with 
> the file position indicator.
> 
> ```lang=bash
> $ cat test.cpp`
> ```
> ```lang=cpp
> #include "stdio.h"
> 
> int main() {
>   FILE *F = fopen("test.cpp", "r");
>   char Buf[1024];
>   const int READ_COUNT = 9;
>   if (READ_COUNT != fread(Buf, sizeof(char), READ_COUNT, F)) {
> printf("%i\n", feof(F));
>   }
> }
> ```
> ```lang=bash
> $ build/bin/clang++ test.cpp && ./a.out 
> 1
> 
> ```
> 
> Operations on an EOF and indeterminate stream are very different.
I wanted to say in that comment that `FilePositionIndeterminate` should be 
ignored if the `ErrorState` is `ErrorFEof`. In other words the file is never 
indeterminate if in EOF state.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78374



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

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

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

balazske added a comment.

I think this is a expected way of how it works, failure of a stream operation 
does not necessarily depend on result of a previous operation. So any operation 
can fail or not, independent of the previous error state (the "ferror" may 
happen because a temporary disk error and it is worth to retry the read, at 
least a non-`fread` function where the "indeterminate file position" problem 
does not happen). This would make things more simple. Only the EOF is probably 
exception, in EOF state any read operation fails with EOF. (But I do not know 
if it is possible that a stream in EOF state becomes non-EOF because new data 
appears at the end, if other program can write into it or it is some form of 
"pipe" or special file. If this is true even EOF needs no special handling.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78374



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

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus requested changes to this revision.
Szelethus added a comment.
This revision now requires changes to proceed.

I'm sorry for the late review -- please know that this isn't the first time me 
taking a look, this is a complex issue. I find navigating your phabricator 
comments a bit difficult, it would help a lot if you would accompany them with 
a lot of code examples, and a lot more tests in the actual patch.

Error handling is super annoying with streams. Take a look at this:

  #include "stdio.h"
  
  int main() {
FILE *F = fopen("test.cpp", "r");
putc('c', F);
printf("error: %i\n", ferror(F));
char Buf[1024];
const int READ_COUNT = 5;
if (READ_COUNT != fread(Buf, sizeof(char), READ_COUNT, F))
  printf("eof: %i\n", ferror(F));
else
  printf("%s\n", Buf);
fclose(F);
  }

  $ clang++ test.cpp && ./a.out 
  error: 1
  #incl

Should we allow this? It seems like the error flag was set, but the actual 
problem is unrelated to the read, so the read still works fine.

I feel like we're constantly changing what we're shooting for, and that implies 
immediate code changes. Let's take a step back, draw a state machine (no error, 
feof, ferror, indeterminate state), and just define what is an error, what is a 
code smell, what is perfectly fine, because we don't seem to be on the clear on 
this. We need to follow and agree on the C standard in a very literal sense, 
and we're not there just yet. The amount and placement of the state splits seem 
to cause a lot of confusion and we're not ready for that discussion just yet.

D70470  is a great example for a patch with a 
state machine.

In D78374#2023195 , @balazske wrote:

> Pre-statement checks for `fread` and `fwrite` filter out the cases when the 
> position is "indeterminate", because in those states it is fatal error to 
> call the function. Otherwise if not only the **EOF** flag is set initially 
> (at start of `fread`) in `ErrorState` the `fread` should not generate state 
> when other errors are possible (or none) (this contains an execution where 
> initially EOF is set, after the function no **EOF** is set or **FERROR** is 
> set). So the **EOF** initial state must be handled separately, probably split 
> from the rest of the error state.


I read through this many times but I just don't understand what you mean, could 
you rephrase this please?

In D78374#2031694 , @balazske wrote:

> Added state split before fread to make warning for error state possible.


Is there a significant gain behind doing that instead of doing a state split 
immediately? It feels unnatural to delay the split. It doesn't accurately 
represent how the code would run. Are we supposed to do this at every precall? 
What about other checkers that may rely on this one?




Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:37
+/// Note: A stream has either FEOF or FERROR set but not both at the same time.
+struct StreamErrorState {
+  /// There is an execution path with none of the error flags set.

balazske wrote:
> Szelethus wrote:
> > We're not describing the error state of a stream here, but rather 
> > //possible// error states, so the name should reflect that.
> I do not think that `StreamPossibleErrorState` is better, it is anyway a 
> //state// that can contain any information. It is an error related state even 
> if it defines not one exact error.
Alright, you convinced me.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:107
+  /// This value applies to all execution paths in ErrorState except the FEOF
+  /// case. In more detail, an EOF+indeterminate state is the same as EOF 
state.
+  bool FilePositionIndeterminate = false;

The standard suggests that this happens on FERROR, not FEOF.

> If an error occurs, the resulting value of the file position indicator for 
> the stream is indeterminate. If a partial element is read, its value is 
> indeterminate.

It would be wild if the stream wouldn't set the FEOF flag after reaching the 
end of the file. Also, I would imagine FEOF being usually implemented with the 
file position indicator.

```lang=bash
$ cat test.cpp`
```
```lang=cpp
#include "stdio.h"

int main() {
  FILE *F = fopen("test.cpp", "r");
  char Buf[1024];
  const int READ_COUNT = 9;
  if (READ_COUNT != fread(Buf, sizeof(char), READ_COUNT, F)) {
printf("%i\n", feof(F));
  }
}
```
```lang=bash
$ build/bin/clang++ test.cpp && ./a.out 
1

```

Operations on an EOF and indeterminate stream are very different.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78374



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

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

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

balazske updated this revision to Diff 263447.
balazske added a comment.

Added state split before fread to make warning for error state possible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78374

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
@@ -3,6 +3,7 @@
 #include "Inputs/system-header-simulator.h"
 
 void clang_analyzer_eval(int);
+void clang_analyzer_warnIfReached();
 void StreamTesterChecker_make_feof_stream(FILE *);
 void StreamTesterChecker_make_ferror_stream(FILE *);
 
@@ -53,6 +54,80 @@
   fclose(F);
 }
 
+void error_fread() {
+  FILE *F = tmpfile();
+  if (!F)
+return;
+  int Ret = fread(0, 1, 10, F);
+  if (Ret == 10) {
+clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}}
+  } else {
+clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{TRUE}}
+if (feof(F)) {
+  clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+  fread(0, 1, 10, F); // expected-warning {{Function called when stream has already error state}}
+}
+if (ferror(F)) {
+  clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+  fread(0, 1, 10, F); // expected-warning {{Stream might be in indeterminate state}}
+}
+  }
+  fclose(F);
+  Ret = fread(0, 1, 10, F); // expected-warning {{Stream might be already closed}}
+}
+
+void error_fwrite() {
+  FILE *F = tmpfile();
+  if (!F)
+return;
+  int Ret = fwrite(0, 1, 10, F);
+  if (Ret == 10) {
+clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}}
+  } else {
+clang_analyzer_eval(feof(F));   // expected-warning {{FALSE}}
+clang_analyzer_eval(ferror(F)); // expected-warning {{TRUE}}
+fwrite(0, 1, 10, F);// expected-warning {{Stream might be in indeterminate state}}
+  }
+  fclose(F);
+  Ret = fwrite(0, 1, 10, F); // expected-warning {{Stream might be already closed}}
+}
+
+void freadwrite_zerosize(FILE *F) {
+  fwrite(0, 1, 0, F);
+  fwrite(0, 0, 1, F);
+  fread(0, 1, 0, F);
+  fread(0, 0, 1, F);
+}
+
+void freadwrite_zerosize_errorstate(FILE *F) {
+  fwrite(0, 1, 0, F);
+  fwrite(0, 0, 1, F);
+  fread(0, 1, 0, F); // expected-warning {{Function called when stream has already error state}}
+  fread(0, 0, 1, F); // expected-warning {{Function called when stream has already error state}}
+}
+
+void error_fread_fwrite_zerosize() {
+  FILE *F = fopen("file", "r");
+  if (!F)
+return;
+
+  freadwrite_zerosize(F);
+  clang_analyzer_eval(feof(F));   // expected-warning {{FALSE}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{FALSE}}
+
+  StreamTesterChecker_make_ferror_stream(F);
+  freadwrite_zerosize_errorstate(F);
+  clang_analyzer_eval(feof(F));   // expected-warning {{FALSE}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{TRUE}}
+
+  StreamTesterChecker_make_feof_stream(F);
+  freadwrite_zerosize_errorstate(F);
+  clang_analyzer_eval(feof(F));   // expected-warning {{TRUE}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{FALSE}}
+
+  fclose(F);
+}
+
 void error_fseek() {
   FILE *F = fopen("file", "r");
   if (!F)
@@ -83,3 +158,34 @@
   }
   fclose(F);
 }
+
+void error_fseek_indeterminate() {
+  FILE *F = fopen("file", "r");
+  if (!F)
+return;
+  int rc = fseek(F, 0, SEEK_SET);
+  if (rc) {
+if (feof(F)) {
+  fwrite(0, 1, 10, F); // no warning
+} else if (ferror(F)) {
+  fwrite(0, 1, 10, F); // expected-warning {{Stream might be in indeterminate state}}
+} else {
+  fwrite(0, 1, 10, F); // expected-warning {{Stream might be in indeterminate state}}
+}
+  }
+  fclose(F);
+}
+
+void error_clearerr_indeterminate() {
+  FILE *F = fopen("file", "r");
+  if (!F)
+return;
+  int rc = fseek(F, 0, SEEK_SET);
+  if (rc) {
+if (!feof(F)) {
+  clearerr(F);
+  fwrite(0, 1, 10, F); // expected-warning {{Stream might be in indeterminate state}}
+}
+  }
+  fclose(F);
+}
Index: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -19,15 +19,66 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
+#include 
 
 using namespace clang;
 using namespace ento;
+using namespace std::placeholders;
 
 namespace {
 
 struct FnDescription;
 
+/// State of the stream related error flags.
+/// It indicates possibility of different error states.
+/// In other sense, every set flag means a separate 

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

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

balazske added a comment.

I am not sure if this FEOF thing is such a big problem. The modeling is not 
exact but practically this does not cause big problems and still better than 
before this patch. The problem is, if a `fread` is made and the stream is 
already in **EOF** state the (modeled) `fread` may still succeed or fail again 
with other error, according to the current way of modeling it. The calling 
program must handle still every possible error after `fread` regardless what 
error was there before. I am not sure if false positives can appear because 
this issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78374



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

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

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

balazske added a comment.

The "indeterminate file position" case is now handled separately and is always 
error. The current implementation looks to be wrong because the indeterminate 
position is set at `fread` when a **FEOF** error is produced. This can be 
solved probably only with more state splits, because feof and ferror flags have 
more different behavior and can not be handled together the same way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78374



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

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:478
+
+  // FIXME: Check for stream in EOF state?
+

balazske wrote:
> balazske wrote:
> > Szelethus wrote:
> > > balazske wrote:
> > > > Szelethus wrote:
> > > > > Will that be the next patch? :D Eager to see it!
> > > > It may be too big change to add it as well here (or cause more 
> > > > difficulties). But it should be a  check for `ErrorFEof` in the 
> > > > `ErrorState` (and add another bug type). This is not a fatal error.
> > > > 
> > > > It may be more difficult to make `fread` return the feof error again if 
> > > > it starts already with feof state. (Because the state with FEof should 
> > > > be split from the generic error state that can contain other possible 
> > > > errors.)
> > > > It may be too big change to add it as well here (or cause more 
> > > > difficulties). But it should be a check for ErrorFEof in the ErrorState 
> > > > (and add another bug type). This is not a fatal error.
> > > 
> > > Oh, yea, right, we went over this once :) What I really meant, is an EOF 
> > > related error, like reading a variable with an EOF value in any context 
> > > that isn't a comparison to the actual `EOF` (which in many contexts still 
> > > isn't a //fatal// error, but is far more an indicator of code smell). But 
> > > I'm just thinking aloud, please proceed with this project however it is 
> > > convenient to you!
> > > 
> > > > It may be more difficult to make fread return the feof error again if 
> > > > it starts already with feof state. (Because the state with FEof should 
> > > > be split from the generic error state that can contain other possible 
> > > > errors.)
> > > 
> > > I gave this plenty of thought, how do you imagine the problem of us not 
> > > splitting up to 3 states to show up? Suppose we're calling fread on a 
> > > stream where the error state is either EOF or ERROR, but we don't know 
> > > which. We could just leave the `StreamErrorState` as-is, couldn't we?
> > > reading a variable with an EOF value in any context that isn't a 
> > > comparison to the actual EOF
> > This could be another checker, or it can be integrated somehow into 
> > `ErrorReturnChecker` (that does something similar already). I mean, 
> > remembering if a variable contains **EOF** that comes from a function that 
> > may return **EOF** (otherwise the program can do nothing with a simple -1 
> > numerical value without getting the warning), then if any other thing is 
> > done with it than comparison to **EOF** (or passing it to other function) 
> > it can be an error case.
> > Suppose we're calling fread on a stream where the error state is either EOF 
> > or ERROR, but we don't know which. We could just leave the StreamErrorState 
> > as-is, couldn't we?
>  * If `fread` is called with **FEOF** flag, it returns 0 and **FEOF** remains.
>  * If `fread` is called with **FERROR** flag, it may fail (with any error) or 
> not. This is similar as calling `fread` in no-error state.
> 
> * If fread is called with FERROR flag, it may fail (with any error) or not. 
> This is similar as calling fread in no-error state.

Is it ever possible that an fread on a stream in FERROR returns non-zero and 
changes the streams state? Lets unpack this.

§7.19.8.1.2 states that
> The fread function reads, into the array pointed to by ptr, up to nmemb 
> elements whose size is specified by size, from the stream pointed to by 
> stream. **For each object, size calls are made to the fgetc function and the 
> results stored, in the order read, in an array of unsigned char exactly 
> overlaying the object.** The file position indicator for the stream (if 
> defined) is advanced by the number of characters successfully read. If an 
> error occurs, the resulting value of the file position indicator for the 
> stream is indeterminate. If a partial element is read, its value is 
> indeterminate.

§7.19.7.1.3 talks about the return value of fgetc:

> **If the end-of-file indicator for the stream is set, or if the stream is at 
> end-of-file, the end-of-file indicator for the stream is set and the fgetc 
> function returns EOF.**  Otherwise, the fgetc function returns the next 
> character from the input stream pointed to by stream. **If a read error 
> occurs, the error indicator for the stream is set and the fgetc function 
> returns EOF.**

Now, speaking of fgetc, its true that the standard doesn't specifically talk 
about what happens if the stream is already in error state, yet it does mention 
what happens if it is in eof-state, but then again, I guess its strongly 
implied that if the stream is an error state, a read error will occur right 
away, and the error state is preserved as specified.

If fread is little more then sequential calls to fgetc, I would believe that a 
stream is in FERROR, FERROR will be preserved, because fgetc wouldn't be able 
to read a single character. Additionally, as 

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

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

balazske marked 2 inline comments as done.
balazske added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:478
+
+  // FIXME: Check for stream in EOF state?
+

Szelethus wrote:
> balazske wrote:
> > Szelethus wrote:
> > > Will that be the next patch? :D Eager to see it!
> > It may be too big change to add it as well here (or cause more 
> > difficulties). But it should be a  check for `ErrorFEof` in the 
> > `ErrorState` (and add another bug type). This is not a fatal error.
> > 
> > It may be more difficult to make `fread` return the feof error again if it 
> > starts already with feof state. (Because the state with FEof should be 
> > split from the generic error state that can contain other possible errors.)
> > It may be too big change to add it as well here (or cause more 
> > difficulties). But it should be a check for ErrorFEof in the ErrorState 
> > (and add another bug type). This is not a fatal error.
> 
> Oh, yea, right, we went over this once :) What I really meant, is an EOF 
> related error, like reading a variable with an EOF value in any context that 
> isn't a comparison to the actual `EOF` (which in many contexts still isn't a 
> //fatal// error, but is far more an indicator of code smell). But I'm just 
> thinking aloud, please proceed with this project however it is convenient to 
> you!
> 
> > It may be more difficult to make fread return the feof error again if it 
> > starts already with feof state. (Because the state with FEof should be 
> > split from the generic error state that can contain other possible errors.)
> 
> I gave this plenty of thought, how do you imagine the problem of us not 
> splitting up to 3 states to show up? Suppose we're calling fread on a stream 
> where the error state is either EOF or ERROR, but we don't know which. We 
> could just leave the `StreamErrorState` as-is, couldn't we?
> reading a variable with an EOF value in any context that isn't a comparison 
> to the actual EOF
This could be another checker, or it can be integrated somehow into 
`ErrorReturnChecker` (that does something similar already). I mean, remembering 
if a variable contains **EOF** that comes from a function that may return 
**EOF** (otherwise the program can do nothing with a simple -1 numerical value 
without getting the warning), then if any other thing is done with it than 
comparison to **EOF** (or passing it to other function) it can be an error case.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:478
+
+  // FIXME: Check for stream in EOF state?
+

balazske wrote:
> Szelethus wrote:
> > balazske wrote:
> > > Szelethus wrote:
> > > > Will that be the next patch? :D Eager to see it!
> > > It may be too big change to add it as well here (or cause more 
> > > difficulties). But it should be a  check for `ErrorFEof` in the 
> > > `ErrorState` (and add another bug type). This is not a fatal error.
> > > 
> > > It may be more difficult to make `fread` return the feof error again if 
> > > it starts already with feof state. (Because the state with FEof should be 
> > > split from the generic error state that can contain other possible 
> > > errors.)
> > > It may be too big change to add it as well here (or cause more 
> > > difficulties). But it should be a check for ErrorFEof in the ErrorState 
> > > (and add another bug type). This is not a fatal error.
> > 
> > Oh, yea, right, we went over this once :) What I really meant, is an EOF 
> > related error, like reading a variable with an EOF value in any context 
> > that isn't a comparison to the actual `EOF` (which in many contexts still 
> > isn't a //fatal// error, but is far more an indicator of code smell). But 
> > I'm just thinking aloud, please proceed with this project however it is 
> > convenient to you!
> > 
> > > It may be more difficult to make fread return the feof error again if it 
> > > starts already with feof state. (Because the state with FEof should be 
> > > split from the generic error state that can contain other possible 
> > > errors.)
> > 
> > I gave this plenty of thought, how do you imagine the problem of us not 
> > splitting up to 3 states to show up? Suppose we're calling fread on a 
> > stream where the error state is either EOF or ERROR, but we don't know 
> > which. We could just leave the `StreamErrorState` as-is, couldn't we?
> > reading a variable with an EOF value in any context that isn't a comparison 
> > to the actual EOF
> This could be another checker, or it can be integrated somehow into 
> `ErrorReturnChecker` (that does something similar already). I mean, 
> remembering if a variable contains **EOF** that comes from a function that 
> may return **EOF** (otherwise the program can do nothing with a simple -1 
> numerical value without getting the warning), then if any other thing is done 
> with it than comparison to **EOF** (or passing it to other function) it can 
> 

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

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

balazske added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:152-158
 /// Get the value of the stream argument out of the passed call event.
 /// The call should contain a function that is described by Desc.
 SVal getStreamArg(const FnDescription *Desc, const CallEvent ) {
   assert(Desc && Desc->StreamArgNo != ArgNone &&
  "Try to get a non-existing stream argument.");
   return Call.getArgSVal(Desc->StreamArgNo);
 }

Szelethus wrote:
> If we add methods to `FnDescription`, we might as well add this as well, but 
> I don't insist.
Now this is again the only function that could be member, in the current form 
the assert can be done but not in the member form.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:37
+/// Note: A stream has either FEOF or FERROR set but not both at the same time.
+struct StreamErrorState {
+  /// There is an execution path with none of the error flags set.

Szelethus wrote:
> We're not describing the error state of a stream here, but rather 
> //possible// error states, so the name should reflect that.
I do not think that `StreamPossibleErrorState` is better, it is anyway a 
//state// that can contain any information. It is an error related state even 
if it defines not one exact error.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78374



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

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:478
+
+  // FIXME: Check for stream in EOF state?
+

balazske wrote:
> Szelethus wrote:
> > Will that be the next patch? :D Eager to see it!
> It may be too big change to add it as well here (or cause more difficulties). 
> But it should be a  check for `ErrorFEof` in the `ErrorState` (and add 
> another bug type). This is not a fatal error.
> 
> It may be more difficult to make `fread` return the feof error again if it 
> starts already with feof state. (Because the state with FEof should be split 
> from the generic error state that can contain other possible errors.)
> It may be too big change to add it as well here (or cause more difficulties). 
> But it should be a check for ErrorFEof in the ErrorState (and add another bug 
> type). This is not a fatal error.

Oh, yea, right, we went over this once :) What I really meant, is an EOF 
related error, like reading a variable with an EOF value in any context that 
isn't a comparison to the actual `EOF` (which in many contexts still isn't a 
//fatal// error, but is far more an indicator of code smell). But I'm just 
thinking aloud, please proceed with this project however it is convenient to 
you!

> It may be more difficult to make fread return the feof error again if it 
> starts already with feof state. (Because the state with FEof should be split 
> from the generic error state that can contain other possible errors.)

I gave this plenty of thought, how do you imagine the problem of us not 
splitting up to 3 states to show up? Suppose we're calling fread on a stream 
where the error state is either EOF or ERROR, but we don't know which. We could 
just leave the `StreamErrorState` as-is, couldn't we?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78374



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

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

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

balazske updated this revision to Diff 262058.
balazske marked 2 inline comments as done.
balazske added a comment.

Small review related fixes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78374

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
@@ -3,6 +3,7 @@
 #include "Inputs/system-header-simulator.h"
 
 void clang_analyzer_eval(int);
+void clang_analyzer_warnIfReached();
 void StreamTesterChecker_make_feof_stream(FILE *);
 void StreamTesterChecker_make_ferror_stream(FILE *);
 
@@ -53,6 +54,73 @@
   fclose(F);
 }
 
+void error_fread() {
+  FILE *F = tmpfile();
+  if (!F)
+return;
+  int Ret = fread(0, 1, 10, F);
+  if (Ret == 10) {
+clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}}
+  } else {
+clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{TRUE}}
+if (feof(F)) {
+  clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+  fread(0, 1, 10, F); // no warning
+}
+if (ferror(F)) {
+  clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+  fread(0, 1, 10, F); // expected-warning {{Stream might be in indeterminate state}}
+}
+  }
+  fclose(F);
+  Ret = fread(0, 1, 10, F); // expected-warning {{Stream might be already closed}}
+}
+
+void error_fwrite() {
+  FILE *F = tmpfile();
+  if (!F)
+return;
+  int Ret = fwrite(0, 1, 10, F);
+  if (Ret == 10) {
+clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}}
+  } else {
+clang_analyzer_eval(feof(F));   // expected-warning {{FALSE}}
+clang_analyzer_eval(ferror(F)); // expected-warning {{TRUE}}
+fwrite(0, 1, 10, F);// expected-warning {{Stream might be in indeterminate state}}
+  }
+  fclose(F);
+  Ret = fwrite(0, 1, 10, F); // expected-warning {{Stream might be already closed}}
+}
+
+void freadwrite_zerosize(FILE *F) {
+  fwrite(0, 1, 0, F);
+  fwrite(0, 0, 1, F);
+  fread(0, 1, 0, F);
+  fread(0, 0, 1, F);
+}
+
+void error_fread_fwrite_zerosize() {
+  FILE *F = fopen("file", "r");
+  if (!F)
+return;
+
+  freadwrite_zerosize(F);
+  clang_analyzer_eval(feof(F));   // expected-warning {{FALSE}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{FALSE}}
+
+  StreamTesterChecker_make_ferror_stream(F);
+  freadwrite_zerosize(F);
+  clang_analyzer_eval(feof(F));   // expected-warning {{FALSE}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{TRUE}}
+
+  StreamTesterChecker_make_feof_stream(F);
+  freadwrite_zerosize(F);
+  clang_analyzer_eval(feof(F));   // expected-warning {{TRUE}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{FALSE}}
+
+  fclose(F);
+}
+
 void error_fseek() {
   FILE *F = fopen("file", "r");
   if (!F)
@@ -83,3 +151,34 @@
   }
   fclose(F);
 }
+
+void error_fseek_indeterminate() {
+  FILE *F = fopen("file", "r");
+  if (!F)
+return;
+  int rc = fseek(F, 0, SEEK_SET);
+  if (rc) {
+if (feof(F)) {
+  fwrite(0, 1, 10, F); // no warning
+} else if (ferror(F)) {
+  fwrite(0, 1, 10, F); // expected-warning {{Stream might be in indeterminate state}}
+} else {
+  fwrite(0, 1, 10, F); // expected-warning {{Stream might be in indeterminate state}}
+}
+  }
+  fclose(F);
+}
+
+void error_clearerr_indeterminate() {
+  FILE *F = fopen("file", "r");
+  if (!F)
+return;
+  int rc = fseek(F, 0, SEEK_SET);
+  if (rc) {
+if (!feof(F)) {
+  clearerr(F);
+  fwrite(0, 1, 10, F); // expected-warning {{Stream might be in indeterminate state}}
+}
+  }
+  fclose(F);
+}
Index: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -19,15 +19,64 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
+#include 
 
 using namespace clang;
 using namespace ento;
+using namespace std::placeholders;
 
 namespace {
 
 struct FnDescription;
 
+/// State of the stream related error flags.
+/// It indicates possibility of different error states.
+/// In other sense, every set flag means a separate execution path.
+/// At least one of the bool flags must be set to have a valid StreamErrorState.
+/// Note: A stream has either FEOF or FERROR set but not both at the same time.
+struct StreamErrorState {
+  /// There is an execution path with none of the error flags set.
+  bool None = true;
+  /// There is an execution path with EOF indicator set.
+  bool FEof = false;
+  /// There 

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

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

balazske marked 3 inline comments as done.
balazske added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:207-210
+  {{"fread", 4},
+   {::preFread,
+std::bind(::evalFreadFwrite, _1, _2, _3, _4,
+  ErrorFEof | ErrorFError),

Szelethus wrote:
> What does this mean? I guess not the possible states after an fread call, but 
> rather the possible states after a **failed** fread call, but then...
Name of the parameter may be misleading, for `evalFreadFwrite` the last 
argument describes the new state after the operation has failed.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:225-228
   {{"feof", 1},
{::preDefault,
-::evalFeofFerror,
-0,
-{StreamState::FError, StreamState::NoError}}},
-  // Note: ferror can result in Unknown if at the call there is a
-  // PossibleErrors with all 3 error states (including NoError).
-  // Then if ferror is false the remaining error could be FEof or NoError.
+std::bind(::evalFeofFerror, _1, _2, _3, _4, ErrorFEof),
+0}},

Szelethus wrote:
> ...this doesn't make much sense, `feof` doesn't **cause** and error, it 
> merely detects it.
At `evalFeofFerror` the last parameter simply indicates if it is the `feof` or 
the `ferror` case.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:478
+
+  // FIXME: Check for stream in EOF state?
+

Szelethus wrote:
> Will that be the next patch? :D Eager to see it!
It may be too big change to add it as well here (or cause more difficulties). 
But it should be a  check for `ErrorFEof` in the `ErrorState` (and add another 
bug type). This is not a fatal error.

It may be more difficult to make `fread` return the feof error again if it 
starts already with feof state. (Because the state with FEof should be split 
from the generic error state that can contain other possible errors.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78374



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

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

I gave a lot of thought to the high level logic, but I need some time to really 
internalize whats happening here.

In D78374#1993858 , @balazske wrote:

> Finally I had to make the decision to remove the `ErrorKindTy` enum and use 
> boolean flags instead for every possible error (including no error). This is 
> needed because we do not know always what error is possible if it is 
> "unknown". It could be determined from the last operation but now not any 
> more. The documentation for `fread` says that if 0 is passed as the size or 
> count argument the function returns zero and does not change the state. So 
> the error state before `fread` remains active. The new design is to have bool 
> values for every error kind (**feof** and **ferror** and a no-error state) so 
> multiple can be active in a state to indicate that more than one error state 
> is possible. If no-error or **feof** is possible these flags are turned on. 
> If we need to know in such a state if the stream is in **EOF** a state split 
> is needed to handle both cases (one with **EOF** and one with no-error). This 
> split must be done in the pre-call handler, for example if we want a warning 
> that the operation is not safe to use in **EOF** state. (But in this case 
> really no split is needed, only clear the EOF flag and make a warning.)


I guess one solution would be to have a history of last operations on the 
stream, but overall, it makes sense to have a make a shift to calculate the 
possible errors as we're evaluating the functions. Great idea!

> We can have another approach if we do not set return values of the functions 
> at all, instead save the symbol of the function and determine the returned 
> value later from the constraints actually applied on it. This may save state 
> splits, but only until a condition is encountered that checks for the 
> function's return value.

Or any stream modifying operation. If we don't have branches for the return 
value, that is almost a bug in itself. Also, digging the return value out of a 
branch condition may be difficult (see `ReturnValueChecker`). Lets stick with 
the state splits at the function call for now.




Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:222
 {StreamState::FEof, StreamState::FError, StreamState::NoError}}},
-  {{"ftell", 1}, {::preDefault, nullptr, 0, {}}},
+  // Note: ftell sets errno only.
+  {{"ftell", 1},

balazske wrote:
> Szelethus wrote:
> > Szelethus wrote:
> > > C'99 standard [[ 
> > > http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf | 
> > > §7.19.9.4.3]], about the `ftell function`:
> > > 
> > > > If successful, the `ftell` function returns the current value of the 
> > > > file position indicator for the stream. On failure, the `ftell` 
> > > > function returns `-1L` and stores an implementation-defined positive 
> > > > value in `errno`.
> > > 
> > > So we need an evalCall for this.
> > I mean, this function can only fail if the stream itself is an erroneous or 
> > closed state, right? So we can associate the -1 return value with the 
> > stream being in that, and the other with the stream being opened.
> The `ftell` is part of a later patch that is adding `ftell` (and probably 
> other functions). Only the "PossibleErrors" was updated in this patch because 
> its meaning was changed (it contains value even if only one error kind is 
> possible, before it was used only if at least two errors are possible).
> It is not sure how the file operations work, maybe the `ftell` needs access 
> to some data that is not available at the moment (and can fail even if the 
> stream was not OK). If the stream is already in failed state the question is: 
> Do ftell fail (then the we can make it return -1) or it does not fail but 
> gives an unusable value (then generate a checker warning and stop the 
> analysis).
In any case, this should be removed from the patch, because adding it seems to 
be an orthogonal change to this one.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:37
+/// Note: A stream has either FEOF or FERROR set but not both at the same time.
+struct StreamErrorState {
+  /// There is an execution path with none of the error flags set.

We're not describing the error state of a stream here, but rather //possible// 
error states, so the name should reflect that.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:207-210
+  {{"fread", 4},
+   {::preFread,
+std::bind(::evalFreadFwrite, _1, _2, _3, _4,
+  ErrorFEof | ErrorFError),

What does this mean? I guess not the possible states after an fread call, but 
rather the possible states after a **failed** fread call, but then...



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:225-228
   

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

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

balazske updated this revision to Diff 259491.
balazske added a comment.

- Stream error is stored in separate boolean flags instead of enum.
- Removed `PossibleErrors`.
- Added "FilePositionIndeterminate".
- Updated fread, fwrite, fseek, clearerr.
- Added tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78374

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
@@ -3,6 +3,7 @@
 #include "Inputs/system-header-simulator.h"
 
 void clang_analyzer_eval(int);
+void clang_analyzer_warnIfReached();
 void StreamTesterChecker_make_feof_stream(FILE *);
 void StreamTesterChecker_make_ferror_stream(FILE *);
 
@@ -53,6 +54,73 @@
   fclose(F);
 }
 
+void error_fread() {
+  FILE *F = tmpfile();
+  if (!F)
+return;
+  int Ret = fread(0, 1, 10, F);
+  if (Ret == 10) {
+clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}}
+  } else {
+clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{TRUE}}
+if (feof(F)) {
+  clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+  fread(0, 1, 10, F); // no warning
+}
+if (ferror(F)) {
+  clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+  fread(0, 1, 10, F); // expected-warning {{Stream might be in indeterminate state}}
+}
+  }
+  fclose(F);
+  Ret = fread(0, 1, 10, F); // expected-warning {{Stream might be already closed}}
+}
+
+void error_fwrite() {
+  FILE *F = tmpfile();
+  if (!F)
+return;
+  int Ret = fwrite(0, 1, 10, F);
+  if (Ret == 10) {
+clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}}
+  } else {
+clang_analyzer_eval(feof(F));   // expected-warning {{FALSE}}
+clang_analyzer_eval(ferror(F)); // expected-warning {{TRUE}}
+fwrite(0, 1, 10, F);// expected-warning {{Stream might be in indeterminate state}}
+  }
+  fclose(F);
+  Ret = fwrite(0, 1, 10, F); // expected-warning {{Stream might be already closed}}
+}
+
+void freadwrite_zerosize(FILE *F) {
+  fwrite(0, 1, 0, F);
+  fwrite(0, 0, 1, F);
+  fread(0, 1, 0, F);
+  fread(0, 0, 1, F);
+}
+
+void error_fread_fwrite_zerosize() {
+  FILE *F = fopen("file", "r");
+  if (!F)
+return;
+
+  freadwrite_zerosize(F);
+  clang_analyzer_eval(feof(F));   // expected-warning {{FALSE}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{FALSE}}
+
+  StreamTesterChecker_make_ferror_stream(F);
+  freadwrite_zerosize(F);
+  clang_analyzer_eval(feof(F));   // expected-warning {{FALSE}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{TRUE}}
+
+  StreamTesterChecker_make_feof_stream(F);
+  freadwrite_zerosize(F);
+  clang_analyzer_eval(feof(F));   // expected-warning {{TRUE}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{FALSE}}
+
+  fclose(F);
+}
+
 void error_fseek() {
   FILE *F = fopen("file", "r");
   if (!F)
@@ -83,3 +151,34 @@
   }
   fclose(F);
 }
+
+void error_fseek_indeterminate() {
+  FILE *F = fopen("file", "r");
+  if (!F)
+return;
+  int rc = fseek(F, 0, SEEK_SET);
+  if (rc) {
+if (feof(F)) {
+  fwrite(0, 1, 10, F); // no warning
+} else if (ferror(F)) {
+  fwrite(0, 1, 10, F); // expected-warning {{Stream might be in indeterminate state}}
+} else {
+  fwrite(0, 1, 10, F); // expected-warning {{Stream might be in indeterminate state}}
+}
+  }
+  fclose(F);
+}
+
+void error_clearerr_indeterminate() {
+  FILE *F = fopen("file", "r");
+  if (!F)
+return;
+  int rc = fseek(F, 0, SEEK_SET);
+  if (rc) {
+if (!feof(F)) {
+  clearerr(F);
+  fwrite(0, 1, 10, F); // expected-warning {{Stream might be in indeterminate state}}
+}
+  }
+  fclose(F);
+}
Index: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -19,15 +19,64 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
+#include 
 
 using namespace clang;
 using namespace ento;
+using namespace std::placeholders;
 
 namespace {
 
 struct FnDescription;
 
+/// State of the stream related error flags.
+/// It indicates possibility of different error states.
+/// In other sense, every set flag means a separate execution path.
+/// At least one of the bool flags must be set to have a valid StreamErrorState.
+/// Note: A stream has either FEOF or FERROR set but not both at the same time.
+struct StreamErrorState {
+  /// There is an execution path with none of the error flags 

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

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

balazske added a comment.

Finally I had to make the decision to remove the `ErrorKindTy` enum and use 
boolean flags instead for every possible error (including no error). This is 
needed because we do not know always what error is possible if it is "unknown". 
It could be determined from the last operation but now not any more. The 
documentation for `fread` says that if 0 is passed as the size or count 
argument the function returns zero and does not change the state. So the error 
state before `fread` remains active. The new design is to have bool values for 
every error kind (**feof** and **ferror** and a no-error state) so multiple can 
be active in a state to indicate that more than one error state is possible. If 
no-error or **feof** is possible these flags are turned on. If we need to know 
in such a state if the stream is in **EOF** a state split is needed to handle 
both cases (one with **EOF** and one with no-error). This split must be done in 
the pre-call handler, for example if we want a warning that the operation is 
not safe to use in **EOF** state. (But in this case really no split is needed, 
only clear the EOF flag and make a warning.)

We can have another approach if we do not set return values of the functions at 
all, instead save the symbol of the function and determine the returned value 
later from the constraints actually applied on it. This may save state splits, 
but only until a condition is encountered that checks for the function's return 
value.

  int rc = fread(buf, size, count, fp);
  if (rc  < count) {
int eof = feof(fp);
  }

In this code if the `fread` has no concrete value set, the `if` would result in 
state split to `rc < count` and `rc >= count`. If the `fread` sets the return 
value the same split is done at `fread` but not at the `if`. If there is no 
such `if` or only later, this saves some state splits. In the first case the 
possible error state of the stream may be hard to determine because it depends 
on the things that were done before the `fread` call too. (We can save the 
symbol of `fread` and `count` and ask at a later point if the condition `fread` 
< `count` is true to get if `fread` has failed. But need to know additionally 
if `count` or `size` was zero and if yes, use a previous error state.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78374



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

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

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

balazske marked 2 inline comments as done.
balazske added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:222
 {StreamState::FEof, StreamState::FError, StreamState::NoError}}},
-  {{"ftell", 1}, {::preDefault, nullptr, 0, {}}},
+  // Note: ftell sets errno only.
+  {{"ftell", 1},

Szelethus wrote:
> Szelethus wrote:
> > C'99 standard [[ http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf 
> > | §7.19.9.4.3]], about the `ftell function`:
> > 
> > > If successful, the `ftell` function returns the current value of the file 
> > > position indicator for the stream. On failure, the `ftell` function 
> > > returns `-1L` and stores an implementation-defined positive value in 
> > > `errno`.
> > 
> > So we need an evalCall for this.
> I mean, this function can only fail if the stream itself is an erroneous or 
> closed state, right? So we can associate the -1 return value with the stream 
> being in that, and the other with the stream being opened.
The `ftell` is part of a later patch that is adding `ftell` (and probably other 
functions). Only the "PossibleErrors" was updated in this patch because its 
meaning was changed (it contains value even if only one error kind is possible, 
before it was used only if at least two errors are possible).
It is not sure how the file operations work, maybe the `ftell` needs access to 
some data that is not available at the moment (and can fail even if the stream 
was not OK). If the stream is already in failed state the question is: Do ftell 
fail (then the we can make it return -1) or it does not fail but gives an 
unusable value (then generate a checker warning and stop the analysis).



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:485
+State = bindInt(0, State, C, CE);
+State = State->set(StreamSym, StreamState::getOpened(Desc));
+C.addTransition(State);

Szelethus wrote:
> Isn't the state change redundant? We have a `preCall` to this function and we 
> assert this as well.
We need to clarify what file operations are valid if the stream is in failed 
state or in EOF state. Again the question is if the function will simply fail 
again or it does not fail but does wrong things, or it will work correctly. 
Currently the initial stream error state is not taken into account for `fread` 
and `fwrite`, this needs to be fixed. For example for `fread`: "If an error 
occurs, the resulting value of the file position indicator for the stream is 
indeterminate." So at least a next read or write or `ftell` can not work 
correct after this (or works but with unusable result).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78374



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

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus requested changes to this revision.
Szelethus added inline comments.
This revision now requires changes to proceed.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:152-158
 /// Get the value of the stream argument out of the passed call event.
 /// The call should contain a function that is described by Desc.
 SVal getStreamArg(const FnDescription *Desc, const CallEvent ) {
   assert(Desc && Desc->StreamArgNo != ArgNone &&
  "Try to get a non-existing stream argument.");
   return Call.getArgSVal(Desc->StreamArgNo);
 }

If we add methods to `FnDescription`, we might as well add this as well, but I 
don't insist.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:222
 {StreamState::FEof, StreamState::FError, StreamState::NoError}}},
-  {{"ftell", 1}, {::preDefault, nullptr, 0, {}}},
+  // Note: ftell sets errno only.
+  {{"ftell", 1},

C'99 standard [[ http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf | 
§7.19.9.4.3]], about the `ftell function`:

> If successful, the `ftell` function returns the current value of the file 
> position indicator for the stream. On failure, the `ftell` function returns 
> `-1L` and stores an implementation-defined positive value in `errno`.

So we need an evalCall for this.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:485
+State = bindInt(0, State, C, CE);
+State = State->set(StreamSym, StreamState::getOpened(Desc));
+C.addTransition(State);

Isn't the state change redundant? We have a `preCall` to this function and we 
assert this as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78374



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

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:222
 {StreamState::FEof, StreamState::FError, StreamState::NoError}}},
-  {{"ftell", 1}, {::preDefault, nullptr, 0, {}}},
+  // Note: ftell sets errno only.
+  {{"ftell", 1},

Szelethus wrote:
> C'99 standard [[ http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf | 
> §7.19.9.4.3]], about the `ftell function`:
> 
> > If successful, the `ftell` function returns the current value of the file 
> > position indicator for the stream. On failure, the `ftell` function returns 
> > `-1L` and stores an implementation-defined positive value in `errno`.
> 
> So we need an evalCall for this.
I mean, this function can only fail if the stream itself is an erroneous or 
closed state, right? So we can associate the -1 return value with the stream 
being in that, and the other with the stream being opened.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78374



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

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

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

balazske created this revision.
Herald added subscribers: cfe-commits, ASDenysPetrov, martong, Charusso, 
gamesh411, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, szepet, 
baloghadamsoftware, xazax.hun.
Herald added a reviewer: Szelethus.
Herald added a project: clang.

Functions fread and fwrite are now modeled with the possible
return values and stream error flags on error.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D78374

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
@@ -3,6 +3,7 @@
 #include "Inputs/system-header-simulator.h"
 
 void clang_analyzer_eval(int);
+void clang_analyzer_warnIfReached();
 void StreamTesterChecker_make_feof_stream(FILE *);
 void StreamTesterChecker_make_ferror_stream(FILE *);
 
@@ -53,6 +54,39 @@
   fclose(F);
 }
 
+void error_fread() {
+  FILE *F = tmpfile();
+  if (!F)
+return;
+  int Ret = fread(0, 1, 10, F);
+  if (Ret == 10) {
+clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}}
+  } else {
+clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{TRUE}}
+if (feof(F))
+  clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+if (ferror(F))
+  clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+  }
+  fclose(F);
+  Ret = fread(0, 1, 10, F); // expected-warning {{Stream might be already closed}}
+}
+
+void error_fwrite() {
+  FILE *F = tmpfile();
+  if (!F)
+return;
+  int Ret = fwrite(0, 1, 10, F);
+  if (Ret == 10) {
+clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}}
+  } else {
+clang_analyzer_eval(feof(F));   // expected-warning {{FALSE}}
+clang_analyzer_eval(ferror(F)); // expected-warning {{TRUE}}
+  }
+  fclose(F);
+  Ret = fwrite(0, 1, 10, F); // expected-warning {{Stream might be already closed}}
+}
+
 void error_fseek() {
   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
@@ -122,11 +122,16 @@
   FnCheck PreFn;
   FnCheck EvalFn;
   ArgNoTy StreamArgNo;
-  // What errors are possible after this operation.
-  // Used only if this operation resulted in Unknown state
-  // (otherwise there is a known single error).
-  // Must contain 2 or 3 elements, or zero.
+  // What errors are possible after the operation has failed.
   llvm::SmallVector PossibleErrors = {};
+
+  StreamState::ErrorKindTy getErrorKindAfterFailedOperation() const {
+assert(!PossibleErrors.empty() &&
+   "Function must be called on operation that can fail.");
+if (PossibleErrors.size() == 1)
+  return PossibleErrors.front();
+return StreamState::Unknown;
+  }
 };
 
 Optional
@@ -199,17 +204,31 @@
   {{"tmpfile"}, {nullptr, ::evalFopen, ArgNone, {}}},
   {{"fclose", 1},
{::preDefault, ::evalFclose, 0, {}}},
-  {{"fread", 4}, {::preDefault, nullptr, 3, {}}},
-  {{"fwrite", 4}, {::preDefault, nullptr, 3, {}}},
+  {{"fread", 4},
+   {::preDefault,
+::evalFreadFwrite,
+3,
+{StreamState::FError, StreamState::FEof}}},
+  {{"fwrite", 4},
+   {::preDefault,
+::evalFreadFwrite,
+3,
+{StreamState::FError}}},
   {{"fseek", 3},
{::preFseek,
 ::evalFseek,
 0,
 {StreamState::FEof, StreamState::FError, StreamState::NoError}}},
-  {{"ftell", 1}, {::preDefault, nullptr, 0, {}}},
+  // Note: ftell sets errno only.
+  {{"ftell", 1},
+   {::preDefault, nullptr, 0, {StreamState::NoError}}},
   {{"rewind", 1}, {::preDefault, nullptr, 0, {}}},
-  {{"fgetpos", 2}, {::preDefault, nullptr, 0, {}}},
-  {{"fsetpos", 2}, {::preDefault, nullptr, 0, {}}},
+  // Note: fgetpos sets errno only.
+  {{"fgetpos", 2},
+   {::preDefault, nullptr, 0, {StreamState::NoError}}},
+  // Note: fsetpos sets errno only.
+  {{"fsetpos", 2},
+   {::preDefault, nullptr, 0, {StreamState::NoError}}},
   {{"clearerr", 1},
{::preDefault, ::evalClearerr, 0, {}}},
   // Note: feof can result in Unknown if at the call there is a
@@ -249,17 +268,20 @@
   void evalFclose(const FnDescription *Desc, const CallEvent ,
   CheckerContext ) const;
 
+  void evalFreadFwrite(const FnDescription *Desc, const CallEvent ,
+   CheckerContext ) const;
+
   void preFseek(const FnDescription *Desc, const CallEvent ,
 CheckerContext ) const;
   void evalFseek(const FnDescription *Desc, const CallEvent ,
  CheckerContext ) const;
 
-  void preDefault(const FnDescription *Desc,