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

2024-03-07 Thread Balázs Kéri via cfe-commits 

https://github.com/balazske closed 
https://github.com/llvm/llvm-project/pull/84191
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2024-03-07 Thread Ben Shi via cfe-commits 

https://github.com/benshi001 approved this pull request.

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

https://github.com/llvm/llvm-project/pull/84191
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2024-03-07 Thread Balazs Benics via cfe-commits 

https://github.com/steakhal approved this pull request.

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

https://github.com/llvm/llvm-project/pull/84191
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2024-03-07 Thread via cfe-commits 

https://github.com/NagyDonat edited 
https://github.com/llvm/llvm-project/pull/84191
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2024-03-07 Thread via cfe-commits 

https://github.com/NagyDonat edited 
https://github.com/llvm/llvm-project/pull/84191
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2024-03-07 Thread via cfe-commits 


@@ -880,6 +883,24 @@ void StreamChecker::preReadWrite(const FnDescription *Desc,
   }
 }
 
+void StreamChecker::preWrite(const FnDescription *Desc, const CallEvent ,

NagyDonat wrote:

I agree that `std::bind` is hard to follow, and dedicated handlers dispatching 
to common implementations is a better model.

The "Copy-pasting their implementation while specializing it" could be even 
better if the common parts are lifted out into helper functions; but I strongly 
suggest that we should avoid code duplication.

https://github.com/llvm/llvm-project/pull/84191
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2024-03-07 Thread Balazs Benics via cfe-commits 


@@ -880,6 +883,24 @@ void StreamChecker::preReadWrite(const FnDescription *Desc,
   }
 }
 
+void StreamChecker::preWrite(const FnDescription *Desc, const CallEvent ,

steakhal wrote:

Let me share my experience with `std::bind` and with this checker.
Unfortunately, we also have/had a couple (<10) conflicting patches to this 
checker, and I had to rebase 16 times to clang-18. This is fine, but when I 
this many `std::binds`, and sometimes changes where the railing "bool" flag 
meaning was changed, I really had to be on my toes when resolving syntactic & 
semantic conflicts.
I feel the `std::bind` usage got out of hand here, and I now regret not pushing 
back.

The bottom line is, that I think having dedicated handlers dispatching to 
common implementations, or just copy-pasting their implementation while 
specializing it leads to cleaner code than using `std::bind` and directly 
binding to the generic implementation.

https://github.com/llvm/llvm-project/pull/84191
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2024-03-07 Thread Balázs Kéri via cfe-commits 


@@ -880,6 +883,24 @@ void StreamChecker::preReadWrite(const FnDescription *Desc,
   }
 }
 
+void StreamChecker::preWrite(const FnDescription *Desc, const CallEvent ,

balazske wrote:

The `CallDescriptionMap` was uncomfortable to handle because too many 
`std::bind` functions.

https://github.com/llvm/llvm-project/pull/84191
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2024-03-07 Thread Balázs Kéri via cfe-commits 

https://github.com/balazske updated 
https://github.com/llvm/llvm-project/pull/84191

From dbaf3348510582c013254ed48b69663b42816be0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= 
Date: Wed, 6 Mar 2024 16:01:01 +0100
Subject: [PATCH] [clang][analyzer] Fix StreamChecker `ftell` and `fgetpos` at
 indeterminate file position.

These functions should not be allowed if the file position is indeterminate
(they return the file position).
This condition is now checked, and tests are improved to check it.
---
 .../StaticAnalyzer/Checkers/StreamChecker.cpp | 91 ---
 clang/test/Analysis/stream-error.c| 27 +-
 2 files changed, 80 insertions(+), 38 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 2ec47bf55df76b..10972158f39862 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -307,64 +307,64 @@ class StreamChecker : public Checker FnTestDescriptions = {
   {{{"StreamTesterChecker_make_feof_stream"}, 1},
{nullptr,
-std::bind(::evalSetFeofFerror, _1, _2, _3, _4, 
ErrorFEof),
+std::bind(::evalSetFeofFerror, _1, _2, _3, _4, ErrorFEof,
+  false),
 0}},
   {{{"StreamTesterChecker_make_ferror_stream"}, 1},
{nullptr,
 std::bind(::evalSetFeofFerror, _1, _2, _3, _4,
-  ErrorFError),
+  ErrorFError, false),
+0}},
+  {{{"StreamTesterChecker_make_ferror_indeterminate_stream"}, 1},
+   {nullptr,
+std::bind(::evalSetFeofFerror, _1, _2, _3, _4,
+  ErrorFError, true),
 0}},
   };
 
@@ -415,8 +421,11 @@ class StreamChecker : public CheckerStreamArgNo), C,
@@ -865,11 +873,6 @@ void StreamChecker::preReadWrite(const FnDescription *Desc,
   if (!State)
 return;
 
-  if (!IsRead) {
-C.addTransition(State);
-return;
-  }
-
   SymbolRef Sym = StreamVal.getAsSymbol();
   if (Sym && State->get(Sym)) {
 const StreamState *SS = State->get(Sym);
@@ -880,6 +883,24 @@ void StreamChecker::preReadWrite(const FnDescription *Desc,
   }
 }
 
+void StreamChecker::preWrite(const FnDescription *Desc, const CallEvent ,
+ CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+  SVal StreamVal = getStreamArg(Desc, Call);
+  State = ensureStreamNonNull(StreamVal, Call.getArgExpr(Desc->StreamArgNo), C,
+  State);
+  if (!State)
+return;
+  State = ensureStreamOpened(StreamVal, C, State);
+  if (!State)
+return;
+  State = ensureNoFilePositionIndeterminate(StreamVal, C, State);
+  if (!State)
+return;
+
+  C.addTransition(State);
+}
+
 void StreamChecker::evalFreadFwrite(const FnDescription *Desc,
 const CallEvent , CheckerContext ,
 bool IsFread) const {
@@ -1496,14 +1517,16 @@ void StreamChecker::preDefault(const FnDescription 
*Desc, const CallEvent ,
 
 void StreamChecker::evalSetFeofFerror(const FnDescription *Desc,
   const CallEvent , CheckerContext ,
-  const StreamErrorState ) const 
{
+  const StreamErrorState ,
+  bool Indeterminate) const {
   ProgramStateRef State = C.getState();
   SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol();
   assert(StreamSym && "Operation not permitted on non-symbolic stream value.");
   const StreamState *SS = State->get(StreamSym);
   assert(SS && "Stream should be tracked by the checker.");
   State = State->set(
-  StreamSym, StreamState::getOpened(SS->LastOperation, ErrorKind));
+  StreamSym,
+  StreamState::getOpened(SS->LastOperation, ErrorKind, Indeterminate));
   C.addTransition(State);
 }
 
diff --git a/clang/test/Analysis/stream-error.c 
b/clang/test/Analysis/stream-error.c
index ac31083bfc691f..88f7de4234ffb4 100644
--- a/clang/test/Analysis/stream-error.c
+++ b/clang/test/Analysis/stream-error.c
@@ -11,6 +11,7 @@ void clang_analyzer_dump(int);
 void clang_analyzer_warnIfReached(void);
 void StreamTesterChecker_make_feof_stream(FILE *);
 void StreamTesterChecker_make_ferror_stream(FILE *);
+void StreamTesterChecker_make_ferror_indeterminate_stream(FILE *);
 
 void error_fopen(void) {
   FILE *F = fopen("file", "r");
@@ -52,6 +53,8 @@ void stream_error_feof(void) {
   clearerr(F);
   clang_analyzer_eval(feof(F));   // expected-warning {{FALSE}}
   clang_analyzer_eval(ferror(F)); // expected-warning {{FALSE}}
+  StreamTesterChecker_make_ferror_indeterminate_stream(F);
+  clang_analyzer_eval(feof(F));   // expected-warning {{FALSE}}
   fclose(F);
 }
 
@@ -65,6 +68,8 @@ void stream_error_ferror(void) {
   clearerr(F);
   clang_analyzer_eval(feof(F));   // expected-warning {{FALSE}}
   clang_analyzer_eval(ferror(F)); //

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

2024-03-06 Thread Ben Shi via cfe-commits 


@@ -880,6 +883,24 @@ void StreamChecker::preReadWrite(const FnDescription *Desc,
   }
 }
 
+void StreamChecker::preWrite(const FnDescription *Desc, const CallEvent ,

benshi001 wrote:

Why 　we need a separated `preWrite` ? The original `preReadWrite` also works.

https://github.com/llvm/llvm-project/pull/84191
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2024-03-06 Thread via cfe-commits 

llvmbot wrote:




@llvm/pr-subscribers-clang

Author: Balázs Kéri (balazske)


Changes

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

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


2 Files Affected:

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


``diff
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 2ec47bf55df76b..346d4ae8db2dfe 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -307,64 +307,64 @@ class StreamChecker : public Checker FnTestDescriptions = {
   {{{"StreamTesterChecker_make_feof_stream"}, 1},
{nullptr,
-std::bind(::evalSetFeofFerror, _1, _2, _3, _4, 
ErrorFEof),
+std::bind(::evalSetFeofFerror, _1, _2, _3, _4, ErrorFEof,
+  false),
 0}},
   {{{"StreamTesterChecker_make_ferror_stream"}, 1},
{nullptr,
 std::bind(::evalSetFeofFerror, _1, _2, _3, _4,
-  ErrorFError),
+  ErrorFError, false),
+0}},
+  {{{"StreamTesterChecker_make_ferror_indeterminate_stream"}, 1},
+   {nullptr,
+std::bind(::evalSetFeofFerror, _1, _2, _3, _4,
+  ErrorFError, true),
 0}},
   };
 
@@ -415,8 +421,11 @@ class StreamChecker : public CheckerStreamArgNo), C,
@@ -865,11 +873,6 @@ void StreamChecker::preReadWrite(const FnDescription *Desc,
   if (!State)
 return;
 
-  if (!IsRead) {
-C.addTransition(State);
-return;
-  }
-
   SymbolRef Sym = StreamVal.getAsSymbol();
   if (Sym && State->get(Sym)) {
 const StreamState *SS = State->get(Sym);
@@ -880,6 +883,24 @@ void StreamChecker::preReadWrite(const FnDescription *Desc,
   }
 }
 
+void StreamChecker::preWrite(const FnDescription *Desc, const CallEvent ,
+ CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+  SVal StreamVal = getStreamArg(Desc, Call);
+  State = ensureStreamNonNull(StreamVal, Call.getArgExpr(Desc->StreamArgNo), C,
+  State);
+  if (!State)
+return;
+  State = ensureStreamOpened(StreamVal, C, State);
+  if (!State)
+return;
+  State = ensureNoFilePositionIndeterminate(StreamVal, C, State);
+  if (!State)
+return;
+
+  C.addTransition(State);
+}
+
 void StreamChecker::evalFreadFwrite(const FnDescription *Desc,
 const CallEvent , CheckerContext ,
 bool IsFread) const {
@@ -1496,14 +1517,16 @@ void StreamChecker::preDefault(const FnDescription 
*Desc, const CallEvent ,
 
 void StreamChecker::evalSetFeofFerror(const FnDescription *Desc,
   const CallEvent , CheckerContext ,
-  const StreamErrorState ) const 
{
+  const StreamErrorState ,
+  bool Indeterminate) const {
   ProgramStateRef State = C.getState();
   SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol();
   assert(StreamSym && "Operation not permitted on non-symbolic stream value.");
   const StreamState *SS = State->get(StreamSym);
   assert(SS && "Stream should be tracked by the checker.");
   State = State->set(
-  StreamSym, StreamState::getOpened(SS->LastOperation, ErrorKind));
+  StreamSym,
+  StreamState::getOpened(SS->LastOperation, ErrorKind, Indeterminate));
   C.addTransition(State);
 }
 
diff --git a/clang/test/Analysis/stream-error.c 
b/clang/test/Analysis/stream-error.c
index ac31083bfc691f..88f7de4234ffb4 100644
--- a/clang/test/Analysis/stream-error.c
+++ b/clang/test/Analysis/stream-error.c
@@ -11,6 +11,7 @@ void clang_analyzer_dump(int);
 void clang_analyzer_warnIfReached(void);
 void StreamTesterChecker_make_feof_stream(FILE *);
 void StreamTesterChecker_make_ferror_stream(FILE *);
+void StreamTesterChecker_make_ferror_indeterminate_stream(FILE *);
 
 void error_fopen(void) {
   FILE *F = fopen("file", "r");
@@ -52,6 +53,8 @@ void stream_error_feof(void) {
   clearerr(F);
   clang_analyzer_eval(feof(F));   // expected-warning {{FALSE}}
   clang_analyzer_eval(ferror(F)); // expected-warning {{FALSE}}
+  StreamTesterChecker_make_ferror_indeterminate_stream(F);
+  clang_analyzer_eval(feof(F));   // expected-warning {{FALSE}}
   fclose(F);
 }
 
@@ -65,6 +68,8 @@ void stream_error_ferror(void) {
   clearerr(F);
   clang_analyzer_eval(feof(F));   // expected-warning {{FALSE}}
   clang_analyzer_eval(ferror(F)); // expected-warning {{FALSE}}
+  StreamTesterChecker_make_ferror_indeterminate_stream(F);
+  clang_analyzer_eval(ferror(F)); // expected-warning {{TRUE}}
   fclose(F);
 }
 
@@ -233,7 +238,7 @@ void

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

2024-03-06 Thread Balázs Kéri via cfe-commits 

https://github.com/balazske created 
https://github.com/llvm/llvm-project/pull/84191

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

From f5e2ba88eb79d778daf9d3e8f55a57c15db961a5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= 
Date: Wed, 6 Mar 2024 16:01:01 +0100
Subject: [PATCH] [clang][analyzer] Fix StreamChecker `ftell` and `fgetpos` at
 indeterminate file position.

These functions should not be allowed if the file position is indeterminate
(they return the file position).
This condition is now checked, and tests are improved to check it.
---
 .../StaticAnalyzer/Checkers/StreamChecker.cpp | 83 ---
 clang/test/Analysis/stream-error.c| 27 +-
 2 files changed, 76 insertions(+), 34 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 2ec47bf55df76b..346d4ae8db2dfe 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -307,64 +307,64 @@ class StreamChecker : public Checker FnTestDescriptions = {
   {{{"StreamTesterChecker_make_feof_stream"}, 1},
{nullptr,
-std::bind(::evalSetFeofFerror, _1, _2, _3, _4, 
ErrorFEof),
+std::bind(::evalSetFeofFerror, _1, _2, _3, _4, ErrorFEof,
+  false),
 0}},
   {{{"StreamTesterChecker_make_ferror_stream"}, 1},
{nullptr,
 std::bind(::evalSetFeofFerror, _1, _2, _3, _4,
-  ErrorFError),
+  ErrorFError, false),
+0}},
+  {{{"StreamTesterChecker_make_ferror_indeterminate_stream"}, 1},
+   {nullptr,
+std::bind(::evalSetFeofFerror, _1, _2, _3, _4,
+  ErrorFError, true),
 0}},
   };
 
@@ -415,8 +421,11 @@ class StreamChecker : public CheckerStreamArgNo), C,
@@ -865,11 +873,6 @@ void StreamChecker::preReadWrite(const FnDescription *Desc,
   if (!State)
 return;
 
-  if (!IsRead) {
-C.addTransition(State);
-return;
-  }
-
   SymbolRef Sym = StreamVal.getAsSymbol();
   if (Sym && State->get(Sym)) {
 const StreamState *SS = State->get(Sym);
@@ -880,6 +883,24 @@ void StreamChecker::preReadWrite(const FnDescription *Desc,
   }
 }
 
+void StreamChecker::preWrite(const FnDescription *Desc, const CallEvent ,
+ CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+  SVal StreamVal = getStreamArg(Desc, Call);
+  State = ensureStreamNonNull(StreamVal, Call.getArgExpr(Desc->StreamArgNo), C,
+  State);
+  if (!State)
+return;
+  State = ensureStreamOpened(StreamVal, C, State);
+  if (!State)
+return;
+  State = ensureNoFilePositionIndeterminate(StreamVal, C, State);
+  if (!State)
+return;
+
+  C.addTransition(State);
+}
+
 void StreamChecker::evalFreadFwrite(const FnDescription *Desc,
 const CallEvent , CheckerContext ,
 bool IsFread) const {
@@ -1496,14 +1517,16 @@ void StreamChecker::preDefault(const FnDescription 
*Desc, const CallEvent ,
 
 void StreamChecker::evalSetFeofFerror(const FnDescription *Desc,
   const CallEvent , CheckerContext ,
-  const StreamErrorState ) const 
{
+  const StreamErrorState ,
+  bool Indeterminate) const {
   ProgramStateRef State = C.getState();
   SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol();
   assert(StreamSym && "Operation not permitted on non-symbolic stream value.");
   const StreamState *SS = State->get(StreamSym);
   assert(SS && "Stream should be tracked by the checker.");
   State = State->set(
-  StreamSym, StreamState::getOpened(SS->LastOperation, ErrorKind));
+  StreamSym,
+  StreamState::getOpened(SS->LastOperation, ErrorKind, Indeterminate));
   C.addTransition(State);
 }
 
diff --git a/clang/test/Analysis/stream-error.c 
b/clang/test/Analysis/stream-error.c
index ac31083bfc691f..88f7de4234ffb4 100644
--- a/clang/test/Analysis/stream-error.c
+++ b/clang/test/Analysis/stream-error.c
@@ -11,6 +11,7 @@ void clang_analyzer_dump(int);
 void clang_analyzer_warnIfReached(void);
 void StreamTesterChecker_make_feof_stream(FILE *);
 void StreamTesterChecker_make_ferror_stream(FILE *);
+void StreamTesterChecker_make_ferror_indeterminate_stream(FILE *);
 
 void error_fopen(void) {
   FILE *F = fopen("file", "r");
@@ -52,6 +53,8 @@ void stream_error_feof(void) {
   clearerr(F);
   clang_analyzer_eval(feof(F));   // expected-warning {{FALSE}}
   clang_analyzer_eval(ferror(F)); // expected-warning {{FALSE}}
+  StreamTesterChecker_make_ferror_indeterminate_stream(F);
+  clang_analyzer_eval(feof(F));   // expected-warning {{FALSE}}