[clang] [analyzer] Fix crash of StreamChecker when eval calling 'fopen' (PR #100990)
Balazs Benics via cfe-commits
cfe-commits at lists.llvm.org
Mon Jul 29 02:55:01 PDT 2024
https://github.com/steakhal updated https://github.com/llvm/llvm-project/pull/100990
>From 12ff3274fce8dfe534e71abb9511d6549e32f74c Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Mon, 29 Jul 2024 10:33:55 +0200
Subject: [PATCH 1/6] [analyzer] Fix crash of StreamChecker when eval calling
'fopen'
Actually, on the failure branch of `fopen`, the resulting pointer could
alias with `stdout` iff `stdout` is already known to be null.
We crashed in this case as the implementation assumed that the
state-split for creating the success and failure branches both should be
viable; thus dereferenced both of those states - leading to the crash.
To fix this, let's just only add this no-alias property for the success
path, and that's it :)
Fixes #100901
---
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp | 3 ++-
clang/test/Analysis/stream.c | 10 +++++++++-
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 53770532609d5..942e7541a83cd 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -938,7 +938,6 @@ void StreamChecker::evalFopen(const FnDescription *Desc, const CallEvent &Call,
assert(RetSym && "RetVal must be a symbol here.");
State = State->BindExpr(CE, C.getLocationContext(), RetVal);
- State = assumeNoAliasingWithStdStreams(State, RetVal, C);
// Bifurcate the state into two: one with a valid FILE* pointer, the other
// with a NULL.
@@ -951,6 +950,8 @@ void StreamChecker::evalFopen(const FnDescription *Desc, const CallEvent &Call,
StateNull =
StateNull->set<StreamMap>(RetSym, StreamState::getOpenFailed(Desc));
+ StateNotNull = assumeNoAliasingWithStdStreams(StateNotNull, RetVal, C);
+
C.addTransition(StateNotNull,
constructLeakNoteTag(C, RetSym, "Stream opened here"));
C.addTransition(StateNull);
diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c
index b3a47ce4153d3..575b32d97be31 100644
--- a/clang/test/Analysis/stream.c
+++ b/clang/test/Analysis/stream.c
@@ -502,11 +502,19 @@ void gh_93408_regression_ZeroSized(struct ZeroSized *buffer) {
extern FILE *stdout_like_ptr;
void no_aliasing(void) {
FILE *f = fopen("file", "r");
+ if (!f) return;
clang_analyzer_eval(f == stdin); // expected-warning {{FALSE}} no-TRUE
clang_analyzer_eval(f == stdout); // expected-warning {{FALSE}} no-TRUE
clang_analyzer_eval(f == stderr); // expected-warning {{FALSE}} no-TRUE
clang_analyzer_eval(f == stdout_like_ptr); // expected-warning {{FALSE}} expected-warning {{TRUE}}
- if (f && f != stdout) {
+ if (f != stdout) {
fclose(f);
}
} // no-leak: 'fclose()' is always called because 'f' cannot be 'stdout'.
+
+void only_success_path_does_not_alias_with_stdout(void) {
+ if (stdout) return;
+ FILE *f = fopen("/tmp/foof", "r"); // no-crash
+ if (!f) return;
+ fclose(f);
+}
>From 7453968ac1764adbc70f867d43fa3d63473b09f3 Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Mon, 29 Jul 2024 10:39:03 +0200
Subject: [PATCH 2/6] NFC Fixup previous PR comment
Addresses:
https://github.com/llvm/llvm-project/pull/100085#discussion_r1688051166
---
clang/test/Analysis/stream.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c
index 575b32d97be31..aff884f190c8e 100644
--- a/clang/test/Analysis/stream.c
+++ b/clang/test/Analysis/stream.c
@@ -499,14 +499,14 @@ void gh_93408_regression_ZeroSized(struct ZeroSized *buffer) {
fclose(f);
}
-extern FILE *stdout_like_ptr;
-void no_aliasing(void) {
+extern FILE *non_standard_stream_ptr;
+void test_fopen_does_not_alias_with_standard_streams(void) {
FILE *f = fopen("file", "r");
if (!f) return;
clang_analyzer_eval(f == stdin); // expected-warning {{FALSE}} no-TRUE
clang_analyzer_eval(f == stdout); // expected-warning {{FALSE}} no-TRUE
clang_analyzer_eval(f == stderr); // expected-warning {{FALSE}} no-TRUE
- clang_analyzer_eval(f == stdout_like_ptr); // expected-warning {{FALSE}} expected-warning {{TRUE}}
+ clang_analyzer_eval(f == non_standard_stream_ptr); // expected-warning {{FALSE}} expected-warning {{TRUE}}
if (f != stdout) {
fclose(f);
}
>From db92245ab09f195cbb859d85f2b93a81fd1b2617 Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Mon, 29 Jul 2024 10:46:16 +0200
Subject: [PATCH 3/6] NFC Fixup previous PR comment
Addresses
https://github.com/llvm/llvm-project/pull/100085#discussion_r1688054004
---
.../StaticAnalyzer/Checkers/StreamChecker.cpp | 25 ++++++-------------
1 file changed, 8 insertions(+), 17 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 942e7541a83cd..4454f30630e27 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -615,30 +615,22 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
});
}
- void initMacroValues(CheckerContext &C) const {
+ void initMacroValues(const Preprocessor &PP) const {
if (EofVal)
return;
- if (const std::optional<int> OptInt =
- tryExpandAsInteger("EOF", C.getPreprocessor()))
+ if (const std::optional<int> OptInt = tryExpandAsInteger("EOF", PP))
EofVal = *OptInt;
else
EofVal = -1;
- if (const std::optional<int> OptInt =
- tryExpandAsInteger("SEEK_SET", C.getPreprocessor()))
+ if (const std::optional<int> OptInt = tryExpandAsInteger("SEEK_SET", PP))
SeekSetVal = *OptInt;
- if (const std::optional<int> OptInt =
- tryExpandAsInteger("SEEK_END", C.getPreprocessor()))
+ if (const std::optional<int> OptInt = tryExpandAsInteger("SEEK_END", PP))
SeekEndVal = *OptInt;
- if (const std::optional<int> OptInt =
- tryExpandAsInteger("SEEK_CUR", C.getPreprocessor()))
+ if (const std::optional<int> OptInt = tryExpandAsInteger("SEEK_CUR", PP))
SeekCurVal = *OptInt;
}
- void initVaListType(CheckerContext &C) const {
- VaListType = C.getASTContext().getBuiltinVaListType().getCanonicalType();
- }
-
/// Searches for the ExplodedNode where the file descriptor was acquired for
/// StreamSym.
static const ExplodedNode *getAcquisitionSite(const ExplodedNode *N,
@@ -880,9 +872,6 @@ static ProgramStateRef escapeArgs(ProgramStateRef State, CheckerContext &C,
void StreamChecker::checkPreCall(const CallEvent &Call,
CheckerContext &C) const {
- initMacroValues(C);
- initVaListType(C);
-
const FnDescription *Desc = lookupFn(Call);
if (!Desc || !Desc->PreFn)
return;
@@ -2082,10 +2071,12 @@ getGlobalStreamPointerByName(const TranslationUnitDecl *TU, StringRef VarName) {
}
void StreamChecker::checkASTDecl(const TranslationUnitDecl *TU,
- AnalysisManager &, BugReporter &) const {
+ AnalysisManager &Mgr, BugReporter &) const {
StdinDecl = getGlobalStreamPointerByName(TU, "stdin");
StdoutDecl = getGlobalStreamPointerByName(TU, "stdout");
StderrDecl = getGlobalStreamPointerByName(TU, "stderr");
+ VaListType = TU->getASTContext().getBuiltinVaListType().getCanonicalType();
+ initMacroValues(Mgr.getPreprocessor());
}
//===----------------------------------------------------------------------===//
>From a3eb1a0f3b6c3057be2de62d608002b2148db267 Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Mon, 29 Jul 2024 10:54:45 +0200
Subject: [PATCH 4/6] NFC Fixup previous PR comment
Addresses
https://github.com/llvm/llvm-project/pull/100085#issuecomment-2245295054
---
clang/test/Analysis/stream.c | 28 ++++++++++++++++++++--------
1 file changed, 20 insertions(+), 8 deletions(-)
diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c
index aff884f190c8e..b9a5b1ba8cd49 100644
--- a/clang/test/Analysis/stream.c
+++ b/clang/test/Analysis/stream.c
@@ -1,11 +1,11 @@
// RUN: %clang_analyze_cc1 -triple=x86_64-pc-linux-gnu -analyzer-checker=core,unix.Stream,debug.ExprInspection \
-// RUN: -analyzer-config unix.Stream:Pedantic=true -verify %s
+// RUN: -analyzer-config eagerly-assume=false,unix.Stream:Pedantic=true -verify %s
// RUN: %clang_analyze_cc1 -triple=armv8-none-linux-eabi -analyzer-checker=core,unix.Stream,debug.ExprInspection \
-// RUN: -analyzer-config unix.Stream:Pedantic=true -verify %s
+// RUN: -analyzer-config eagerly-assume=false,unix.Stream:Pedantic=true -verify %s
// RUN: %clang_analyze_cc1 -triple=aarch64-linux-gnu -analyzer-checker=core,unix.Stream,debug.ExprInspection \
-// RUN: -analyzer-config unix.Stream:Pedantic=true -verify %s
+// RUN: -analyzer-config eagerly-assume=false,unix.Stream:Pedantic=true -verify %s
// RUN: %clang_analyze_cc1 -triple=hexagon -analyzer-checker=core,unix.Stream,debug.ExprInspection \
-// RUN: -analyzer-config unix.Stream:Pedantic=true -verify %s
+// RUN: -analyzer-config eagerly-assume=false,unix.Stream:Pedantic=true -verify %s
#include "Inputs/system-header-simulator.h"
#include "Inputs/system-header-simulator-for-malloc.h"
@@ -503,15 +503,27 @@ extern FILE *non_standard_stream_ptr;
void test_fopen_does_not_alias_with_standard_streams(void) {
FILE *f = fopen("file", "r");
if (!f) return;
- clang_analyzer_eval(f == stdin); // expected-warning {{FALSE}} no-TRUE
- clang_analyzer_eval(f == stdout); // expected-warning {{FALSE}} no-TRUE
- clang_analyzer_eval(f == stderr); // expected-warning {{FALSE}} no-TRUE
- clang_analyzer_eval(f == non_standard_stream_ptr); // expected-warning {{FALSE}} expected-warning {{TRUE}}
+ clang_analyzer_eval(f == stdin); // expected-warning {{FALSE}} no-TRUE
+ clang_analyzer_eval(f == stdout); // expected-warning {{FALSE}} no-TRUE
+ clang_analyzer_eval(f == stderr); // expected-warning {{FALSE}} no-TRUE
+ clang_analyzer_eval(f == non_standard_stream_ptr); // expected-warning {{UNKNOWN}}
if (f != stdout) {
fclose(f);
}
} // no-leak: 'fclose()' is always called because 'f' cannot be 'stdout'.
+void reopen_std_stream(void) {
+ FILE *oldStdout = stdout;
+ fclose(stdout);
+ FILE *fp = fopen("blah", "w");
+ if (!fp) return;
+
+ stdout = fp; // Let's make them alias.
+ clang_analyzer_eval(fp == oldStdout); // expected-warning {{UNKNOWN}}
+ clang_analyzer_eval(fp == stdout); // expected-warning {{TRUE}} no-FALSE
+ clang_analyzer_eval(oldStdout == stdout); // expected-warning {{UNKNOWN}}
+}
+
void only_success_path_does_not_alias_with_stdout(void) {
if (stdout) return;
FILE *f = fopen("/tmp/foof", "r"); // no-crash
>From 0a762c12a79b2ceca1813371f8cfbe148e93a05d Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Mon, 29 Jul 2024 10:58:41 +0200
Subject: [PATCH 5/6] NFC Fixup previous PR comment
Addresses
https://github.com/llvm/llvm-project/pull/100085#issuecomment-2247168389
---
clang/docs/analyzer/checkers.rst | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 76a9aae170893..452948aa2cb00 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -1703,7 +1703,11 @@ are detected:
* Invalid 3rd ("``whence``") argument to ``fseek``.
The stream operations are by this checker usually split into two cases, a success
-and a failure case. However, in the case of write operations (like ``fwrite``,
+and a failure case.
+On the success case it also assumes that the current value of ``stdout``,
+``stderr``, or ``stdin`` can't be equal to the file pointer returned by ``fopen``.
+
+In the case of write operations (like ``fwrite``,
``fprintf`` and even ``fsetpos``) this behavior could produce a large amount of
unwanted reports on projects that don't have error checks around the write
operations, so by default the checker assumes that write operations always succeed.
@@ -1766,13 +1770,6 @@ are assumed to succeed.)
fclose(p);
}
-**Limitations**
-
-The checker does not track the correspondence between integer file descriptors
-and ``FILE *`` pointers. Operations on standard streams like ``stdin`` are not
-treated specially and are therefore often not recognized (because these streams
-are usually not opened explicitly by the program, and are global variables).
-
.. _osx-checkers:
osx
>From bba8ac7fe2f4a144e806fbaf32ebcd94dc0b7f12 Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Mon, 29 Jul 2024 11:53:11 +0200
Subject: [PATCH 6/6] Add back the "Limitations" section
https://github.com/llvm/llvm-project/pull/100990#discussion_r1694902780
---
clang/docs/analyzer/checkers.rst | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 452948aa2cb00..b3ef591b2d88c 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -1770,6 +1770,11 @@ are assumed to succeed.)
fclose(p);
}
+**Limitations**
+
+The checker does not track the correspondence between integer file descriptors
+and ``FILE *`` pointers.
+
.. _osx-checkers:
osx
More information about the cfe-commits
mailing list