[clang] [clang][analyzer] Fix argument invalidations in StreamChecker. (PR #79470)
Balázs Kéri via cfe-commits
cfe-commits at lists.llvm.org
Fri Jan 26 08:11:35 PST 2024
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?= <balazs.keri at ericsson.com>
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 <functional>
#include <optional>
@@ -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<unsigned int> &EscapingArgs) {
+ const auto *CE = Call.getOriginExpr();
+
+ SmallVector<SVal> 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<NonLoc>();
@@ -1032,6 +1057,11 @@ void StreamChecker::evalFscanf(const FnDescription *Desc, const CallEvent &Call,
return;
StateNotFailed = StateNotFailed->assume(*RetGeZero, true);
+ SmallVector<unsigned int> 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<NonLoc>();
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 000000000000000..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(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}}
+
+ fclose(F);
+}
+
+void test_fgetpos() {
+ FILE *F = tmpfile();
+ if (!F)
+ return;
+
+ fpos_t Pos = 1;
+ int Ret = fgetpos(F, &Pos);
+ if (Ret == 0) {
+ clang_analyzer_dump(Pos); // expected-warning {{conj_$}}
+ } else {
+ clang_analyzer_dump(Pos); // expected-warning {{1 S32b}}
+ }
+
+ fclose(F);
+}
>From 8834232bdacd6cc91602c6dbd47edcad2a358cdd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= <balazs.keri at ericsson.com>
Date: Fri, 26 Jan 2024 17:10:48 +0100
Subject: [PATCH 2/2] using ArrayRef, improved test file
---
.../StaticAnalyzer/Checkers/StreamChecker.cpp | 6 +--
clang/test/Analysis/stream-invalidate.c | 40 +++++++++++++------
2 files changed, 30 insertions(+), 16 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 166bd981a003af2..96f578cebe5020d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -545,9 +545,9 @@ const ExplodedNode *StreamChecker::getAcquisitionSite(const ExplodedNode *N,
return nullptr;
}
-static ProgramStateRef
-escapeArgs(ProgramStateRef State, CheckerContext &C, const CallEvent &Call,
- const SmallVector<unsigned int> &EscapingArgs) {
+static ProgramStateRef escapeArgs(ProgramStateRef State, CheckerContext &C,
+ const CallEvent &Call,
+ ArrayRef<unsigned int> EscapingArgs) {
const auto *CE = Call.getOriginExpr();
SmallVector<SVal> EscapingVals;
diff --git a/clang/test/Analysis/stream-invalidate.c b/clang/test/Analysis/stream-invalidate.c
index c5b7874f0ee8f6f..6745d11a2fe7017 100644
--- a/clang/test/Analysis/stream-invalidate.c
+++ b/clang/test/Analysis/stream-invalidate.c
@@ -1,16 +1,12 @@
// 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+");
@@ -19,7 +15,7 @@ void test_fread(void) {
char Buf[3] = {10, 10, 10};
fread(Buf, 1, 3, F);
- // this check applies to succes and failure
+ // The check applies to success 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)) {
@@ -39,7 +35,7 @@ void test_fwrite(void) {
char Buf[3] = {10, 10, 10};
fwrite(Buf, 1, 3, F);
- // this check applies to succes and failure
+ // The check applies to success and failure.
clang_analyzer_dump(Buf[0]); // expected-warning {{10 S32b}}
clang_analyzer_dump(Buf[2]); // expected-warning {{10 S32b}}
@@ -53,7 +49,7 @@ void test_fgets() {
char Buf[3] = {10, 10, 10};
fgets(Buf, 3, F);
- // this check applies to succes and failure
+ // The check applies to success 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)) {
@@ -73,7 +69,7 @@ void test_fputs() {
char *Buf = "aaa";
fputs(Buf, F);
- // this check applies to succes and failure
+ // The check applies to success 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}}
@@ -89,14 +85,25 @@ void test_fscanf() {
int a = 1;
unsigned b;
int Ret = fscanf(F, "%d %u", &a, &b);
- if (Ret >= 0) {
- // FIXME: return value
+ if (Ret == 0) {
+ clang_analyzer_dump(a); // expected-warning {{conj_$}}
+ // FIXME: should be {{1 S32b}}.
+ clang_analyzer_dump(b); // expected-warning {{conj_$}}
+ // FIXME: should be {{uninitialized value}}.
+ } else if (Ret == 1) {
+ clang_analyzer_dump(a); // expected-warning {{conj_$}}
+ clang_analyzer_dump(b); // expected-warning {{conj_$}}
+ // FIXME: should be {{uninitialized value}}.
+ } else if (Ret >= 2) {
clang_analyzer_dump(a); // expected-warning {{conj_$}}
clang_analyzer_dump(b); // expected-warning {{conj_$}}
+ clang_analyzer_eval(Ret == 2); // expected-warning {{FALSE}} expected-warning {{TRUE}}
+ // FIXME: should be only TRUE.
} else {
clang_analyzer_dump(a); // expected-warning {{1 S32b}}
clang_analyzer_dump(b); // expected-warning {{uninitialized value}}
}
+
fclose(F);
}
@@ -108,10 +115,17 @@ void test_getdelim(char *P, size_t Sz) {
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}} \
+ if (Ret < 0) {
+ clang_analyzer_eval(P == P1); // expected-warning {{FALSE}} \
+ // expected-warning {{TRUE}}
+ clang_analyzer_eval(Sz == Sz1); // expected-warning {{FALSE}} \
+ // expected-warning {{TRUE}}
+ } else {
+ clang_analyzer_eval(P == P1); // expected-warning {{FALSE}} \
// expected-warning {{TRUE}}
+ clang_analyzer_eval(Sz == Sz1); // expected-warning {{FALSE}} \
+ // expected-warning {{TRUE}}
+ }
fclose(F);
}
More information about the cfe-commits
mailing list