[clang] [clang][analyzer] Fix argument invalidations in StreamChecker. (PR #79470)

2024-02-23 Thread Balázs Kéri via cfe-commits

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


[clang] [clang][analyzer] Fix argument invalidations in StreamChecker. (PR #79470)

2024-02-22 Thread Balázs Kéri via cfe-commits

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

From 70eeae8170a782b93b546b81ac913e1b8eacd28e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= 
Date: Thu, 22 Feb 2024 10:18:06 +0100
Subject: [PATCH 1/2] [clang][analyzer] Fix argument invalidations in
 StreamChecker.

Specific arguments passed to stream handling functions are changed by the 
function,
this means these should be invalidated ("escaped") by the analyzer.
This change adds the argument invalidation (in specific cases) to the checker.
---
 .../StaticAnalyzer/Checkers/StreamChecker.cpp | 39 ++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index a070f451694a3b..65bdc4cac30940 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -21,6 +21,7 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
+#include "llvm/ADT/Sequence.h"
 #include 
 #include 
 
@@ -629,6 +630,21 @@ const ExplodedNode 
*StreamChecker::getAcquisitionSite(const ExplodedNode *N,
   return nullptr;
 }
 
+static ProgramStateRef escapeArgs(ProgramStateRef State, CheckerContext &C,
+  const CallEvent &Call,
+  ArrayRef EscapingArgs) {
+  const auto *CE = Call.getOriginExpr();
+
+  SmallVector EscapingVals;
+  EscapingVals.reserve(EscapingArgs.size());
+  for (auto EscArgIdx : EscapingArgs)
+EscapingVals.push_back(Call.getArgSVal(EscArgIdx));
+  State = State->invalidateRegions(EscapingVals, CE, C.blockCount(),
+   C.getLocationContext(),
+   /*CausesPointerEscape=*/false);
+  return State;
+}
+
 
//===--===//
 // Methods of StreamChecker.
 
//===--===//
@@ -819,6 +835,11 @@ void StreamChecker::evalFreadFwrite(const FnDescription 
*Desc,
 return;
   }
 
+  // At read, invalidate the buffer in any case of error or success,
+  // except if EOF was already present.
+  if (IsFread && !E.isStreamEof())
+State = escapeArgs(State, C, Call, {0});
+
   // Generate a transition for the success state.
   // If we know the state to be FEOF at fread, do not add a success state.
   if (!IsFread || !E.isStreamEof()) {
@@ -863,6 +884,9 @@ void StreamChecker::evalFgetx(const FnDescription *Desc, 
const CallEvent &Call,
 return;
 
   if (!E.isStreamEof()) {
+// If there was already EOF, assume that read buffer is not changed.
+// Otherwise it may change at success or failure.
+State = escapeArgs(State, C, Call, {0});
 if (SingleChar) {
   // Generate a transition for the success state of `fgetc`.
   NonLoc RetVal = makeRetVal(C, E.CE).castAs();
@@ -1011,6 +1035,14 @@ void StreamChecker::evalFscanf(const FnDescription 
*Desc, const CallEvent &Call,
 State->BindExpr(E.CE, C.getLocationContext(), RetVal);
 StateNotFailed =
 E.assumeBinOpNN(StateNotFailed, BO_GE, RetVal, E.getZeroVal(Call));
+if (!StateNotFailed)
+  return;
+
+SmallVector EscArgs;
+for (auto EscArg : llvm::seq(2u, Call.getNumArgs()))
+  EscArgs.push_back(EscArg);
+StateNotFailed = escapeArgs(StateNotFailed, C, Call, EscArgs);
+
 if (StateNotFailed)
   C.addTransition(StateNotFailed);
   }
@@ -1073,8 +1105,12 @@ void StreamChecker::evalGetdelim(const FnDescription 
*Desc,
   // return -1.
   // If an error occurs, the function shall return -1 and set 'errno'.
 
-  // Add transition for the successful state.
   if (!E.isStreamEof()) {
+// Escape buffer and size (may change by the call).
+// May happen even at error (partial read?).
+State = escapeArgs(State, C, Call, {0, 1});
+
+// Add transition for the successful state.
 NonLoc RetVal = makeRetVal(C, E.CE).castAs();
 ProgramStateRef StateNotFailed =
 State->BindExpr(E.CE, C.getLocationContext(), RetVal);
@@ -1161,6 +1197,7 @@ void StreamChecker::evalFgetpos(const FnDescription *Desc,
 
   ProgramStateRef StateNotFailed, StateFailed;
   std::tie(StateFailed, StateNotFailed) = E.makeRetValAndAssumeDual(State, C);
+  StateNotFailed = escapeArgs(StateNotFailed, C, Call, {1});
 
   // This function does not affect the stream state.
   // Still we add success and failure state with the appropriate return value.

From deb62b2e2694e8a45eca1c6001b8d90dfb1b1d7b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= 
Date: Fri, 23 Feb 2024 08:59:03 +0100
Subject: [PATCH 2/2] Add test file

---
 clang/test/Analysis/stream-invalidate.c | 147 
 1 file changed, 147 inserti

[clang] [clang][analyzer] Fix argument invalidations in StreamChecker. (PR #79470)

2024-02-22 Thread Alejandro Álvarez Ayllón via cfe-commits


@@ -763,6 +779,11 @@ void StreamChecker::evalFreadFwrite(const FnDescription 
*Desc,
 return;
   }
 
+  // At read, invalidate the buffer in any case of error or success,
+  // except if EOF was already present.
+  if (IsFread && (OldSS->ErrorState != ErrorFEof))
+State = escapeArgs(State, C, Call, {0});

alejandro-alvarez-sonarsource wrote:

Ok, let’s do that, and send this patch as a separate pr. @steakhal LGTM.

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


[clang] [clang][analyzer] Fix argument invalidations in StreamChecker. (PR #79470)

2024-02-22 Thread Balázs Kéri via cfe-commits


@@ -763,6 +779,11 @@ void StreamChecker::evalFreadFwrite(const FnDescription 
*Desc,
 return;
   }
 
+  // At read, invalidate the buffer in any case of error or success,
+  // except if EOF was already present.
+  if (IsFread && (OldSS->ErrorState != ErrorFEof))
+State = escapeArgs(State, C, Call, {0});

balazske wrote:

This could be a future improvement (after this patch). I am not sure if this 
works always correct, because it looks like that the `ElementCount` value at 
`fread` is used but not the "size" argument (size of one element). It is 
assumed that the specified element size is always the same as one array 
element, but this can be different.

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


[clang] [clang][analyzer] Fix argument invalidations in StreamChecker. (PR #79470)

2024-02-22 Thread Alejandro Álvarez Ayllón via cfe-commits


@@ -763,6 +779,11 @@ void StreamChecker::evalFreadFwrite(const FnDescription 
*Desc,
 return;
   }
 
+  // At read, invalidate the buffer in any case of error or success,
+  // except if EOF was already present.
+  if (IsFread && (OldSS->ErrorState != ErrorFEof))
+State = escapeArgs(State, C, Call, {0});

alejandro-alvarez-sonarsource wrote:

Here it is: 
https://gist.github.com/alejandro-alvarez-sonarsource/48edec4debc8912a6485f989b2a6f0db

I have rebased it on top of 4f12f47550eee85447c9ec37d27a20c6593d3d40 and run 
check-clang to make sure I didn't break anything.

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


[clang] [clang][analyzer] Fix argument invalidations in StreamChecker. (PR #79470)

2024-02-22 Thread Alejandro Álvarez Ayllón via cfe-commits


@@ -763,6 +779,11 @@ void StreamChecker::evalFreadFwrite(const FnDescription 
*Desc,
 return;
   }
 
+  // At read, invalidate the buffer in any case of error or success,
+  // except if EOF was already present.
+  if (IsFread && (OldSS->ErrorState != ErrorFEof))
+State = escapeArgs(State, C, Call, {0});

alejandro-alvarez-sonarsource wrote:

It turns out I was comparing with our downstream copy of v17, where 
`UpdateBufferRegionForFread` is defined.
So it can be done for the case of arrays. I have double-checked with the 
original author and he agrees it can be upstreamed. Let me uplift the patch and 
then I will share it with you.

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


[clang] [clang][analyzer] Fix argument invalidations in StreamChecker. (PR #79470)

2024-02-22 Thread Balázs Kéri via cfe-commits

balazske wrote:

I have rebased the branch to latest version of `StreamChecker`, no other 
changes were made to the patch.

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


[clang] [clang][analyzer] Fix argument invalidations in StreamChecker. (PR #79470)

2024-02-22 Thread Balázs Kéri via cfe-commits

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

From 70eeae8170a782b93b546b81ac913e1b8eacd28e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= 
Date: Thu, 22 Feb 2024 10:18:06 +0100
Subject: [PATCH] [clang][analyzer] Fix argument invalidations in
 StreamChecker.

Specific arguments passed to stream handling functions are changed by the 
function,
this means these should be invalidated ("escaped") by the analyzer.
This change adds the argument invalidation (in specific cases) to the checker.
---
 .../StaticAnalyzer/Checkers/StreamChecker.cpp | 39 ++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index a070f451694a3b..65bdc4cac30940 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -21,6 +21,7 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
+#include "llvm/ADT/Sequence.h"
 #include 
 #include 
 
@@ -629,6 +630,21 @@ const ExplodedNode 
*StreamChecker::getAcquisitionSite(const ExplodedNode *N,
   return nullptr;
 }
 
+static ProgramStateRef escapeArgs(ProgramStateRef State, CheckerContext &C,
+  const CallEvent &Call,
+  ArrayRef EscapingArgs) {
+  const auto *CE = Call.getOriginExpr();
+
+  SmallVector EscapingVals;
+  EscapingVals.reserve(EscapingArgs.size());
+  for (auto EscArgIdx : EscapingArgs)
+EscapingVals.push_back(Call.getArgSVal(EscArgIdx));
+  State = State->invalidateRegions(EscapingVals, CE, C.blockCount(),
+   C.getLocationContext(),
+   /*CausesPointerEscape=*/false);
+  return State;
+}
+
 
//===--===//
 // Methods of StreamChecker.
 
//===--===//
@@ -819,6 +835,11 @@ void StreamChecker::evalFreadFwrite(const FnDescription 
*Desc,
 return;
   }
 
+  // At read, invalidate the buffer in any case of error or success,
+  // except if EOF was already present.
+  if (IsFread && !E.isStreamEof())
+State = escapeArgs(State, C, Call, {0});
+
   // Generate a transition for the success state.
   // If we know the state to be FEOF at fread, do not add a success state.
   if (!IsFread || !E.isStreamEof()) {
@@ -863,6 +884,9 @@ void StreamChecker::evalFgetx(const FnDescription *Desc, 
const CallEvent &Call,
 return;
 
   if (!E.isStreamEof()) {
+// If there was already EOF, assume that read buffer is not changed.
+// Otherwise it may change at success or failure.
+State = escapeArgs(State, C, Call, {0});
 if (SingleChar) {
   // Generate a transition for the success state of `fgetc`.
   NonLoc RetVal = makeRetVal(C, E.CE).castAs();
@@ -1011,6 +1035,14 @@ void StreamChecker::evalFscanf(const FnDescription 
*Desc, const CallEvent &Call,
 State->BindExpr(E.CE, C.getLocationContext(), RetVal);
 StateNotFailed =
 E.assumeBinOpNN(StateNotFailed, BO_GE, RetVal, E.getZeroVal(Call));
+if (!StateNotFailed)
+  return;
+
+SmallVector EscArgs;
+for (auto EscArg : llvm::seq(2u, Call.getNumArgs()))
+  EscArgs.push_back(EscArg);
+StateNotFailed = escapeArgs(StateNotFailed, C, Call, EscArgs);
+
 if (StateNotFailed)
   C.addTransition(StateNotFailed);
   }
@@ -1073,8 +1105,12 @@ void StreamChecker::evalGetdelim(const FnDescription 
*Desc,
   // return -1.
   // If an error occurs, the function shall return -1 and set 'errno'.
 
-  // Add transition for the successful state.
   if (!E.isStreamEof()) {
+// Escape buffer and size (may change by the call).
+// May happen even at error (partial read?).
+State = escapeArgs(State, C, Call, {0, 1});
+
+// Add transition for the successful state.
 NonLoc RetVal = makeRetVal(C, E.CE).castAs();
 ProgramStateRef StateNotFailed =
 State->BindExpr(E.CE, C.getLocationContext(), RetVal);
@@ -1161,6 +1197,7 @@ void StreamChecker::evalFgetpos(const FnDescription *Desc,
 
   ProgramStateRef StateNotFailed, StateFailed;
   std::tie(StateFailed, StateNotFailed) = E.makeRetValAndAssumeDual(State, C);
+  StateNotFailed = escapeArgs(StateNotFailed, C, Call, {1});
 
   // This function does not affect the stream state.
   // Still we add success and failure state with the appropriate return value.

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


[clang] [clang][analyzer] Fix argument invalidations in StreamChecker. (PR #79470)

2024-02-21 Thread Balázs Kéri via cfe-commits

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


[clang] [clang][analyzer] Fix argument invalidations in StreamChecker. (PR #79470)

2024-02-21 Thread Balázs Kéri via cfe-commits


@@ -763,6 +779,11 @@ void StreamChecker::evalFreadFwrite(const FnDescription 
*Desc,
 return;
   }
 
+  // At read, invalidate the buffer in any case of error or success,
+  // except if EOF was already present.
+  if (IsFread && (OldSS->ErrorState != ErrorFEof))
+State = escapeArgs(State, C, Call, {0});

balazske wrote:

The current solution invalidates the whole buffer (if I think correctly) 
because the `SVal` of the buffer is passed to `invalidateRegions`. Is there a 
solution to invalidate a partial buffer (of specific size)?

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


[clang] [clang][analyzer] Fix argument invalidations in StreamChecker. (PR #79470)

2024-02-21 Thread Alejandro Álvarez Ayllón via cfe-commits

https://github.com/alejandro-alvarez-sonarsource edited 
https://github.com/llvm/llvm-project/pull/79470
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [clang][analyzer] Fix argument invalidations in StreamChecker. (PR #79470)

2024-02-21 Thread Alejandro Álvarez Ayllón via cfe-commits

https://github.com/alejandro-alvarez-sonarsource edited 
https://github.com/llvm/llvm-project/pull/79470
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [clang][analyzer] Fix argument invalidations in StreamChecker. (PR #79470)

2024-02-21 Thread Alejandro Álvarez Ayllón via cfe-commits


@@ -763,6 +779,11 @@ void StreamChecker::evalFreadFwrite(const FnDescription 
*Desc,
 return;
   }
 
+  // At read, invalidate the buffer in any case of error or success,
+  // except if EOF was already present.
+  if (IsFread && (OldSS->ErrorState != ErrorFEof))
+State = escapeArgs(State, C, Call, {0});

alejandro-alvarez-sonarsource wrote:

From what I can tell, the lambda `UpdateBufferRegionForFread` is already used 
to take care of this in a more fine-grained manner. For instance:

```cpp
int buffer[10];
buffer[5] = 42;
if (1 == fread(buffer, sizeof(int), 5, fd)) {
assert(buffer[5] == 42);
}
```

Before this change, the assertion would pass, since lambda took `nmemb` into 
account. With this change, the whole buffer is invalidated.

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


[clang] [clang][analyzer] Fix argument invalidations in StreamChecker. (PR #79470)

2024-02-21 Thread Balazs Benics via cfe-commits
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 


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

Thanks for resolving my comments.
FYI if I forget about a PR (that I promise to come back on the next day) - feel 
free to ping it or explicitly push the "request review" button.

Wait for my collage to also have a look, as I believe he might be in context to 
review this change. @alejandro-alvarez-sonarsource 

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


[clang] [clang][analyzer] Fix argument invalidations in StreamChecker. (PR #79470)

2024-01-26 Thread Balázs Kéri via cfe-commits

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

From dbd9af4e77c34fcf6ce82f226f7dbf836033a8f6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= 
Date: Thu, 25 Jan 2024 17:50:42 +0100
Subject: [PATCH 1/2] [clang][analyzer] Fix argument invalidations in
 StreamChecker.

Specific arguments passed to stream handling functions are changed by the 
function,
this means these should be invalidated ("escaped") by the analyzer.
This change adds the argument invalidation (in specific cases) to the checker.
---
 .../StaticAnalyzer/Checkers/StreamChecker.cpp |  35 +
 clang/test/Analysis/stream-invalidate.c   | 133 ++
 2 files changed, 168 insertions(+)
 create mode 100644 clang/test/Analysis/stream-invalidate.c

diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 07727b339d967ae..166bd981a003af2 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -21,6 +21,7 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
+#include "llvm/ADT/Sequence.h"
 #include 
 #include 
 
@@ -544,6 +545,21 @@ const ExplodedNode 
*StreamChecker::getAcquisitionSite(const ExplodedNode *N,
   return nullptr;
 }
 
+static ProgramStateRef
+escapeArgs(ProgramStateRef State, CheckerContext &C, const CallEvent &Call,
+   const SmallVector &EscapingArgs) {
+  const auto *CE = Call.getOriginExpr();
+
+  SmallVector EscapingVals;
+  EscapingVals.reserve(EscapingArgs.size());
+  for (auto EscArgIdx : EscapingArgs)
+EscapingVals.push_back(Call.getArgSVal(EscArgIdx));
+  State = State->invalidateRegions(EscapingVals, CE, C.blockCount(),
+   C.getLocationContext(),
+   /*CausesPointerEscape=*/false);
+  return State;
+}
+
 
//===--===//
 // Methods of StreamChecker.
 
//===--===//
@@ -763,6 +779,11 @@ void StreamChecker::evalFreadFwrite(const FnDescription 
*Desc,
 return;
   }
 
+  // At read, invalidate the buffer in any case of error or success,
+  // except if EOF was already present.
+  if (IsFread && (OldSS->ErrorState != ErrorFEof))
+State = escapeArgs(State, C, Call, {0});
+
   // Generate a transition for the success state.
   // If we know the state to be FEOF at fread, do not add a success state.
   if (!IsFread || (OldSS->ErrorState != ErrorFEof)) {
@@ -824,6 +845,10 @@ void StreamChecker::evalFgetx(const FnDescription *Desc, 
const CallEvent &Call,
   // `fgets` returns the read buffer address on success, otherwise returns 
NULL.
 
   if (OldSS->ErrorState != ErrorFEof) {
+// If there was already EOF, assume that read buffer is not changed.
+// Otherwise it may change at success or failure.
+
+State = escapeArgs(State, C, Call, {0});
 if (SingleChar) {
   // Generate a transition for the success state of `fgetc`.
   NonLoc RetVal = makeRetVal(C, CE).castAs();
@@ -1032,6 +1057,11 @@ void StreamChecker::evalFscanf(const FnDescription 
*Desc, const CallEvent &Call,
   return;
 StateNotFailed = StateNotFailed->assume(*RetGeZero, true);
 
+SmallVector EscArgs;
+for (auto EscArg : llvm::seq(2u, Call.getNumArgs()))
+  EscArgs.push_back(EscArg);
+StateNotFailed = escapeArgs(StateNotFailed, C, Call, EscArgs);
+
 C.addTransition(StateNotFailed);
   }
 
@@ -1118,6 +1148,10 @@ void StreamChecker::evalGetdelim(const FnDescription 
*Desc,
 
   // Add transition for the successful state.
   if (OldSS->ErrorState != ErrorFEof) {
+// Escape buffer and size (may change by the call).
+// May happen even at error (partial read?).
+State = escapeArgs(State, C, Call, {0, 1});
+
 NonLoc RetVal = makeRetVal(C, CE).castAs();
 ProgramStateRef StateNotFailed =
 State->BindExpr(CE, C.getLocationContext(), RetVal);
@@ -1236,6 +1270,7 @@ void StreamChecker::evalFgetpos(const FnDescription *Desc,
   ProgramStateRef StateNotFailed, StateFailed;
   std::tie(StateFailed, StateNotFailed) =
   C.getConstraintManager().assumeDual(State, RetVal);
+  StateNotFailed = escapeArgs(StateNotFailed, C, Call, {1});
 
   // This function does not affect the stream state.
   // Still we add success and failure state with the appropriate return value.
diff --git a/clang/test/Analysis/stream-invalidate.c 
b/clang/test/Analysis/stream-invalidate.c
new file mode 100644
index 000..c5b7874f0ee8f6f
--- /dev/null
+++ b/clang/test/Analysis/stream-invalidate.c
@@ -0,0 +1,133 @@
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN: -analyzer-checker=core \
+// RUN: -analyzer-checker=alpha.unix.Stream \
+// RUN: -analyze

[clang] [clang][analyzer] Fix argument invalidations in StreamChecker. (PR #79470)

2024-01-26 Thread Balazs Benics via cfe-commits


@@ -544,6 +545,21 @@ const ExplodedNode 
*StreamChecker::getAcquisitionSite(const ExplodedNode *N,
   return nullptr;
 }
 
+static ProgramStateRef
+escapeArgs(ProgramStateRef State, CheckerContext &C, const CallEvent &Call,
+   const SmallVector &EscapingArgs) {
+  const auto *CE = Call.getOriginExpr();
+
+  SmallVector EscapingVals;
+  EscapingVals.reserve(EscapingArgs.size());
+  for (auto EscArgIdx : EscapingArgs)
+EscapingVals.push_back(Call.getArgSVal(EscArgIdx));
+  State = State->invalidateRegions(EscapingVals, CE, C.blockCount(),
+   C.getLocationContext(),
+   /*CausesPointerEscape=*/false);

steakhal wrote:

Yup, have a look at `RegionStoreManager::invalidateRegions`. Depending on the 
nature of the call, invalidates system or every global variables.
We don't really care about it, so let's just not pass the `Call` here.

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


[clang] [clang][analyzer] Fix argument invalidations in StreamChecker. (PR #79470)

2024-01-26 Thread Balázs Kéri via cfe-commits


@@ -544,6 +545,21 @@ const ExplodedNode 
*StreamChecker::getAcquisitionSite(const ExplodedNode *N,
   return nullptr;
 }
 
+static ProgramStateRef
+escapeArgs(ProgramStateRef State, CheckerContext &C, const CallEvent &Call,
+   const SmallVector &EscapingArgs) {
+  const auto *CE = Call.getOriginExpr();
+
+  SmallVector EscapingVals;
+  EscapingVals.reserve(EscapingArgs.size());
+  for (auto EscArgIdx : EscapingArgs)
+EscapingVals.push_back(Call.getArgSVal(EscArgIdx));
+  State = State->invalidateRegions(EscapingVals, CE, C.blockCount(),
+   C.getLocationContext(),
+   /*CausesPointerEscape=*/false);

balazske wrote:

If this change is made, a error shows up in **stream-errno.c** line 190 
(unexpected warning: FALSE).
```
  int Ret = fgetpos(F, &Pos);
  if (Ret)
clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}}
  else
clang_analyzer_eval(errno == 0); // expected-warning{{TRUE}} we get FALSE 
too
```
Probably the `errno` is invalidated somehow, this is likely to be caused by the 
`Call` argument. Without these additional arguments to `invalidateRegions` the 
test passes.

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


[clang] [clang][analyzer] Fix argument invalidations in StreamChecker. (PR #79470)

2024-01-26 Thread Balázs Kéri via cfe-commits


@@ -0,0 +1,133 @@
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN: -analyzer-checker=core \
+// RUN: -analyzer-checker=alpha.unix.Stream \
+// RUN: -analyzer-checker=debug.StreamTester \
+// RUN: -analyzer-checker=debug.ExprInspection
+
+#include "Inputs/system-header-simulator.h"
+
+void clang_analyzer_eval(int);
+void clang_analyzer_dump(int);
+void clang_analyzer_warnIfReached(void);
+void StreamTesterChecker_make_feof_stream(FILE *);
+void StreamTesterChecker_make_ferror_stream(FILE *);
+
+void test_fread(void) {
+  FILE *F = fopen("file", "r+");
+  if (!F)
+return;
+
+  char Buf[3] = {10, 10, 10};
+  fread(Buf, 1, 3, F);
+  // this check applies to succes and failure
+  clang_analyzer_dump(Buf[0]); // expected-warning {{conj_$}} Should not 
preserve the previous value, thus should not be 10.
+  clang_analyzer_dump(Buf[2]); // expected-warning {{conj_$}}
+  if (feof(F)) {

balazske wrote:

At `ferror(F)` the next stream call should produce a "file position 
indeterminate" warning (this is tested in another file) that stops the 
analysis, buffer invalidation is not needed to be tested.

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


[clang] [clang][analyzer] Fix argument invalidations in StreamChecker. (PR #79470)

2024-01-25 Thread Balazs Benics via cfe-commits


@@ -0,0 +1,133 @@
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN: -analyzer-checker=core \
+// RUN: -analyzer-checker=alpha.unix.Stream \
+// RUN: -analyzer-checker=debug.StreamTester \
+// RUN: -analyzer-checker=debug.ExprInspection
+
+#include "Inputs/system-header-simulator.h"
+
+void clang_analyzer_eval(int);
+void clang_analyzer_dump(int);
+void clang_analyzer_warnIfReached(void);
+void StreamTesterChecker_make_feof_stream(FILE *);
+void StreamTesterChecker_make_ferror_stream(FILE *);
+
+void test_fread(void) {
+  FILE *F = fopen("file", "r+");
+  if (!F)
+return;
+
+  char Buf[3] = {10, 10, 10};
+  fread(Buf, 1, 3, F);
+  // this check applies to succes and failure
+  clang_analyzer_dump(Buf[0]); // expected-warning {{conj_$}} Should not 
preserve the previous value, thus should not be 10.
+  clang_analyzer_dump(Buf[2]); // expected-warning {{conj_$}}
+  if (feof(F)) {

steakhal wrote:

In this, and the in the rest of the test, could we have a branch for testing if 
the api-call encountered an error?
E.g. when `ferror(F)==true`. On that path, we should still invalidate the 
buffer.

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


[clang] [clang][analyzer] Fix argument invalidations in StreamChecker. (PR #79470)

2024-01-25 Thread Balazs Benics via cfe-commits


@@ -544,6 +545,21 @@ const ExplodedNode 
*StreamChecker::getAcquisitionSite(const ExplodedNode *N,
   return nullptr;
 }
 
+static ProgramStateRef
+escapeArgs(ProgramStateRef State, CheckerContext &C, const CallEvent &Call,
+   const SmallVector &EscapingArgs) {
+  const auto *CE = Call.getOriginExpr();
+
+  SmallVector EscapingVals;
+  EscapingVals.reserve(EscapingArgs.size());
+  for (auto EscArgIdx : EscapingArgs)
+EscapingVals.push_back(Call.getArgSVal(EscArgIdx));
+  State = State->invalidateRegions(EscapingVals, CE, C.blockCount(),
+   C.getLocationContext(),
+   /*CausesPointerEscape=*/false);

steakhal wrote:

```suggestion
  State = State->invalidateRegions(EscapingVals, CE, C.blockCount(),
   C.getLocationContext(),
   /*CausesPointerEscape=*/false, 
/*InvalidatedSymbols=*/nullptr, &Call);
```

I can't recall now what difference it makes to pass the `Call` to this API, but 
given that we have one, why not?

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


[clang] [clang][analyzer] Fix argument invalidations in StreamChecker. (PR #79470)

2024-01-25 Thread Balazs Benics via cfe-commits


@@ -0,0 +1,133 @@
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN: -analyzer-checker=core \
+// RUN: -analyzer-checker=alpha.unix.Stream \
+// RUN: -analyzer-checker=debug.StreamTester \
+// RUN: -analyzer-checker=debug.ExprInspection
+
+#include "Inputs/system-header-simulator.h"
+
+void clang_analyzer_eval(int);
+void clang_analyzer_dump(int);
+void clang_analyzer_warnIfReached(void);
+void StreamTesterChecker_make_feof_stream(FILE *);
+void StreamTesterChecker_make_ferror_stream(FILE *);
+
+void test_fread(void) {
+  FILE *F = fopen("file", "r+");
+  if (!F)
+return;
+
+  char Buf[3] = {10, 10, 10};
+  fread(Buf, 1, 3, F);
+  // this check applies to succes and failure
+  clang_analyzer_dump(Buf[0]); // expected-warning {{conj_$}} Should not 
preserve the previous value, thus should not be 10.
+  clang_analyzer_dump(Buf[2]); // expected-warning {{conj_$}}
+  if (feof(F)) {
+char Buf1[3] = {10, 10, 10};
+fread(Buf1, 1, 3, F); // expected-warning {{is in EOF state}}
+clang_analyzer_dump(Buf1[0]); // expected-warning {{10 S32b}}
+clang_analyzer_dump(Buf1[2]); // expected-warning {{10 S32b}}
+  }
+
+  fclose(F);
+}
+
+void test_fwrite(void) {
+  FILE *F = fopen("file", "r+");
+  if (!F)
+return;
+
+  char Buf[3] = {10, 10, 10};
+  fwrite(Buf, 1, 3, F);
+  // this check applies to succes and failure

steakhal wrote:

```suggestion
  // This check applies to success and failure.
```
As per llvm style, capitalize and punctuate comments.
`sed "s/this check applies to succes and failure/This check applies to success 
and failure."`
There was also a typo `sed "s/succes/success/g"` this file.

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


[clang] [clang][analyzer] Fix argument invalidations in StreamChecker. (PR #79470)

2024-01-25 Thread Balazs Benics via cfe-commits


@@ -0,0 +1,133 @@
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN: -analyzer-checker=core \
+// RUN: -analyzer-checker=alpha.unix.Stream \
+// RUN: -analyzer-checker=debug.StreamTester \
+// RUN: -analyzer-checker=debug.ExprInspection
+
+#include "Inputs/system-header-simulator.h"
+
+void clang_analyzer_eval(int);
+void clang_analyzer_dump(int);
+void clang_analyzer_warnIfReached(void);
+void StreamTesterChecker_make_feof_stream(FILE *);
+void StreamTesterChecker_make_ferror_stream(FILE *);
+
+void test_fread(void) {
+  FILE *F = fopen("file", "r+");
+  if (!F)
+return;
+
+  char Buf[3] = {10, 10, 10};
+  fread(Buf, 1, 3, F);
+  // this check applies to succes and failure
+  clang_analyzer_dump(Buf[0]); // expected-warning {{conj_$}} Should not 
preserve the previous value, thus should not be 10.
+  clang_analyzer_dump(Buf[2]); // expected-warning {{conj_$}}
+  if (feof(F)) {
+char Buf1[3] = {10, 10, 10};
+fread(Buf1, 1, 3, F); // expected-warning {{is in EOF state}}
+clang_analyzer_dump(Buf1[0]); // expected-warning {{10 S32b}}
+clang_analyzer_dump(Buf1[2]); // expected-warning {{10 S32b}}
+  }
+
+  fclose(F);
+}
+
+void test_fwrite(void) {
+  FILE *F = fopen("file", "r+");
+  if (!F)
+return;
+
+  char Buf[3] = {10, 10, 10};
+  fwrite(Buf, 1, 3, F);
+  // this check applies to succes and failure
+  clang_analyzer_dump(Buf[0]); // expected-warning {{10 S32b}}
+  clang_analyzer_dump(Buf[2]); // expected-warning {{10 S32b}}
+
+  fclose(F);
+}
+
+void test_fgets() {
+  FILE *F = tmpfile();
+  if (!F)
+return;
+
+  char Buf[3] = {10, 10, 10};
+  fgets(Buf, 3, F);
+  // this check applies to succes and failure
+  clang_analyzer_dump(Buf[0]); // expected-warning {{conj_$}} Should not 
preserve the previous value, thus should not be 10.
+  clang_analyzer_dump(Buf[2]); // expected-warning {{conj_$}}
+  if (feof(F)) {
+char Buf1[3] = {10, 10, 10};
+fgets(Buf1, 3, F); // expected-warning {{is in EOF state}}
+clang_analyzer_dump(Buf1[0]); // expected-warning {{10 S32b}}
+clang_analyzer_dump(Buf1[2]); // expected-warning {{10 S32b}}
+  }
+
+  fclose(F);
+}
+
+void test_fputs() {
+  FILE *F = tmpfile();
+  if (!F)
+return;
+
+  char *Buf = "aaa";
+  fputs(Buf, F);
+  // this check applies to succes and failure
+  clang_analyzer_dump(Buf[0]); // expected-warning {{97 S32b}}
+  clang_analyzer_dump(Buf[2]); // expected-warning {{97 S32b}}
+  clang_analyzer_dump(Buf[3]); // expected-warning {{0 S32b}}
+
+  fclose(F);
+}
+
+void test_fscanf() {
+  FILE *F = tmpfile();
+  if (!F)
+return;
+
+  int a = 1;
+  unsigned b;
+  int Ret = fscanf(F, "%d %u", &a, &b);
+  if (Ret >= 0) {
+// FIXME: return value
+clang_analyzer_dump(a); // expected-warning {{conj_$}}
+clang_analyzer_dump(b); // expected-warning {{conj_$}}
+  } else {
+clang_analyzer_dump(a); // expected-warning {{1 S32b}}
+clang_analyzer_dump(b); // expected-warning {{uninitialized value}}
+  }
+  fclose(F);
+}
+
+void test_getdelim(char *P, size_t Sz) {
+  FILE *F = tmpfile();
+  if (!F)
+return;
+
+  char *P1 = P;
+  size_t Sz1 = Sz;
+  ssize_t Ret = getdelim(&P, &Sz, '\t', F);
+  clang_analyzer_eval(P == P1); // expected-warning {{FALSE}} \
+// expected-warning {{TRUE}}
+  clang_analyzer_eval(Sz == Sz1); // expected-warning {{FALSE}} \
+  // expected-warning {{TRUE}}

steakhal wrote:

I don't think this is too useful this way. We can't distinguish the failure and 
success branches.
Lets have different branches depending on `Ret`, to make it clear.

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


[clang] [clang][analyzer] Fix argument invalidations in StreamChecker. (PR #79470)

2024-01-25 Thread Balazs Benics via cfe-commits


@@ -544,6 +545,21 @@ const ExplodedNode 
*StreamChecker::getAcquisitionSite(const ExplodedNode *N,
   return nullptr;
 }
 
+static ProgramStateRef
+escapeArgs(ProgramStateRef State, CheckerContext &C, const CallEvent &Call,
+   const SmallVector &EscapingArgs) {

steakhal wrote:

```suggestion
escapeArgs(ProgramStateRef State, CheckerContext &C, const CallEvent &Call,
   ArrayRef EscapingArgs) {
```
Views should be preferred for input parameters.

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


[clang] [clang][analyzer] Fix argument invalidations in StreamChecker. (PR #79470)

2024-01-25 Thread Balazs Benics via cfe-commits


@@ -0,0 +1,133 @@
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN: -analyzer-checker=core \
+// RUN: -analyzer-checker=alpha.unix.Stream \
+// RUN: -analyzer-checker=debug.StreamTester \
+// RUN: -analyzer-checker=debug.ExprInspection
+
+#include "Inputs/system-header-simulator.h"
+
+void clang_analyzer_eval(int);
+void clang_analyzer_dump(int);
+void clang_analyzer_warnIfReached(void);
+void StreamTesterChecker_make_feof_stream(FILE *);
+void StreamTesterChecker_make_ferror_stream(FILE *);
+
+void test_fread(void) {
+  FILE *F = fopen("file", "r+");
+  if (!F)
+return;
+
+  char Buf[3] = {10, 10, 10};
+  fread(Buf, 1, 3, F);
+  // this check applies to succes and failure
+  clang_analyzer_dump(Buf[0]); // expected-warning {{conj_$}} Should not 
preserve the previous value, thus should not be 10.
+  clang_analyzer_dump(Buf[2]); // expected-warning {{conj_$}}
+  if (feof(F)) {
+char Buf1[3] = {10, 10, 10};
+fread(Buf1, 1, 3, F); // expected-warning {{is in EOF state}}
+clang_analyzer_dump(Buf1[0]); // expected-warning {{10 S32b}}
+clang_analyzer_dump(Buf1[2]); // expected-warning {{10 S32b}}
+  }
+
+  fclose(F);
+}
+
+void test_fwrite(void) {
+  FILE *F = fopen("file", "r+");
+  if (!F)
+return;
+
+  char Buf[3] = {10, 10, 10};
+  fwrite(Buf, 1, 3, F);
+  // this check applies to succes and failure
+  clang_analyzer_dump(Buf[0]); // expected-warning {{10 S32b}}
+  clang_analyzer_dump(Buf[2]); // expected-warning {{10 S32b}}
+
+  fclose(F);
+}
+
+void test_fgets() {
+  FILE *F = tmpfile();
+  if (!F)
+return;
+
+  char Buf[3] = {10, 10, 10};
+  fgets(Buf, 3, F);
+  // this check applies to succes and failure
+  clang_analyzer_dump(Buf[0]); // expected-warning {{conj_$}} Should not 
preserve the previous value, thus should not be 10.
+  clang_analyzer_dump(Buf[2]); // expected-warning {{conj_$}}
+  if (feof(F)) {
+char Buf1[3] = {10, 10, 10};
+fgets(Buf1, 3, F); // expected-warning {{is in EOF state}}
+clang_analyzer_dump(Buf1[0]); // expected-warning {{10 S32b}}
+clang_analyzer_dump(Buf1[2]); // expected-warning {{10 S32b}}
+  }
+
+  fclose(F);
+}
+
+void test_fputs() {
+  FILE *F = tmpfile();
+  if (!F)
+return;
+
+  char *Buf = "aaa";
+  fputs(Buf, F);
+  // this check applies to succes and failure
+  clang_analyzer_dump(Buf[0]); // expected-warning {{97 S32b}}
+  clang_analyzer_dump(Buf[2]); // expected-warning {{97 S32b}}
+  clang_analyzer_dump(Buf[3]); // expected-warning {{0 S32b}}
+
+  fclose(F);
+}
+
+void test_fscanf() {
+  FILE *F = tmpfile();
+  if (!F)
+return;
+
+  int a = 1;
+  unsigned b;
+  int Ret = fscanf(F, "%d %u", &a, &b);
+  if (Ret >= 0) {
+// FIXME: return value
+clang_analyzer_dump(a); // expected-warning {{conj_$}}

steakhal wrote:

```suggestion
clang_analyzer_dump(a); // expected-warning {{conj_$}}
clang_analyzer_eval(Ret > 2); // expected-warning {{FALSE}} 
expected-warning {{TRUE}} FIXME: should be only FALSE.
```
Let's take the opportunity for adding an expected (but failing) assumption 
here. May be fixed in the future.

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


[clang] [clang][analyzer] Fix argument invalidations in StreamChecker. (PR #79470)

2024-01-25 Thread Balazs Benics via cfe-commits

https://github.com/steakhal commented:

I like what you do in this patch.
I only have a couple nits. That's it.

Tomorrow, I'll check if there are any other APIs that we should test; but seems 
complete at first glance.

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


[clang] [clang][analyzer] Fix argument invalidations in StreamChecker. (PR #79470)

2024-01-25 Thread Balazs Benics via cfe-commits

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


[clang] [clang][analyzer] Fix argument invalidations in StreamChecker. (PR #79470)

2024-01-25 Thread via cfe-commits

llvmbot wrote:




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

Author: Balázs Kéri (balazske)


Changes

Specific arguments passed to stream handling functions are changed by the 
function, this means these should be invalidated ("escaped") by the analyzer. 
This change adds the argument invalidation (in specific cases) to the checker.

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


2 Files Affected:

- (modified) clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp (+35) 
- (added) clang/test/Analysis/stream-invalidate.c (+133) 


``diff
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 07727b339d967ae..166bd981a003af2 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -21,6 +21,7 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
+#include "llvm/ADT/Sequence.h"
 #include 
 #include 
 
@@ -544,6 +545,21 @@ const ExplodedNode 
*StreamChecker::getAcquisitionSite(const ExplodedNode *N,
   return nullptr;
 }
 
+static ProgramStateRef
+escapeArgs(ProgramStateRef State, CheckerContext &C, const CallEvent &Call,
+   const SmallVector &EscapingArgs) {
+  const auto *CE = Call.getOriginExpr();
+
+  SmallVector EscapingVals;
+  EscapingVals.reserve(EscapingArgs.size());
+  for (auto EscArgIdx : EscapingArgs)
+EscapingVals.push_back(Call.getArgSVal(EscArgIdx));
+  State = State->invalidateRegions(EscapingVals, CE, C.blockCount(),
+   C.getLocationContext(),
+   /*CausesPointerEscape=*/false);
+  return State;
+}
+
 
//===--===//
 // Methods of StreamChecker.
 
//===--===//
@@ -763,6 +779,11 @@ void StreamChecker::evalFreadFwrite(const FnDescription 
*Desc,
 return;
   }
 
+  // At read, invalidate the buffer in any case of error or success,
+  // except if EOF was already present.
+  if (IsFread && (OldSS->ErrorState != ErrorFEof))
+State = escapeArgs(State, C, Call, {0});
+
   // Generate a transition for the success state.
   // If we know the state to be FEOF at fread, do not add a success state.
   if (!IsFread || (OldSS->ErrorState != ErrorFEof)) {
@@ -824,6 +845,10 @@ void StreamChecker::evalFgetx(const FnDescription *Desc, 
const CallEvent &Call,
   // `fgets` returns the read buffer address on success, otherwise returns 
NULL.
 
   if (OldSS->ErrorState != ErrorFEof) {
+// If there was already EOF, assume that read buffer is not changed.
+// Otherwise it may change at success or failure.
+
+State = escapeArgs(State, C, Call, {0});
 if (SingleChar) {
   // Generate a transition for the success state of `fgetc`.
   NonLoc RetVal = makeRetVal(C, CE).castAs();
@@ -1032,6 +1057,11 @@ void StreamChecker::evalFscanf(const FnDescription 
*Desc, const CallEvent &Call,
   return;
 StateNotFailed = StateNotFailed->assume(*RetGeZero, true);
 
+SmallVector EscArgs;
+for (auto EscArg : llvm::seq(2u, Call.getNumArgs()))
+  EscArgs.push_back(EscArg);
+StateNotFailed = escapeArgs(StateNotFailed, C, Call, EscArgs);
+
 C.addTransition(StateNotFailed);
   }
 
@@ -1118,6 +1148,10 @@ void StreamChecker::evalGetdelim(const FnDescription 
*Desc,
 
   // Add transition for the successful state.
   if (OldSS->ErrorState != ErrorFEof) {
+// Escape buffer and size (may change by the call).
+// May happen even at error (partial read?).
+State = escapeArgs(State, C, Call, {0, 1});
+
 NonLoc RetVal = makeRetVal(C, CE).castAs();
 ProgramStateRef StateNotFailed =
 State->BindExpr(CE, C.getLocationContext(), RetVal);
@@ -1236,6 +1270,7 @@ void StreamChecker::evalFgetpos(const FnDescription *Desc,
   ProgramStateRef StateNotFailed, StateFailed;
   std::tie(StateFailed, StateNotFailed) =
   C.getConstraintManager().assumeDual(State, RetVal);
+  StateNotFailed = escapeArgs(StateNotFailed, C, Call, {1});
 
   // This function does not affect the stream state.
   // Still we add success and failure state with the appropriate return value.
diff --git a/clang/test/Analysis/stream-invalidate.c 
b/clang/test/Analysis/stream-invalidate.c
new file mode 100644
index 000..c5b7874f0ee8f6f
--- /dev/null
+++ b/clang/test/Analysis/stream-invalidate.c
@@ -0,0 +1,133 @@
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN: -analyzer-checker=core \
+// RUN: -analyzer-checker=alpha.unix.Stream \
+// RUN: -analyzer-checker=debug.StreamTester \
+// RUN: -analyzer-checker=debug.ExprInspection
+
+#include "Inputs/system-header-simulator.h"
+
+void clang_analyzer_eval(int);
+void clang_analyzer_dump(int);
+void clang_analyze

[clang] [clang][analyzer] Fix argument invalidations in StreamChecker. (PR #79470)

2024-01-25 Thread via cfe-commits

llvmbot wrote:




@llvm/pr-subscribers-clang

Author: Balázs Kéri (balazske)


Changes

Specific arguments passed to stream handling functions are changed by the 
function, this means these should be invalidated ("escaped") by the analyzer. 
This change adds the argument invalidation (in specific cases) to the checker.

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


2 Files Affected:

- (modified) clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp (+35) 
- (added) clang/test/Analysis/stream-invalidate.c (+133) 


``diff
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 07727b339d967ae..166bd981a003af2 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -21,6 +21,7 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
+#include "llvm/ADT/Sequence.h"
 #include 
 #include 
 
@@ -544,6 +545,21 @@ const ExplodedNode 
*StreamChecker::getAcquisitionSite(const ExplodedNode *N,
   return nullptr;
 }
 
+static ProgramStateRef
+escapeArgs(ProgramStateRef State, CheckerContext &C, const CallEvent &Call,
+   const SmallVector &EscapingArgs) {
+  const auto *CE = Call.getOriginExpr();
+
+  SmallVector EscapingVals;
+  EscapingVals.reserve(EscapingArgs.size());
+  for (auto EscArgIdx : EscapingArgs)
+EscapingVals.push_back(Call.getArgSVal(EscArgIdx));
+  State = State->invalidateRegions(EscapingVals, CE, C.blockCount(),
+   C.getLocationContext(),
+   /*CausesPointerEscape=*/false);
+  return State;
+}
+
 
//===--===//
 // Methods of StreamChecker.
 
//===--===//
@@ -763,6 +779,11 @@ void StreamChecker::evalFreadFwrite(const FnDescription 
*Desc,
 return;
   }
 
+  // At read, invalidate the buffer in any case of error or success,
+  // except if EOF was already present.
+  if (IsFread && (OldSS->ErrorState != ErrorFEof))
+State = escapeArgs(State, C, Call, {0});
+
   // Generate a transition for the success state.
   // If we know the state to be FEOF at fread, do not add a success state.
   if (!IsFread || (OldSS->ErrorState != ErrorFEof)) {
@@ -824,6 +845,10 @@ void StreamChecker::evalFgetx(const FnDescription *Desc, 
const CallEvent &Call,
   // `fgets` returns the read buffer address on success, otherwise returns 
NULL.
 
   if (OldSS->ErrorState != ErrorFEof) {
+// If there was already EOF, assume that read buffer is not changed.
+// Otherwise it may change at success or failure.
+
+State = escapeArgs(State, C, Call, {0});
 if (SingleChar) {
   // Generate a transition for the success state of `fgetc`.
   NonLoc RetVal = makeRetVal(C, CE).castAs();
@@ -1032,6 +1057,11 @@ void StreamChecker::evalFscanf(const FnDescription 
*Desc, const CallEvent &Call,
   return;
 StateNotFailed = StateNotFailed->assume(*RetGeZero, true);
 
+SmallVector EscArgs;
+for (auto EscArg : llvm::seq(2u, Call.getNumArgs()))
+  EscArgs.push_back(EscArg);
+StateNotFailed = escapeArgs(StateNotFailed, C, Call, EscArgs);
+
 C.addTransition(StateNotFailed);
   }
 
@@ -1118,6 +1148,10 @@ void StreamChecker::evalGetdelim(const FnDescription 
*Desc,
 
   // Add transition for the successful state.
   if (OldSS->ErrorState != ErrorFEof) {
+// Escape buffer and size (may change by the call).
+// May happen even at error (partial read?).
+State = escapeArgs(State, C, Call, {0, 1});
+
 NonLoc RetVal = makeRetVal(C, CE).castAs();
 ProgramStateRef StateNotFailed =
 State->BindExpr(CE, C.getLocationContext(), RetVal);
@@ -1236,6 +1270,7 @@ void StreamChecker::evalFgetpos(const FnDescription *Desc,
   ProgramStateRef StateNotFailed, StateFailed;
   std::tie(StateFailed, StateNotFailed) =
   C.getConstraintManager().assumeDual(State, RetVal);
+  StateNotFailed = escapeArgs(StateNotFailed, C, Call, {1});
 
   // This function does not affect the stream state.
   // Still we add success and failure state with the appropriate return value.
diff --git a/clang/test/Analysis/stream-invalidate.c 
b/clang/test/Analysis/stream-invalidate.c
new file mode 100644
index 000..c5b7874f0ee8f6f
--- /dev/null
+++ b/clang/test/Analysis/stream-invalidate.c
@@ -0,0 +1,133 @@
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN: -analyzer-checker=core \
+// RUN: -analyzer-checker=alpha.unix.Stream \
+// RUN: -analyzer-checker=debug.StreamTester \
+// RUN: -analyzer-checker=debug.ExprInspection
+
+#include "Inputs/system-header-simulator.h"
+
+void clang_analyzer_eval(int);
+void clang_analyzer_dump(int);
+void clang_analyzer_warnIfReached(vo

[clang] [clang][analyzer] Fix argument invalidations in StreamChecker. (PR #79470)

2024-01-25 Thread Balázs Kéri via cfe-commits

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

Specific arguments passed to stream handling functions are changed by the 
function, this means these should be invalidated ("escaped") by the analyzer. 
This change adds the argument invalidation (in specific cases) to the checker.

From dbd9af4e77c34fcf6ce82f226f7dbf836033a8f6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= 
Date: Thu, 25 Jan 2024 17:50:42 +0100
Subject: [PATCH] [clang][analyzer] Fix argument invalidations in
 StreamChecker.

Specific arguments passed to stream handling functions are changed by the 
function,
this means these should be invalidated ("escaped") by the analyzer.
This change adds the argument invalidation (in specific cases) to the checker.
---
 .../StaticAnalyzer/Checkers/StreamChecker.cpp |  35 +
 clang/test/Analysis/stream-invalidate.c   | 133 ++
 2 files changed, 168 insertions(+)
 create mode 100644 clang/test/Analysis/stream-invalidate.c

diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 07727b339d967ae..166bd981a003af2 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -21,6 +21,7 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
+#include "llvm/ADT/Sequence.h"
 #include 
 #include 
 
@@ -544,6 +545,21 @@ const ExplodedNode 
*StreamChecker::getAcquisitionSite(const ExplodedNode *N,
   return nullptr;
 }
 
+static ProgramStateRef
+escapeArgs(ProgramStateRef State, CheckerContext &C, const CallEvent &Call,
+   const SmallVector &EscapingArgs) {
+  const auto *CE = Call.getOriginExpr();
+
+  SmallVector EscapingVals;
+  EscapingVals.reserve(EscapingArgs.size());
+  for (auto EscArgIdx : EscapingArgs)
+EscapingVals.push_back(Call.getArgSVal(EscArgIdx));
+  State = State->invalidateRegions(EscapingVals, CE, C.blockCount(),
+   C.getLocationContext(),
+   /*CausesPointerEscape=*/false);
+  return State;
+}
+
 
//===--===//
 // Methods of StreamChecker.
 
//===--===//
@@ -763,6 +779,11 @@ void StreamChecker::evalFreadFwrite(const FnDescription 
*Desc,
 return;
   }
 
+  // At read, invalidate the buffer in any case of error or success,
+  // except if EOF was already present.
+  if (IsFread && (OldSS->ErrorState != ErrorFEof))
+State = escapeArgs(State, C, Call, {0});
+
   // Generate a transition for the success state.
   // If we know the state to be FEOF at fread, do not add a success state.
   if (!IsFread || (OldSS->ErrorState != ErrorFEof)) {
@@ -824,6 +845,10 @@ void StreamChecker::evalFgetx(const FnDescription *Desc, 
const CallEvent &Call,
   // `fgets` returns the read buffer address on success, otherwise returns 
NULL.
 
   if (OldSS->ErrorState != ErrorFEof) {
+// If there was already EOF, assume that read buffer is not changed.
+// Otherwise it may change at success or failure.
+
+State = escapeArgs(State, C, Call, {0});
 if (SingleChar) {
   // Generate a transition for the success state of `fgetc`.
   NonLoc RetVal = makeRetVal(C, CE).castAs();
@@ -1032,6 +1057,11 @@ void StreamChecker::evalFscanf(const FnDescription 
*Desc, const CallEvent &Call,
   return;
 StateNotFailed = StateNotFailed->assume(*RetGeZero, true);
 
+SmallVector EscArgs;
+for (auto EscArg : llvm::seq(2u, Call.getNumArgs()))
+  EscArgs.push_back(EscArg);
+StateNotFailed = escapeArgs(StateNotFailed, C, Call, EscArgs);
+
 C.addTransition(StateNotFailed);
   }
 
@@ -1118,6 +1148,10 @@ void StreamChecker::evalGetdelim(const FnDescription 
*Desc,
 
   // Add transition for the successful state.
   if (OldSS->ErrorState != ErrorFEof) {
+// Escape buffer and size (may change by the call).
+// May happen even at error (partial read?).
+State = escapeArgs(State, C, Call, {0, 1});
+
 NonLoc RetVal = makeRetVal(C, CE).castAs();
 ProgramStateRef StateNotFailed =
 State->BindExpr(CE, C.getLocationContext(), RetVal);
@@ -1236,6 +1270,7 @@ void StreamChecker::evalFgetpos(const FnDescription *Desc,
   ProgramStateRef StateNotFailed, StateFailed;
   std::tie(StateFailed, StateNotFailed) =
   C.getConstraintManager().assumeDual(State, RetVal);
+  StateNotFailed = escapeArgs(StateNotFailed, C, Call, {1});
 
   // This function does not affect the stream state.
   // Still we add success and failure state with the appropriate return value.
diff --git a/clang/test/Analysis/stream-invalidate.c 
b/clang/test/Analysis/stream-invalidate.c
new file mode 100644
index 000..c5b