[PATCH] D106644: [clang][analyzer] Add standard streams to alpha.unix.Stream checker.

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:504
+
+  OrigStdin = findStdStream("stdin", C);
+  OrigStdout = findStdStream("stdout", C);

steakhal wrote:
> balazske wrote:
> > steakhal wrote:
> > > balazske wrote:
> > > > steakhal wrote:
> > > > > martong wrote:
> > > > > > We should be careful, to cache the results (either here, or deeper 
> > > > > > in the call stack).
> > > > > > I mean, we certainly don't want to do a lookup of "stdin" every 
> > > > > > time a function is evaluated. We should do this lazily, only once.
> > > > > I agree. We should do this only for the first top-level function, 
> > > > > instead of doing this for every top-level function.
> > > > I am not sure if the `SymbolRef` values are safe to store between 
> > > > top-level function analyses. The `FILE` type and std stream 
> > > > declarations are safe to cache, but the SymbolRef values of these 
> > > > variables probably not.
> > > I think it should be safe. But place there an assert and run the test 
> > > suite. If it won't trigger, that means that we have high confidentiality 
> > > that this is safe. I know it's not 100%, but pretty close.
> > I tried to check if these `SymbolRef`'s are the same at 
> > `checkBeginFunction` after initialized once. The assertion for equality 
> > failed sometimes, or other crash happened when `dump` was called on the 
> > value. So it looks not safe to cache these. And should we add assumptions 
> > about that these `StdinSym`, `StdoutSym`, `StderrSym` are not equal to each 
> > other?
> Good to know. I don't think it's necessary to add assertions.
Okay, so we should not cache the SymbolRefs then. But we must cache the 
VarDecls. I mean, we should avoid calling
```
StdinDecl = findStdStreamDecl("stdin", C);
```
more than once for each TU.

I think you could use and Optional for StdinDecl (and the others) to achieve 
this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106644

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

[PATCH] D106644: [clang][analyzer] Add standard streams to alpha.unix.Stream checker.

[Balázs Benics via Phabricator via cfe-commits]

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

It's fine on my part.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106644

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

[PATCH] D106644: [clang][analyzer] Add standard streams to alpha.unix.Stream checker.

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

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

Simplified a lambda usage.
Moved some functions, changed documentation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106644

Files:
  clang/lib/StaticAnalyzer/Checkers/ASTLookup.cpp
  clang/lib/StaticAnalyzer/Checkers/ASTLookup.h
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
  clang/test/Analysis/stream.c

Index: clang/test/Analysis/stream.c
===
--- clang/test/Analysis/stream.c
+++ clang/test/Analysis/stream.c
@@ -1,7 +1,9 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.unix.Stream -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.unix.Stream,debug.ExprInspection -verify %s
 
 #include "Inputs/system-header-simulator.h"
 
+void clang_analyzer_eval(int);
+
 void check_fread() {
   FILE *fp = tmpfile();
   fread(0, 0, 0, fp); // expected-warning {{Stream pointer might be NULL}}
@@ -267,3 +269,23 @@
 } // expected-warning {{Opened stream never closed. Potential resource leak}}
 // FIXME: This warning should be placed at the `return` above.
 // See https://reviews.llvm.org/D83120 about details.
+
+void check_fopen_is_not_std() {
+  FILE *F = fopen("file", "r");
+  if (!F)
+return;
+  clang_analyzer_eval(F != stdin);  // expected-warning{{TRUE}}
+  clang_analyzer_eval(F != stdout); // expected-warning{{TRUE}}
+  clang_analyzer_eval(F != stderr); // expected-warning{{TRUE}}
+  fclose(F);
+}
+
+void check_tmpfile_is_not_std() {
+  FILE *F = tmpfile();
+  if (!F)
+return;
+  clang_analyzer_eval(F != stdin);  // expected-warning{{TRUE}}
+  clang_analyzer_eval(F != stdout); // expected-warning{{TRUE}}
+  clang_analyzer_eval(F != stderr); // expected-warning{{TRUE}}
+  fclose(F);
+}
Index: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -10,6 +10,8 @@
 //
 //===--===//
 
+#include "ASTLookup.h"
+#include "clang/AST/ParentMapContext.h"
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
@@ -157,6 +159,11 @@
 // StreamChecker class and utility functions.
 //===--===//
 
+// This map holds the state of a stream.
+// The stream is identified with a SymbolRef that is created when a stream
+// opening function is modeled by the checker.
+REGISTER_MAP_WITH_PROGRAMSTATE(StreamMap, SymbolRef, StreamState)
+
 namespace {
 
 class StreamChecker;
@@ -172,6 +179,23 @@
   ArgNoTy StreamArgNo;
 };
 
+/// Find symbolic value of a global "standard stream"
+/// (stdin, stdout, stderr) variable.
+/// The StdDecl value is allowed to be nullptr.
+SymbolRef getStdStreamSym(const VarDecl *StdDecl, CheckerContext ) {
+  if (!StdDecl)
+return nullptr;
+
+  const LocationContext *LCtx = C.getLocationContext();
+
+  const VarRegion *RegionOfStdStreamVar =
+  C.getState()->getRegion(StdDecl, LCtx);
+  if (!RegionOfStdStreamVar)
+return nullptr;
+  SVal StdVal = C.getState()->getSVal(RegionOfStdStreamVar, StdDecl->getType());
+  return StdVal.getAsSymbol();
+}
+
 /// 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 ) {
@@ -206,8 +230,9 @@
   return State;
 }
 
-class StreamChecker : public Checker {
+class StreamChecker
+: public Checker {
   BugType BT_FileNull{this, "NULL stream pointer", "Stream handling error"};
   BugType BT_UseAfterClose{this, "Closed stream", "Stream handling error"};
   BugType BT_UseAfterOpenFailed{this, "Invalid stream",
@@ -221,6 +246,7 @@
   /*SuppressOnSink =*/true};
 
 public:
+  void checkBeginFunction(CheckerContext ) const;
   void checkPreCall(const CallEvent , CheckerContext ) const;
   bool evalCall(const CallEvent , CheckerContext ) const;
   void checkDeadSymbols(SymbolReaper , CheckerContext ) const;
@@ -285,6 +311,17 @@
 0}},
   };
 
+  mutable bool StdStreamsInitialized = false;
+  mutable Optional FILEType;
+  // These can be nullptr if not found.
+  mutable const VarDecl *StdinDecl;
+  mutable const VarDecl *StdoutDecl;
+  mutable const VarDecl *StderrDecl;
+  // These can be nullptr if not found.
+  mutable SymbolRef StdinSym;
+  mutable SymbolRef StdoutSym;
+  mutable SymbolRef StderrSym;
+
   void evalFopen(const FnDescription *Desc, const CallEvent ,
  

[PATCH] D106644: [clang][analyzer] Add standard streams to alpha.unix.Stream checker.

[Balázs Benics via Phabricator via cfe-commits]

steakhal added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:504
+
+  OrigStdin = findStdStream("stdin", C);
+  OrigStdout = findStdStream("stdout", C);

balazske wrote:
> steakhal wrote:
> > balazske wrote:
> > > steakhal wrote:
> > > > martong wrote:
> > > > > We should be careful, to cache the results (either here, or deeper in 
> > > > > the call stack).
> > > > > I mean, we certainly don't want to do a lookup of "stdin" every time 
> > > > > a function is evaluated. We should do this lazily, only once.
> > > > I agree. We should do this only for the first top-level function, 
> > > > instead of doing this for every top-level function.
> > > I am not sure if the `SymbolRef` values are safe to store between 
> > > top-level function analyses. The `FILE` type and std stream declarations 
> > > are safe to cache, but the SymbolRef values of these variables probably 
> > > not.
> > I think it should be safe. But place there an assert and run the test 
> > suite. If it won't trigger, that means that we have high confidentiality 
> > that this is safe. I know it's not 100%, but pretty close.
> I tried to check if these `SymbolRef`'s are the same at `checkBeginFunction` 
> after initialized once. The assertion for equality failed sometimes, or other 
> crash happened when `dump` was called on the value. So it looks not safe to 
> cache these. And should we add assumptions about that these `StdinSym`, 
> `StdoutSym`, `StderrSym` are not equal to each other?
Good to know. I don't think it's necessary to add assertions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106644

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

[PATCH] D106644: [clang][analyzer] Add standard streams to alpha.unix.Stream checker.

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

balazske marked an inline comment as done.
balazske added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:504
+
+  OrigStdin = findStdStream("stdin", C);
+  OrigStdout = findStdStream("stdout", C);

steakhal wrote:
> balazske wrote:
> > steakhal wrote:
> > > martong wrote:
> > > > We should be careful, to cache the results (either here, or deeper in 
> > > > the call stack).
> > > > I mean, we certainly don't want to do a lookup of "stdin" every time a 
> > > > function is evaluated. We should do this lazily, only once.
> > > I agree. We should do this only for the first top-level function, instead 
> > > of doing this for every top-level function.
> > I am not sure if the `SymbolRef` values are safe to store between top-level 
> > function analyses. The `FILE` type and std stream declarations are safe to 
> > cache, but the SymbolRef values of these variables probably not.
> I think it should be safe. But place there an assert and run the test suite. 
> If it won't trigger, that means that we have high confidentiality that this 
> is safe. I know it's not 100%, but pretty close.
I tried to check if these `SymbolRef`'s are the same at `checkBeginFunction` 
after initialized once. The assertion for equality failed sometimes, or other 
crash happened when `dump` was called on the value. So it looks not safe to 
cache these. And should we add assumptions about that these `StdinSym`, 
`StdoutSym`, `StderrSym` are not equal to each other?



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:555
+C.getSValBuilder().makeSymbolVal(StdStream),
+C.getASTContext().getLogicalOperationType());
+Optional NotEqDef =

steakhal wrote:
> `SValBuilder::getConditionType()`, oh they are the same under the hood. We 
> should still probably prefer this one instead.
> It might worth hoisting `C.getSValBuilder()` to a local variable though.
> The symbol for `StdStream` also deserves a separate variable IMO.
This lambda is possible to improve.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106644

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

[PATCH] D106644: [clang][analyzer] Add standard streams to alpha.unix.Stream checker.

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

Let's see if we can cache the stream symbols across top-level analyses.




Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:476
+
+  for (Decl *D : LookupRes) {
+D = D->getCanonicalDecl();





Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:480
+  continue;
+if (auto *VD = dyn_cast(D)) {
+  if (FILEType && !ACtx.hasSameType(*FILEType, VD->getType()))





Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:521
+
+  // These can be nullptr if not found.
+  StdinSym = getStdStreamSym(StdinDecl, C);

This should be probably at the definition of `StdinSym` and `getStdStreamSym()`.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:227-236
+  for (Decl *D : LookupRes) {
+D = D->getCanonicalDecl();
+if (!C.getSourceManager().isInSystemHeader(D->getLocation()))
+  continue;
+if (auto *VD = dyn_cast(D)) {
+  if (FILEType && !ACtx.hasSameType(*FILEType, VD->getType()))
+continue;

balazske wrote:
> steakhal wrote:
> > It will early return and uses one fewer `continue`.
> The intent was that if no `FILEType` is found we find just the first 
> `VarDecl` and do not check the type. This recommended change returns always 
> null if no `FILEType` is found.
Hm, it was not immediately clear to me.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:504
+
+  OrigStdin = findStdStream("stdin", C);
+  OrigStdout = findStdStream("stdout", C);

balazske wrote:
> steakhal wrote:
> > martong wrote:
> > > We should be careful, to cache the results (either here, or deeper in the 
> > > call stack).
> > > I mean, we certainly don't want to do a lookup of "stdin" every time a 
> > > function is evaluated. We should do this lazily, only once.
> > I agree. We should do this only for the first top-level function, instead 
> > of doing this for every top-level function.
> I am not sure if the `SymbolRef` values are safe to store between top-level 
> function analyses. The `FILE` type and std stream declarations are safe to 
> cache, but the SymbolRef values of these variables probably not.
I think it should be safe. But place there an assert and run the test suite. If 
it won't trigger, that means that we have high confidentiality that this is 
safe. I know it's not 100%, but pretty close.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106644

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

[PATCH] D106644: [clang][analyzer] Add standard streams to alpha.unix.Stream checker.

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

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

Init std declarations and FILE type only once.
Make a lambda not operate on captured data.
Simplify test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106644

Files:
  clang/lib/StaticAnalyzer/Checkers/ASTLookup.cpp
  clang/lib/StaticAnalyzer/Checkers/ASTLookup.h
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
  clang/test/Analysis/stream.c

Index: clang/test/Analysis/stream.c
===
--- clang/test/Analysis/stream.c
+++ clang/test/Analysis/stream.c
@@ -1,7 +1,9 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.unix.Stream -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.unix.Stream,debug.ExprInspection -verify %s
 
 #include "Inputs/system-header-simulator.h"
 
+void clang_analyzer_eval(int);
+
 void check_fread() {
   FILE *fp = tmpfile();
   fread(0, 0, 0, fp); // expected-warning {{Stream pointer might be NULL}}
@@ -267,3 +269,23 @@
 } // expected-warning {{Opened stream never closed. Potential resource leak}}
 // FIXME: This warning should be placed at the `return` above.
 // See https://reviews.llvm.org/D83120 about details.
+
+void check_fopen_is_not_std() {
+  FILE *F = fopen("file", "r");
+  if (!F)
+return;
+  clang_analyzer_eval(F != stdin);  // expected-warning{{TRUE}}
+  clang_analyzer_eval(F != stdout); // expected-warning{{TRUE}}
+  clang_analyzer_eval(F != stderr); // expected-warning{{TRUE}}
+  fclose(F);
+}
+
+void check_tmpfile_is_not_std() {
+  FILE *F = tmpfile();
+  if (!F)
+return;
+  clang_analyzer_eval(F != stdin);  // expected-warning{{TRUE}}
+  clang_analyzer_eval(F != stdout); // expected-warning{{TRUE}}
+  clang_analyzer_eval(F != stderr); // expected-warning{{TRUE}}
+  fclose(F);
+}
Index: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -10,6 +10,8 @@
 //
 //===--===//
 
+#include "ASTLookup.h"
+#include "clang/AST/ParentMapContext.h"
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
@@ -157,6 +159,11 @@
 // StreamChecker class and utility functions.
 //===--===//
 
+// This map holds the state of a stream.
+// The stream is identified with a SymbolRef that is created when a stream
+// opening function is modeled by the checker.
+REGISTER_MAP_WITH_PROGRAMSTATE(StreamMap, SymbolRef, StreamState)
+
 namespace {
 
 class StreamChecker;
@@ -206,8 +213,9 @@
   return State;
 }
 
-class StreamChecker : public Checker {
+class StreamChecker
+: public Checker {
   BugType BT_FileNull{this, "NULL stream pointer", "Stream handling error"};
   BugType BT_UseAfterClose{this, "Closed stream", "Stream handling error"};
   BugType BT_UseAfterOpenFailed{this, "Invalid stream",
@@ -221,6 +229,7 @@
   /*SuppressOnSink =*/true};
 
 public:
+  void checkBeginFunction(CheckerContext ) const;
   void checkPreCall(const CallEvent , CheckerContext ) const;
   bool evalCall(const CallEvent , CheckerContext ) const;
   void checkDeadSymbols(SymbolReaper , CheckerContext ) const;
@@ -285,6 +294,15 @@
 0}},
   };
 
+  mutable bool StdStreamsInitialized = false;
+  mutable Optional FILEType;
+  mutable const VarDecl *StdinDecl;
+  mutable const VarDecl *StdoutDecl;
+  mutable const VarDecl *StderrDecl;
+  mutable SymbolRef StdinSym;
+  mutable SymbolRef StdoutSym;
+  mutable SymbolRef StderrSym;
+
   void evalFopen(const FnDescription *Desc, const CallEvent ,
  CheckerContext ) const;
 
@@ -410,20 +428,23 @@
   static const ExplodedNode *getAcquisitionSite(const ExplodedNode *N,
 SymbolRef StreamSym,
 CheckerContext );
+
+  const VarDecl *findStdStreamDecl(StringRef StdName, CheckerContext ) const;
+
+  SymbolRef getStdStreamSym(const VarDecl *StdDecl, CheckerContext ) const;
 };
 
 } // end anonymous namespace
 
-// This map holds the state of a stream.
-// The stream is identified with a SymbolRef that is created when a stream
-// opening function is modeled by the checker.
-REGISTER_MAP_WITH_PROGRAMSTATE(StreamMap, SymbolRef, StreamState)
-
 inline void assertStreamStateOpened(const StreamState *SS) {
   assert(SS->isOpened() &&
  "Previous create of error node for non-opened stream failed?");
 }
 

[PATCH] D106644: [clang][analyzer] Add standard streams to alpha.unix.Stream checker.

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

balazske added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:227-236
+  for (Decl *D : LookupRes) {
+D = D->getCanonicalDecl();
+if (!C.getSourceManager().isInSystemHeader(D->getLocation()))
+  continue;
+if (auto *VD = dyn_cast(D)) {
+  if (FILEType && !ACtx.hasSameType(*FILEType, VD->getType()))
+continue;

steakhal wrote:
> It will early return and uses one fewer `continue`.
The intent was that if no `FILEType` is found we find just the first `VarDecl` 
and do not check the type. This recommended change returns always null if no 
`FILEType` is found.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:504
+
+  OrigStdin = findStdStream("stdin", C);
+  OrigStdout = findStdStream("stdout", C);

steakhal wrote:
> martong wrote:
> > We should be careful, to cache the results (either here, or deeper in the 
> > call stack).
> > I mean, we certainly don't want to do a lookup of "stdin" every time a 
> > function is evaluated. We should do this lazily, only once.
> I agree. We should do this only for the first top-level function, instead of 
> doing this for every top-level function.
I am not sure if the `SymbolRef` values are safe to store between top-level 
function analyses. The `FILE` type and std stream declarations are safe to 
cache, but the SymbolRef values of these variables probably not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106644

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

[PATCH] D106644: [clang][analyzer] Add standard streams to alpha.unix.Stream checker.

[Balázs Benics via Phabricator via cfe-commits]

steakhal added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/ASTLookup.cpp:26
+  // and we have a TypedefDecl with the name 'FILE'.
+  for (Decl *D : LookupRes)
+if (auto *TD = dyn_cast(D))

`Decl *D` -> `const Decl *D`. Same for the other loop.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:225
+
+  Optional FILEType = GetPointer(LookupType("FILE"));
+

You calculate this 3 times now. I mean it's not a big deal, but we could save 
this to do it only once.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:227-236
+  for (Decl *D : LookupRes) {
+D = D->getCanonicalDecl();
+if (!C.getSourceManager().isInSystemHeader(D->getLocation()))
+  continue;
+if (auto *VD = dyn_cast(D)) {
+  if (FILEType && !ACtx.hasSameType(*FILEType, VD->getType()))
+continue;

It will early return and uses one fewer `continue`.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:504
+
+  OrigStdin = findStdStream("stdin", C);
+  OrigStdout = findStdStream("stdout", C);

martong wrote:
> We should be careful, to cache the results (either here, or deeper in the 
> call stack).
> I mean, we certainly don't want to do a lookup of "stdin" every time a 
> function is evaluated. We should do this lazily, only once.
I agree. We should do this only for the first top-level function, instead of 
doing this for every top-level function.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:554
 
+  auto AssumeRetValNotEq = [, , RetVal](SymbolRef StdStream) {
+SVal NotEqS = C.getSValBuilder().evalBinOp(

It might be a personal preference but I think lambdas should be pure in the 
sense that it takes something and produces something else.
Regardless of your choice, the trailing return type would highlight it.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:559-562
+Optional NotEqDef =
+NotEqS.getAs();
+if (!NotEqDef)
+  return;

I think you should be fine with `castAs()`. I'm not expecting this to fail.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:555
+C.getSValBuilder().makeSymbolVal(StdStream),
+C.getASTContext().getLogicalOperationType());
+Optional NotEqDef =

`SValBuilder::getConditionType()`, oh they are the same under the hood. We 
should still probably prefer this one instead.
It might worth hoisting `C.getSValBuilder()` to a local variable though.
The symbol for `StdStream` also deserves a separate variable IMO.



Comment at: clang/test/Analysis/stream.c:276-277
+return;
+  if (F != stdin && F != stdout && F != stderr)
+fclose(F); // no warning: the opened file can not be equal to std streams
+}

How about checking this way instead?
```lang=C++
clang_analyzer_eval(F != stdin);  // TRUE
clang_analyzer_eval(F != stdout); // TRUE
clang_analyzer_eval(F != stderr); // TRUE
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106644

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

[PATCH] D106644: [clang][analyzer] Add standard streams to alpha.unix.Stream checker.

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D106644#2913676 , @balazske wrote:

> Split some type lookup functions from StdLibraryFunctionsChecker into 
> separate files.

Thank you, this is awesome!




Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:504
+
+  OrigStdin = findStdStream("stdin", C);
+  OrigStdout = findStdStream("stdout", C);

We should be careful, to cache the results (either here, or deeper in the call 
stack).
I mean, we certainly don't want to do a lookup of "stdin" every time a function 
is evaluated. We should do this lazily, only once.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106644

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

[PATCH] D106644: [clang][analyzer] Add standard streams to alpha.unix.Stream checker.

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

balazske updated this revision to Diff 362775.
balazske added a comment.
Herald added a subscriber: mgorny.

Split some type lookup functions from StdLibraryFunctionsChecker into separate 
files.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106644

Files:
  clang/lib/StaticAnalyzer/Checkers/ASTLookup.cpp
  clang/lib/StaticAnalyzer/Checkers/ASTLookup.h
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
  clang/test/Analysis/stream.c

Index: clang/test/Analysis/stream.c
===
--- clang/test/Analysis/stream.c
+++ clang/test/Analysis/stream.c
@@ -267,3 +267,12 @@
 } // expected-warning {{Opened stream never closed. Potential resource leak}}
 // FIXME: This warning should be placed at the `return` above.
 // See https://reviews.llvm.org/D83120 about details.
+
+void check_fopen_is_not_std(FILE *F, int Std) {
+  if (!Std)
+F = fopen("file", "r");
+  if (!F)
+return;
+  if (F != stdin && F != stdout && F != stderr)
+fclose(F); // no warning: the opened file can not be equal to std streams
+}
Index: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -10,6 +10,8 @@
 //
 //===--===//
 
+#include "ASTLookup.h"
+#include "clang/AST/ParentMapContext.h"
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
@@ -157,6 +159,11 @@
 // StreamChecker class and utility functions.
 //===--===//
 
+// This map holds the state of a stream.
+// The stream is identified with a SymbolRef that is created when a stream
+// opening function is modeled by the checker.
+REGISTER_MAP_WITH_PROGRAMSTATE(StreamMap, SymbolRef, StreamState)
+
 namespace {
 
 class StreamChecker;
@@ -206,8 +213,49 @@
   return State;
 }
 
-class StreamChecker : public Checker {
+const VarDecl *findStdStreamDecl(StringRef StdName, CheckerContext ) {
+  ASTContext  = C.getASTContext();
+
+  ast_lookup::LookupType LookupType(ACtx);
+  ast_lookup::GetPointerTy GetPointer(ACtx);
+
+  IdentifierInfo  = ACtx.Idents.get(StdName);
+  auto LookupRes = ACtx.getTranslationUnitDecl()->lookup();
+
+  Optional FILEType = GetPointer(LookupType("FILE"));
+
+  for (Decl *D : LookupRes) {
+D = D->getCanonicalDecl();
+if (!C.getSourceManager().isInSystemHeader(D->getLocation()))
+  continue;
+if (auto *VD = dyn_cast(D)) {
+  if (FILEType && !ACtx.hasSameType(*FILEType, VD->getType()))
+continue;
+  return VD;
+}
+  }
+
+  return nullptr;
+}
+
+SymbolRef findStdStream(StringRef StdName, CheckerContext ) {
+  const LocationContext *LCtx = C.getLocationContext();
+
+  const VarDecl *StdDecl = findStdStreamDecl(StdName, C);
+  if (!StdDecl)
+return nullptr;
+
+  const VarRegion *RegionOfStdStreamVar =
+  C.getState()->getRegion(StdDecl, LCtx);
+  if (!RegionOfStdStreamVar)
+return nullptr;
+  SVal StdVal = C.getState()->getSVal(RegionOfStdStreamVar, StdDecl->getType());
+  return StdVal.getAsSymbol();
+}
+
+class StreamChecker
+: public Checker {
   BugType BT_FileNull{this, "NULL stream pointer", "Stream handling error"};
   BugType BT_UseAfterClose{this, "Closed stream", "Stream handling error"};
   BugType BT_UseAfterOpenFailed{this, "Invalid stream",
@@ -221,6 +269,7 @@
   /*SuppressOnSink =*/true};
 
 public:
+  void checkBeginFunction(CheckerContext ) const;
   void checkPreCall(const CallEvent , CheckerContext ) const;
   bool evalCall(const CallEvent , CheckerContext ) const;
   void checkDeadSymbols(SymbolReaper , CheckerContext ) const;
@@ -285,6 +334,10 @@
 0}},
   };
 
+  mutable SymbolRef OrigStdin;
+  mutable SymbolRef OrigStdout;
+  mutable SymbolRef OrigStderr;
+
   void evalFopen(const FnDescription *Desc, const CallEvent ,
  CheckerContext ) const;
 
@@ -414,16 +467,15 @@
 
 } // end anonymous namespace
 
-// This map holds the state of a stream.
-// The stream is identified with a SymbolRef that is created when a stream
-// opening function is modeled by the checker.
-REGISTER_MAP_WITH_PROGRAMSTATE(StreamMap, SymbolRef, StreamState)
-
 inline void assertStreamStateOpened(const StreamState *SS) {
   assert(SS->isOpened() &&
  "Previous create of error node for non-opened stream failed?");
 }
 
+//===--===//
+// Methods of StreamChecker.

[PATCH] D106644: [clang][analyzer] Add standard streams to alpha.unix.Stream checker.

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

Szelethus added a reviewer: vsavchenko.
Szelethus added a comment.

I like the idea, though I wonder whether `evalAssume` would be a better 
callback for this. That way, you'd only need to add an assumption when you 
reach a condition where one of the operands is standard.

Though it may be more trouble than its worth. I don't have strong feelings on 
this.




Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:238
+
+SymbolRef findStdStream(StringRef StdName, CheckerContext ) {
+  const LocationContext *LCtx = C.getLocationContext();

How about `getStdStreamSymbol`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106644

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

[PATCH] D106644: [clang][analyzer] Add standard streams to alpha.unix.Stream checker.

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:215-236
+const VarDecl *findStdStreamDecl(StringRef StdName, CheckerContext ) {
+  ASTContext  = C.getASTContext();
+
+  IdentifierInfo  = ACtx.Idents.get(StdName);
+  auto LookupRes = ACtx.getTranslationUnitDecl()->lookup();
+  QualType FILEType = ACtx.getFILEType();
+  if (!FILEType.isNull())

martong wrote:
> Would it be possible to reuse (i.e put in a common place) the 
> StdLibraryFunctionChecker's `lookupTy`? That also handles the cases when 
> FILEType is null or when we have a `typedef struct FILE FILE;`.
Could you please consider the above question?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106644

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

[PATCH] D106644: [clang][analyzer] Add standard streams to alpha.unix.Stream checker.

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:215-236
+const VarDecl *findStdStreamDecl(StringRef StdName, CheckerContext ) {
+  ASTContext  = C.getASTContext();
+
+  IdentifierInfo  = ACtx.Idents.get(StdName);
+  auto LookupRes = ACtx.getTranslationUnitDecl()->lookup();
+  QualType FILEType = ACtx.getFILEType();
+  if (!FILEType.isNull())

Would it be possible to reuse (i.e put in a common place) the 
StdLibraryFunctionChecker's `lookupTy`? That also handles the cases when 
FILEType is null or when we have a `typedef struct FILE FILE;`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106644

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

[PATCH] D106644: [clang][analyzer] Add standard streams to alpha.unix.Stream checker.

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

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

Recognize initial value of standard streams (stdin, ...)
and assume that a new opened stream is not equal to these.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D106644

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

Index: clang/test/Analysis/stream.c
===
--- clang/test/Analysis/stream.c
+++ clang/test/Analysis/stream.c
@@ -267,3 +267,12 @@
 } // expected-warning {{Opened stream never closed. Potential resource leak}}
 // FIXME: This warning should be placed at the `return` above.
 // See https://reviews.llvm.org/D83120 about details.
+
+void check_fopen_is_not_std(FILE *F, int Std) {
+  if (!Std)
+F = fopen("file", "r");
+  if (!F)
+return;
+  if (F != stdin && F != stdout && F != stderr)
+fclose(F); // no warning: the opened file can not be equal to std streams
+}
Index: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -10,6 +10,7 @@
 //
 //===--===//
 
+#include "clang/AST/ParentMapContext.h"
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
@@ -157,6 +158,11 @@
 // StreamChecker class and utility functions.
 //===--===//
 
+// This map holds the state of a stream.
+// The stream is identified with a SymbolRef that is created when a stream
+// opening function is modeled by the checker.
+REGISTER_MAP_WITH_PROGRAMSTATE(StreamMap, SymbolRef, StreamState)
+
 namespace {
 
 class StreamChecker;
@@ -206,8 +212,47 @@
   return State;
 }
 
-class StreamChecker : public Checker {
+const VarDecl *findStdStreamDecl(StringRef StdName, CheckerContext ) {
+  ASTContext  = C.getASTContext();
+
+  IdentifierInfo  = ACtx.Idents.get(StdName);
+  auto LookupRes = ACtx.getTranslationUnitDecl()->lookup();
+  QualType FILEType = ACtx.getFILEType();
+  if (!FILEType.isNull())
+FILEType = ACtx.getPointerType(FILEType);
+
+  for (Decl *D : LookupRes) {
+D = D->getCanonicalDecl();
+if (!C.getSourceManager().isInSystemHeader(D->getLocation()))
+  continue;
+if (auto *VD = dyn_cast(D)) {
+  if (!FILEType.isNull() && !ACtx.hasSameType(FILEType, VD->getType()))
+continue;
+  return VD;
+}
+  }
+
+  return nullptr;
+}
+
+SymbolRef findStdStream(StringRef StdName, CheckerContext ) {
+  const LocationContext *LCtx = C.getLocationContext();
+
+  const VarDecl *StdDecl = findStdStreamDecl(StdName, C);
+  if (!StdDecl)
+return nullptr;
+
+  const VarRegion *RegionOfStdStreamVar =
+  C.getState()->getRegion(StdDecl, LCtx);
+  if (!RegionOfStdStreamVar)
+return nullptr;
+  SVal StdVal = C.getState()->getSVal(RegionOfStdStreamVar, StdDecl->getType());
+  return StdVal.getAsSymbol();
+}
+
+class StreamChecker
+: public Checker {
   BugType BT_FileNull{this, "NULL stream pointer", "Stream handling error"};
   BugType BT_UseAfterClose{this, "Closed stream", "Stream handling error"};
   BugType BT_UseAfterOpenFailed{this, "Invalid stream",
@@ -221,6 +266,7 @@
   /*SuppressOnSink =*/true};
 
 public:
+  void checkBeginFunction(CheckerContext ) const;
   void checkPreCall(const CallEvent , CheckerContext ) const;
   bool evalCall(const CallEvent , CheckerContext ) const;
   void checkDeadSymbols(SymbolReaper , CheckerContext ) const;
@@ -285,6 +331,10 @@
 0}},
   };
 
+  mutable SymbolRef OrigStdin;
+  mutable SymbolRef OrigStdout;
+  mutable SymbolRef OrigStderr;
+
   void evalFopen(const FnDescription *Desc, const CallEvent ,
  CheckerContext ) const;
 
@@ -414,16 +464,15 @@
 
 } // end anonymous namespace
 
-// This map holds the state of a stream.
-// The stream is identified with a SymbolRef that is created when a stream
-// opening function is modeled by the checker.
-REGISTER_MAP_WITH_PROGRAMSTATE(StreamMap, SymbolRef, StreamState)
-
 inline void assertStreamStateOpened(const StreamState *SS) {
   assert(SS->isOpened() &&
  "Previous create of error node for non-opened stream failed?");
 }
 
+//===--===//
+// Methods of StreamChecker.