[clang] [clang][analyzer] Change modeling of 'fileno' in checkers. (PR #81842)

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

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

[clang] [clang][analyzer] Change modeling of 'fileno' in checkers. (PR #81842)

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

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

From 0f836d8e63f462f57784e62bcd87ac1f4f5a3d00 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= 
Date: Thu, 15 Feb 2024 10:56:32 +0100
Subject: [PATCH 1/3] [clang][analyzer] Change modeling of 'fileno' in
 checkers.

Function 'fileno' fails only if invalid pointer is passed, this is a
case that is often ignored in source code. The failure case leads to
many "false positive" (theoretically correct bug normally "should not
happen" cases) reports. Because this, the function is now assumed to
not fail. The change affects `StdCLibraryFunctionsChecker` and
`StreamChecker`.
---
 .../Checkers/StdLibraryFunctionsChecker.cpp   |   9 +-
 .../StaticAnalyzer/Checkers/StreamChecker.cpp | 203 +++---
 .../std-c-library-functions-path-notes.c  |  22 +-
 clang/test/Analysis/stream-errno-note.c   |  12 +-
 clang/test/Analysis/stream-errno.c|  16 +-
 clang/test/Analysis/stream-error.c|  18 ++
 clang/test/Analysis/stream-noopen.c   |  10 +
 7 files changed, 172 insertions(+), 118 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
index 6b8ac2629453d4..6cc88679458140 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -2388,12 +2388,15 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
 .ArgConstraint(NotNull(ArgNo(0;
 
 // int fileno(FILE *stream);
+// According to POSIX 'fileno' may fail and set 'errno'.
+// But in Linux it may fail only if the specified file pointer is invalid.
+// At many places 'fileno' is used without check for failure and a failure
+// case here would produce a large amount of likely false positive 
warnings.
+// To avoid this, we assume here that it does not fail.
 addToFunctionSummaryMap(
 "fileno", Signature(ArgTypes{FilePtrTy}, RetType{IntTy}),
 Summary(NoEvalCall)
-.Case(ReturnsValidFileDescriptor, ErrnoMustNotBeChecked,
-  GenericSuccessMsg)
-.Case(ReturnsMinusOne, ErrnoNEZeroIrrelevant, GenericFailureMsg)
+.Case(ReturnsValidFileDescriptor, ErrnoUnchanged, 
GenericSuccessMsg)
 .ArgConstraint(NotNull(ArgNo(0;
 
 // void rewind(FILE *stream);
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 07727b339d967a..4dda09c4817d7b 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -304,7 +304,8 @@ class StreamChecker : public Checker FnTestDescriptions = {
@@ -400,6 +401,9 @@ class StreamChecker : public Checker Stream = StreamVal.getAs();
+  if (!Stream)
+return;
+
+  ProgramStateRef StateNotNull, StateNull;
+  std::tie(StateNotNull, StateNull) =
+  C.getConstraintManager().assumeDual(State, *Stream);
+  if (StateNotNull && !StateNull)
+ensureStreamOpened(StreamVal, C, StateNotNull);
+}
+
+void StreamChecker::evalFflush(const FnDescription *Desc, const CallEvent 
,
+   CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+  SVal StreamVal = getStreamArg(Desc, Call);
+  std::optional Stream = StreamVal.getAs();
+  if (!Stream)
+return;
+
+  // Skip if the stream can be both NULL and non-NULL.
+  ProgramStateRef StateNotNull, StateNull;
+  std::tie(StateNotNull, StateNull) =
+  C.getConstraintManager().assumeDual(State, *Stream);
+  if (StateNotNull && StateNull)
+return;
+  if (StateNotNull && !StateNull)
+State = StateNotNull;
+  else
+State = StateNull;
+
+  const CallExpr *CE = dyn_cast_or_null(Call.getOriginExpr());
+  if (!CE)
+return;
+
+  // `fflush` returns EOF on failure, otherwise returns 0.
+  ProgramStateRef StateFailed = bindInt(*EofVal, State, C, CE);
+  ProgramStateRef StateNotFailed = bindInt(0, State, C, CE);
+
+  // Clear error states if `fflush` returns 0, but retain their EOF flags.
+  auto ClearErrorInNotFailed = [, Desc](SymbolRef Sym,
+   const StreamState *SS) {
+if (SS->ErrorState & ErrorFError) {
+  StreamErrorState NewES =
+  (SS->ErrorState & ErrorFEof) ? ErrorFEof : ErrorNone;
+  StreamState NewSS = StreamState::getOpened(Desc, NewES, false);
+  StateNotFailed = StateNotFailed->set(Sym, NewSS);
+}
+  };
+
+  if (StateNotNull && !StateNull) {
+// Skip if the input stream's state is unknown, open-failed or closed.
+if (SymbolRef StreamSym = StreamVal.getAsSymbol()) {
+  const StreamState *SS = State->get(StreamSym);
+  if (SS) {
+assert(SS->isOpened() && "Stream is expected to be opened");
+ClearErrorInNotFailed(StreamSym, SS);
+  } else
+

[clang] [clang][analyzer] Change modeling of 'fileno' in checkers. (PR #81842)

2024-02-20 Thread Ben Shi via cfe-commits 
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 



@@ -1404,6 +1486,47 @@ void StreamChecker::evalFeofFerror(const FnDescription 
*Desc,
   }
 }
 
+void StreamChecker::evalFileno(const FnDescription *Desc, const CallEvent 
,
+   CheckerContext ) const {
+  // Fileno should fail only if the passed pointer is invalid.
+  // Some of the preconditions are checked already in preDefault.
+  // Here we can assume that the operation does not fail.
+  // An added failure case causes many unexpected warnings because a file 
number
+  // becomes -1 that is not expected by the program.
+  // The stream error states are not modified by 'fileno', and not the 'errno'.
+  // (To ensure that errno is not changed, this evalCall is needed to not
+  // invalidate 'errno' like in a default case.)
+  ProgramStateRef State = C.getState();
+  SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol();
+  if (!StreamSym)
+return;
+
+  const CallExpr *CE = dyn_cast_or_null(Call.getOriginExpr());

benshi001 wrote:

Shall we change to `StreamOperationEvaluator` which is introduced by your 
previous simplification patch ?

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

[clang] [clang][analyzer] Change modeling of 'fileno' in checkers. (PR #81842)

2024-02-20 Thread Ben Shi via cfe-commits 
=?utf-8?q?Bal=C3=A1zs_K=C3=A9ri?= 
Message-ID:
In-Reply-To: 



@@ -268,6 +268,16 @@ void test_clearerr(FILE *F) {
// expected-warning@-1{{FALSE}}
 }
 
+void test_fileno(FILE *F) {
+  errno = 0;
+  int A = fileno(F);
+  clang_analyzer_eval(F != NULL); // expected-warning{{TRUE}}
+  clang_analyzer_eval(A >= 0); // expected-warning{{TRUE}}

benshi001 wrote:

It looks better to making the comment lines begin from the same column.

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

[clang] [clang][analyzer] Change modeling of 'fileno' in checkers. (PR #81842)

2024-02-20 Thread Ben Shi via cfe-commits 
=?utf-8?q?Bal=C3=A1zs_K=C3=A9ri?= 
Message-ID:
In-Reply-To: 


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


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

[clang] [clang][analyzer] Change modeling of 'fileno' in checkers. (PR #81842)

2024-02-20 Thread Ben Shi via cfe-commits 
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 


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

[clang] [clang][analyzer] Change modeling of 'fileno' in checkers. (PR #81842)

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


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

[clang] [clang][analyzer] Change modeling of 'fileno' in checkers. (PR #81842)

2024-02-20 Thread via cfe-commits 
=?utf-8?q?Bal=C3=A1zs_K=C3=A9ri?= 
Message-ID:
In-Reply-To: 


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

LGTM, thanks for the update!

The new `getZeroVal()` is a nice simplification.

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

[clang] [clang][analyzer] Change modeling of 'fileno' in checkers. (PR #81842)

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

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

From 0f836d8e63f462f57784e62bcd87ac1f4f5a3d00 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= 
Date: Thu, 15 Feb 2024 10:56:32 +0100
Subject: [PATCH 1/2] [clang][analyzer] Change modeling of 'fileno' in
 checkers.

Function 'fileno' fails only if invalid pointer is passed, this is a
case that is often ignored in source code. The failure case leads to
many "false positive" (theoretically correct bug normally "should not
happen" cases) reports. Because this, the function is now assumed to
not fail. The change affects `StdCLibraryFunctionsChecker` and
`StreamChecker`.
---
 .../Checkers/StdLibraryFunctionsChecker.cpp   |   9 +-
 .../StaticAnalyzer/Checkers/StreamChecker.cpp | 203 +++---
 .../std-c-library-functions-path-notes.c  |  22 +-
 clang/test/Analysis/stream-errno-note.c   |  12 +-
 clang/test/Analysis/stream-errno.c|  16 +-
 clang/test/Analysis/stream-error.c|  18 ++
 clang/test/Analysis/stream-noopen.c   |  10 +
 7 files changed, 172 insertions(+), 118 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
index 6b8ac2629453d4..6cc88679458140 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -2388,12 +2388,15 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
 .ArgConstraint(NotNull(ArgNo(0;
 
 // int fileno(FILE *stream);
+// According to POSIX 'fileno' may fail and set 'errno'.
+// But in Linux it may fail only if the specified file pointer is invalid.
+// At many places 'fileno' is used without check for failure and a failure
+// case here would produce a large amount of likely false positive 
warnings.
+// To avoid this, we assume here that it does not fail.
 addToFunctionSummaryMap(
 "fileno", Signature(ArgTypes{FilePtrTy}, RetType{IntTy}),
 Summary(NoEvalCall)
-.Case(ReturnsValidFileDescriptor, ErrnoMustNotBeChecked,
-  GenericSuccessMsg)
-.Case(ReturnsMinusOne, ErrnoNEZeroIrrelevant, GenericFailureMsg)
+.Case(ReturnsValidFileDescriptor, ErrnoUnchanged, 
GenericSuccessMsg)
 .ArgConstraint(NotNull(ArgNo(0;
 
 // void rewind(FILE *stream);
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 07727b339d967a..4dda09c4817d7b 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -304,7 +304,8 @@ class StreamChecker : public Checker FnTestDescriptions = {
@@ -400,6 +401,9 @@ class StreamChecker : public Checker Stream = StreamVal.getAs();
+  if (!Stream)
+return;
+
+  ProgramStateRef StateNotNull, StateNull;
+  std::tie(StateNotNull, StateNull) =
+  C.getConstraintManager().assumeDual(State, *Stream);
+  if (StateNotNull && !StateNull)
+ensureStreamOpened(StreamVal, C, StateNotNull);
+}
+
+void StreamChecker::evalFflush(const FnDescription *Desc, const CallEvent 
,
+   CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+  SVal StreamVal = getStreamArg(Desc, Call);
+  std::optional Stream = StreamVal.getAs();
+  if (!Stream)
+return;
+
+  // Skip if the stream can be both NULL and non-NULL.
+  ProgramStateRef StateNotNull, StateNull;
+  std::tie(StateNotNull, StateNull) =
+  C.getConstraintManager().assumeDual(State, *Stream);
+  if (StateNotNull && StateNull)
+return;
+  if (StateNotNull && !StateNull)
+State = StateNotNull;
+  else
+State = StateNull;
+
+  const CallExpr *CE = dyn_cast_or_null(Call.getOriginExpr());
+  if (!CE)
+return;
+
+  // `fflush` returns EOF on failure, otherwise returns 0.
+  ProgramStateRef StateFailed = bindInt(*EofVal, State, C, CE);
+  ProgramStateRef StateNotFailed = bindInt(0, State, C, CE);
+
+  // Clear error states if `fflush` returns 0, but retain their EOF flags.
+  auto ClearErrorInNotFailed = [, Desc](SymbolRef Sym,
+   const StreamState *SS) {
+if (SS->ErrorState & ErrorFError) {
+  StreamErrorState NewES =
+  (SS->ErrorState & ErrorFEof) ? ErrorFEof : ErrorNone;
+  StreamState NewSS = StreamState::getOpened(Desc, NewES, false);
+  StateNotFailed = StateNotFailed->set(Sym, NewSS);
+}
+  };
+
+  if (StateNotNull && !StateNull) {
+// Skip if the input stream's state is unknown, open-failed or closed.
+if (SymbolRef StreamSym = StreamVal.getAsSymbol()) {
+  const StreamState *SS = State->get(StreamSym);
+  if (SS) {
+assert(SS->isOpened() && "Stream is expected to be opened");
+ClearErrorInNotFailed(StreamSym, SS);
+  } else
+

[clang] [clang][analyzer] Change modeling of 'fileno' in checkers. (PR #81842)

2024-02-16 Thread via cfe-commits 


@@ -1404,6 +1486,47 @@ void StreamChecker::evalFeofFerror(const FnDescription 
*Desc,
   }
 }
 
+void StreamChecker::evalFileno(const FnDescription *Desc, const CallEvent 
,
+   CheckerContext ) const {
+  // Fileno should fail only if the passed pointer is invalid.
+  // Some of the preconditions are checked already in preDefault.
+  // Here we can assume that the operation does not fail.
+  // An added failure case causes many unexpected warnings because a file 
number
+  // becomes -1 that is not expected by the program.
+  // The stream error states are not modified by 'fileno', and not the 'errno'.
+  // (To ensure that errno is not changed, this evalCall is needed to not
+  // invalidate 'errno' like in a default case.)

NagyDonat wrote:

```suggestion
  // Here we assume that the operation does not fail, because we introduced a
  // separate branch where fileno() returns -1, then it would cause many
  // unexpected and unwanted warnings in situations where fileno() is called
  // on vaild streams.
  // The stream error states are not modified by 'fileno', and 'errno' is also
  // left unchanged (so this evalCall does not invalidate it).
```

I felt that this comment is a bit difficult to understand and composed a 
reworded alternative. Of course, this is a very subjective matter and English 
isn't my first language, so feel free to bikeshed this and/or override my 
suggestions.

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

[clang] [clang][analyzer] Change modeling of 'fileno' in checkers. (PR #81842)

2024-02-16 Thread via cfe-commits 


@@ -1404,6 +1486,47 @@ void StreamChecker::evalFeofFerror(const FnDescription 
*Desc,
   }
 }
 
+void StreamChecker::evalFileno(const FnDescription *Desc, const CallEvent 
,
+   CheckerContext ) const {
+  // Fileno should fail only if the passed pointer is invalid.
+  // Some of the preconditions are checked already in preDefault.
+  // Here we can assume that the operation does not fail.
+  // An added failure case causes many unexpected warnings because a file 
number
+  // becomes -1 that is not expected by the program.
+  // The stream error states are not modified by 'fileno', and not the 'errno'.
+  // (To ensure that errno is not changed, this evalCall is needed to not
+  // invalidate 'errno' like in a default case.)
+  ProgramStateRef State = C.getState();
+  SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol();
+  if (!StreamSym)
+return;
+
+  const CallExpr *CE = dyn_cast_or_null(Call.getOriginExpr());
+  if (!CE)
+return;
+
+  const StreamState *SS = State->get(StreamSym);
+  if (!SS)
+return;
+
+  assertStreamStateOpened(SS);
+
+  SValBuilder  = C.getSValBuilder();
+  NonLoc RetVal = makeRetVal(C, CE).castAs();
+  State = State->BindExpr(CE, C.getLocationContext(), RetVal);
+  auto Cond =
+  SVB.evalBinOp(State, BO_GE, RetVal, 
SVB.makeZeroVal(Call.getResultType()),

NagyDonat wrote:

It's a nice trick that you're creating the zero in the type of the other side 
to avoid surprising type conversion.

There's a chance that I can borrow this solution to handle tricky issues in 
ArrayBoundCheckerV2 :smile: 

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

[clang] [clang][analyzer] Change modeling of 'fileno' in checkers. (PR #81842)

2024-02-16 Thread via cfe-commits 

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

LGTM with some low priority bikeshedding in a comment.

By the way it would be good to rebase this change onto your other commit #79312 
(that was merged a few hours ago) and use the "toolbox" introduced there if 
that's possible.

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

[clang] [clang][analyzer] Change modeling of 'fileno' in checkers. (PR #81842)

2024-02-16 Thread via cfe-commits 

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

[clang] [clang][analyzer] Change modeling of 'fileno' in checkers. (PR #81842)

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

balazske wrote:

Additionally code of `fflush` is moved in `StreamChecker` to a better place.

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

[clang] [clang][analyzer] Change modeling of 'fileno' in checkers. (PR #81842)

2024-02-15 Thread via cfe-commits 

llvmbot wrote:



@llvm/pr-subscribers-clang

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

Author: Balázs Kéri (balazske)


Changes

Function 'fileno' fails only if invalid pointer is passed, this is a case that 
is often ignored in source code. The failure case leads to many "false 
positive" (theoretically correct bug normally "should not happen" cases) 
reports. Because this, the function is now assumed to not fail. The change 
affects `StdCLibraryFunctionsChecker` and `StreamChecker`.

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


7 Files Affected:

- (modified) clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp 
(+6-3) 
- (modified) clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp (+124-79) 
- (modified) clang/test/Analysis/std-c-library-functions-path-notes.c (+10-12) 
- (modified) clang/test/Analysis/stream-errno-note.c (+2-10) 
- (modified) clang/test/Analysis/stream-errno.c (+2-14) 
- (modified) clang/test/Analysis/stream-error.c (+18) 
- (modified) clang/test/Analysis/stream-noopen.c (+10) 


``diff
diff --git a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
index 6b8ac2629453d4..6cc88679458140 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -2388,12 +2388,15 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
 .ArgConstraint(NotNull(ArgNo(0;
 
 // int fileno(FILE *stream);
+// According to POSIX 'fileno' may fail and set 'errno'.
+// But in Linux it may fail only if the specified file pointer is invalid.
+// At many places 'fileno' is used without check for failure and a failure
+// case here would produce a large amount of likely false positive 
warnings.
+// To avoid this, we assume here that it does not fail.
 addToFunctionSummaryMap(
 "fileno", Signature(ArgTypes{FilePtrTy}, RetType{IntTy}),
 Summary(NoEvalCall)
-.Case(ReturnsValidFileDescriptor, ErrnoMustNotBeChecked,
-  GenericSuccessMsg)
-.Case(ReturnsMinusOne, ErrnoNEZeroIrrelevant, GenericFailureMsg)
+.Case(ReturnsValidFileDescriptor, ErrnoUnchanged, 
GenericSuccessMsg)
 .ArgConstraint(NotNull(ArgNo(0;
 
 // void rewind(FILE *stream);
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 07727b339d967a..4dda09c4817d7b 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -304,7 +304,8 @@ class StreamChecker : public Checker FnTestDescriptions = {
@@ -400,6 +401,9 @@ class StreamChecker : public Checker Stream = StreamVal.getAs();
+  if (!Stream)
+return;
+
+  ProgramStateRef StateNotNull, StateNull;
+  std::tie(StateNotNull, StateNull) =
+  C.getConstraintManager().assumeDual(State, *Stream);
+  if (StateNotNull && !StateNull)
+ensureStreamOpened(StreamVal, C, StateNotNull);
+}
+
+void StreamChecker::evalFflush(const FnDescription *Desc, const CallEvent 
,
+   CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+  SVal StreamVal = getStreamArg(Desc, Call);
+  std::optional Stream = StreamVal.getAs();
+  if (!Stream)
+return;
+
+  // Skip if the stream can be both NULL and non-NULL.
+  ProgramStateRef StateNotNull, StateNull;
+  std::tie(StateNotNull, StateNull) =
+  C.getConstraintManager().assumeDual(State, *Stream);
+  if (StateNotNull && StateNull)
+return;
+  if (StateNotNull && !StateNull)
+State = StateNotNull;
+  else
+State = StateNull;
+
+  const CallExpr *CE = dyn_cast_or_null(Call.getOriginExpr());
+  if (!CE)
+return;
+
+  // `fflush` returns EOF on failure, otherwise returns 0.
+  ProgramStateRef StateFailed = bindInt(*EofVal, State, C, CE);
+  ProgramStateRef StateNotFailed = bindInt(0, State, C, CE);
+
+  // Clear error states if `fflush` returns 0, but retain their EOF flags.
+  auto ClearErrorInNotFailed = [, Desc](SymbolRef Sym,
+   const StreamState *SS) {
+if (SS->ErrorState & ErrorFError) {
+  StreamErrorState NewES =
+  (SS->ErrorState & ErrorFEof) ? ErrorFEof : ErrorNone;
+  StreamState NewSS = StreamState::getOpened(Desc, NewES, false);
+  StateNotFailed = StateNotFailed->set(Sym, NewSS);
+}
+  };
+
+  if (StateNotNull && !StateNull) {
+// Skip if the input stream's state is unknown, open-failed or closed.
+if (SymbolRef StreamSym = StreamVal.getAsSymbol()) {
+  const StreamState *SS = State->get(StreamSym);
+  if (SS) {
+assert(SS->isOpened() && "Stream is expected to be opened");
+ClearErrorInNotFailed(StreamSym, SS);
+  } else
+return;
+}
+  } else {
+// Clear error states for all streams.

[clang] [clang][analyzer] Change modeling of 'fileno' in checkers. (PR #81842)

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

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

Function 'fileno' fails only if invalid pointer is passed, this is a case that 
is often ignored in source code. The failure case leads to many "false 
positive" (theoretically correct bug normally "should not happen" cases) 
reports. Because this, the function is now assumed to not fail. The change 
affects `StdCLibraryFunctionsChecker` and `StreamChecker`.

From 0f836d8e63f462f57784e62bcd87ac1f4f5a3d00 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= 
Date: Thu, 15 Feb 2024 10:56:32 +0100
Subject: [PATCH] [clang][analyzer] Change modeling of 'fileno' in checkers.

Function 'fileno' fails only if invalid pointer is passed, this is a
case that is often ignored in source code. The failure case leads to
many "false positive" (theoretically correct bug normally "should not
happen" cases) reports. Because this, the function is now assumed to
not fail. The change affects `StdCLibraryFunctionsChecker` and
`StreamChecker`.
---
 .../Checkers/StdLibraryFunctionsChecker.cpp   |   9 +-
 .../StaticAnalyzer/Checkers/StreamChecker.cpp | 203 +++---
 .../std-c-library-functions-path-notes.c  |  22 +-
 clang/test/Analysis/stream-errno-note.c   |  12 +-
 clang/test/Analysis/stream-errno.c|  16 +-
 clang/test/Analysis/stream-error.c|  18 ++
 clang/test/Analysis/stream-noopen.c   |  10 +
 7 files changed, 172 insertions(+), 118 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
index 6b8ac2629453d4..6cc88679458140 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -2388,12 +2388,15 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
 .ArgConstraint(NotNull(ArgNo(0;
 
 // int fileno(FILE *stream);
+// According to POSIX 'fileno' may fail and set 'errno'.
+// But in Linux it may fail only if the specified file pointer is invalid.
+// At many places 'fileno' is used without check for failure and a failure
+// case here would produce a large amount of likely false positive 
warnings.
+// To avoid this, we assume here that it does not fail.
 addToFunctionSummaryMap(
 "fileno", Signature(ArgTypes{FilePtrTy}, RetType{IntTy}),
 Summary(NoEvalCall)
-.Case(ReturnsValidFileDescriptor, ErrnoMustNotBeChecked,
-  GenericSuccessMsg)
-.Case(ReturnsMinusOne, ErrnoNEZeroIrrelevant, GenericFailureMsg)
+.Case(ReturnsValidFileDescriptor, ErrnoUnchanged, 
GenericSuccessMsg)
 .ArgConstraint(NotNull(ArgNo(0;
 
 // void rewind(FILE *stream);
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 07727b339d967a..4dda09c4817d7b 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -304,7 +304,8 @@ class StreamChecker : public Checker FnTestDescriptions = {
@@ -400,6 +401,9 @@ class StreamChecker : public Checker Stream = StreamVal.getAs();
+  if (!Stream)
+return;
+
+  ProgramStateRef StateNotNull, StateNull;
+  std::tie(StateNotNull, StateNull) =
+  C.getConstraintManager().assumeDual(State, *Stream);
+  if (StateNotNull && !StateNull)
+ensureStreamOpened(StreamVal, C, StateNotNull);
+}
+
+void StreamChecker::evalFflush(const FnDescription *Desc, const CallEvent 
,
+   CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+  SVal StreamVal = getStreamArg(Desc, Call);
+  std::optional Stream = StreamVal.getAs();
+  if (!Stream)
+return;
+
+  // Skip if the stream can be both NULL and non-NULL.
+  ProgramStateRef StateNotNull, StateNull;
+  std::tie(StateNotNull, StateNull) =
+  C.getConstraintManager().assumeDual(State, *Stream);
+  if (StateNotNull && StateNull)
+return;
+  if (StateNotNull && !StateNull)
+State = StateNotNull;
+  else
+State = StateNull;
+
+  const CallExpr *CE = dyn_cast_or_null(Call.getOriginExpr());
+  if (!CE)
+return;
+
+  // `fflush` returns EOF on failure, otherwise returns 0.
+  ProgramStateRef StateFailed = bindInt(*EofVal, State, C, CE);
+  ProgramStateRef StateNotFailed = bindInt(0, State, C, CE);
+
+  // Clear error states if `fflush` returns 0, but retain their EOF flags.
+  auto ClearErrorInNotFailed = [, Desc](SymbolRef Sym,
+   const StreamState *SS) {
+if (SS->ErrorState & ErrorFError) {
+  StreamErrorState NewES =
+  (SS->ErrorState & ErrorFEof) ? ErrorFEof : ErrorNone;
+  StreamState NewSS = StreamState::getOpened(Desc, NewES, false);
+  StateNotFailed = StateNotFailed->set(Sym, NewSS);
+}
+  };
+
+  if (StateNotNull &&