[clang] [clang][analyzer] Support `fgets` in the SteamChecker (PR #73638)
Ben Shi via cfe-commits
cfe-commits at lists.llvm.org
Tue Nov 28 03:37:05 PST 2023
https://github.com/benshi001 created https://github.com/llvm/llvm-project/pull/73638
This PR contains two commits, the first one is identical to https://github.com/llvm/llvm-project/pull/73335, please just review the second one about `fgets`.
>From 31d7b6c5ddbbe37d59819fd7728e817102a9d24b Mon Sep 17 00:00:00 2001
From: Ben Shi <bennshi at tencent.com>
Date: Fri, 24 Nov 2023 22:51:27 +0800
Subject: [PATCH 1/2] [clang][analyzer] Support `fputs` in the StreamChecker
---
.../StaticAnalyzer/Checkers/StreamChecker.cpp | 105 +++++++++++++-----
.../Analysis/Inputs/system-header-simulator.h | 1 +
clang/test/Analysis/stream-error.c | 42 ++++++-
clang/test/Analysis/stream.c | 6 +
4 files changed, 121 insertions(+), 33 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 8eca989d7bcdea4..eccb2063fad10d0 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -252,10 +252,13 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
std::bind(&StreamChecker::evalFreadFwrite, _1, _2, _3, _4, false), 3}},
{{{"fgetc"}, 1},
{std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, true),
- std::bind(&StreamChecker::evalFgetcFputc, _1, _2, _3, _4, true), 0}},
+ std::bind(&StreamChecker::evalFgetc, _1, _2, _3, _4), 0}},
{{{"fputc"}, 2},
{std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, false),
- std::bind(&StreamChecker::evalFgetcFputc, _1, _2, _3, _4, false), 1}},
+ std::bind(&StreamChecker::evalFputx, _1, _2, _3, _4, true), 1}},
+ {{{"fputs"}, 2},
+ {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, false),
+ std::bind(&StreamChecker::evalFputx, _1, _2, _3, _4, false), 1}},
{{{"fseek"}, 3},
{&StreamChecker::preFseek, &StreamChecker::evalFseek, 0}},
{{{"ftell"}, 1},
@@ -317,8 +320,11 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
void evalFreadFwrite(const FnDescription *Desc, const CallEvent &Call,
CheckerContext &C, bool IsFread) const;
- void evalFgetcFputc(const FnDescription *Desc, const CallEvent &Call,
- CheckerContext &C, bool IsRead) const;
+ void evalFgetc(const FnDescription *Desc, const CallEvent &Call,
+ CheckerContext &C) const;
+
+ void evalFputx(const FnDescription *Desc, const CallEvent &Call,
+ CheckerContext &C, bool IsSingleChar) const;
void preFseek(const FnDescription *Desc, const CallEvent &Call,
CheckerContext &C) const;
@@ -754,9 +760,8 @@ void StreamChecker::evalFreadFwrite(const FnDescription *Desc,
C.addTransition(StateFailed);
}
-void StreamChecker::evalFgetcFputc(const FnDescription *Desc,
- const CallEvent &Call, CheckerContext &C,
- bool IsRead) const {
+void StreamChecker::evalFgetc(const FnDescription *Desc, const CallEvent &Call,
+ CheckerContext &C) const {
ProgramStateRef State = C.getState();
SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol();
if (!StreamSym)
@@ -773,27 +778,15 @@ void StreamChecker::evalFgetcFputc(const FnDescription *Desc,
assertStreamStateOpened(OldSS);
// `fgetc` returns the read character on success, otherwise returns EOF.
- // `fputc` returns the written character on success, otherwise returns EOF.
- // Generate a transition for the success state of fputc.
- if (!IsRead) {
- std::optional<NonLoc> PutVal = Call.getArgSVal(0).getAs<NonLoc>();
- if (!PutVal)
- return;
- ProgramStateRef StateNotFailed =
- State->BindExpr(CE, C.getLocationContext(), *PutVal);
- StateNotFailed =
- StateNotFailed->set<StreamMap>(StreamSym, StreamState::getOpened(Desc));
- C.addTransition(StateNotFailed);
- }
- // Generate a transition for the success state of fgetc.
+ // Generate a transition for the success state of `fgetc`.
// If we know the state to be FEOF at fgetc, do not add a success state.
- else if (OldSS->ErrorState != ErrorFEof) {
+ if (OldSS->ErrorState != ErrorFEof) {
NonLoc RetVal = makeRetVal(C, CE).castAs<NonLoc>();
ProgramStateRef StateNotFailed =
State->BindExpr(CE, C.getLocationContext(), RetVal);
SValBuilder &SVB = C.getSValBuilder();
- auto &ASTC = C.getASTContext();
+ ASTContext &ASTC = C.getASTContext();
// The returned 'unsigned char' of `fgetc` is converted to 'int',
// so we need to check if it is in range [0, 255].
auto CondLow =
@@ -824,20 +817,74 @@ void StreamChecker::evalFgetcFputc(const FnDescription *Desc,
// If a (non-EOF) error occurs, the resulting value of the file position
// indicator for the stream is indeterminate.
- StreamErrorState NewES;
- if (IsRead)
- NewES =
- OldSS->ErrorState == ErrorFEof ? ErrorFEof : ErrorFEof | ErrorFError;
- else
- NewES = ErrorFError;
+ StreamErrorState NewES =
+ OldSS->ErrorState == ErrorFEof ? ErrorFEof : ErrorFEof | ErrorFError;
StreamState NewSS = StreamState::getOpened(Desc, NewES, !NewES.isFEof());
StateFailed = StateFailed->set<StreamMap>(StreamSym, NewSS);
- if (IsRead && OldSS->ErrorState != ErrorFEof)
+ if (OldSS->ErrorState != ErrorFEof)
C.addTransition(StateFailed, constructSetEofNoteTag(C, StreamSym));
else
C.addTransition(StateFailed);
}
+void StreamChecker::evalFputx(const FnDescription *Desc, const CallEvent &Call,
+ CheckerContext &C, bool IsSingleChar) const {
+ ProgramStateRef State = C.getState();
+ SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol();
+ if (!StreamSym)
+ return;
+
+ const CallExpr *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
+ if (!CE)
+ return;
+
+ const StreamState *OldSS = State->get<StreamMap>(StreamSym);
+ if (!OldSS)
+ return;
+
+ assertStreamStateOpened(OldSS);
+
+ // `fputc` returns the written character on success, otherwise returns EOF.
+ // `fputs` returns a non negative value on sucecess, otherwise returns EOF.
+
+ if (IsSingleChar) {
+ // Generate a transition for the success state of `fputc`.
+ std::optional<NonLoc> PutVal = Call.getArgSVal(0).getAs<NonLoc>();
+ if (!PutVal)
+ return;
+ ProgramStateRef StateNotFailed =
+ State->BindExpr(CE, C.getLocationContext(), *PutVal);
+ StateNotFailed =
+ StateNotFailed->set<StreamMap>(StreamSym, StreamState::getOpened(Desc));
+ C.addTransition(StateNotFailed);
+ } else {
+ // Generate a transition for the success state of `fputs`.
+ NonLoc RetVal = makeRetVal(C, CE).castAs<NonLoc>();
+ ProgramStateRef StateNotFailed =
+ State->BindExpr(CE, C.getLocationContext(), RetVal);
+ SValBuilder &SVB = C.getSValBuilder();
+ auto &ASTC = C.getASTContext();
+ auto Cond = SVB.evalBinOp(State, BO_GE, RetVal, SVB.makeZeroVal(ASTC.IntTy),
+ SVB.getConditionType())
+ .getAs<DefinedOrUnknownSVal>();
+ if (!Cond)
+ return;
+ StateNotFailed = StateNotFailed->assume(*Cond, true);
+ if (!StateNotFailed)
+ return;
+ StateNotFailed =
+ StateNotFailed->set<StreamMap>(StreamSym, StreamState::getOpened(Desc));
+ C.addTransition(StateNotFailed);
+ }
+
+ // Add transition for the failed state. The resulting value of the file
+ // position indicator for the stream is indeterminate.
+ ProgramStateRef StateFailed = bindInt(*EofVal, State, C, CE);
+ StreamState NewSS = StreamState::getOpened(Desc, ErrorFError, true);
+ StateFailed = StateFailed->set<StreamMap>(StreamSym, NewSS);
+ C.addTransition(StateFailed);
+}
+
void StreamChecker::preFseek(const FnDescription *Desc, const CallEvent &Call,
CheckerContext &C) const {
ProgramStateRef State = C.getState();
diff --git a/clang/test/Analysis/Inputs/system-header-simulator.h b/clang/test/Analysis/Inputs/system-header-simulator.h
index fc57e8bdc3d30c3..7ef5f29fbf42cb1 100644
--- a/clang/test/Analysis/Inputs/system-header-simulator.h
+++ b/clang/test/Analysis/Inputs/system-header-simulator.h
@@ -50,6 +50,7 @@ size_t fread(void *restrict, size_t, size_t, FILE *restrict);
size_t fwrite(const void *restrict, size_t, size_t, FILE *restrict);
int fgetc(FILE *stream);
int fputc(int ch, FILE *stream);
+int fputs(const char *restrict s, FILE *restrict stream);
int fseek(FILE *__stream, long int __off, int __whence);
long int ftell(FILE *__stream);
void rewind(FILE *__stream);
diff --git a/clang/test/Analysis/stream-error.c b/clang/test/Analysis/stream-error.c
index 38e6b77b9bb5053..c90d0cdc5742d37 100644
--- a/clang/test/Analysis/stream-error.c
+++ b/clang/test/Analysis/stream-error.c
@@ -131,16 +131,50 @@ void error_fputc(void) {
if (Ret == EOF) {
clang_analyzer_eval(ferror(F)); // expected-warning {{TRUE}}
clang_analyzer_eval(feof(F)); // expected-warning {{FALSE}}
- fputc('Y', F); // expected-warning {{might be 'indeterminate'}}
+ fputc('Y', F); // expected-warning {{might be 'indeterminate'}}
} else {
- clang_analyzer_eval(Ret == 'X'); // expected-warning {{TRUE}}
- clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}}
- fputc('Y', F); // no-warning
+ clang_analyzer_eval(Ret == 'X'); // expected-warning {{TRUE}}
+ clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}}
+ fputc('Y', F); // no-warning
}
fclose(F);
fputc('A', F); // expected-warning {{Stream might be already closed}}
}
+void error_fputs(void) {
+ FILE *F = tmpfile();
+ if (!F)
+ return;
+ int Ret = fputs("XYZ", F);
+ if (Ret >= 0) {
+ clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}}
+ fputs("QWD", F); // no-warning
+ } else {
+ clang_analyzer_eval(Ret == EOF); // expected-warning {{TRUE}}
+ clang_analyzer_eval(ferror(F)); // expected-warning {{TRUE}}
+ clang_analyzer_eval(feof(F)); // expected-warning {{FALSE}}
+ fputs("QWD", F); // expected-warning {{might be 'indeterminate'}}
+ }
+ fclose(F);
+ fputs("ABC", F); // expected-warning {{Stream might be already closed}}
+}
+
+void write_after_eof_is_allowed(void) {
+ FILE *F = tmpfile();
+ if (!F)
+ return;
+ StreamTesterChecker_make_feof_stream(F);
+ if (fputs("QWD", F) >= 0) // no-warning
+ clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}}
+ StreamTesterChecker_make_feof_stream(F);
+ if (fputc('Q', F) == 'Q') // no-warning
+ clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}}
+ StreamTesterChecker_make_feof_stream(F);
+ if (fwrite("012345678", 1, 10, F) == 10) // no-warning
+ clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}}
+ fclose(F);
+}
+
void freadwrite_zerosize(FILE *F) {
size_t Ret;
Ret = fwrite(0, 1, 0, F);
diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c
index a7ee9982478bb96..b0f39af8eb2f3a3 100644
--- a/clang/test/Analysis/stream.c
+++ b/clang/test/Analysis/stream.c
@@ -26,6 +26,12 @@ void check_fputc(void) {
fclose(fp);
}
+void check_fputs(void) {
+ FILE *fp = tmpfile();
+ fputs("ABC", fp); // expected-warning {{Stream pointer might be NULL}}
+ fclose(fp);
+}
+
void check_fseek(void) {
FILE *fp = tmpfile();
fseek(fp, 0, 0); // expected-warning {{Stream pointer might be NULL}}
>From b6c1d8e3252d692b2fb7ba585c5c03141327b000 Mon Sep 17 00:00:00 2001
From: Ben Shi <bennshi at tencent.com>
Date: Tue, 28 Nov 2023 19:27:21 +0800
Subject: [PATCH 2/2] [clang][analyzer] Support `fgets` in the StreamChecker
---
.../StaticAnalyzer/Checkers/StreamChecker.cpp | 94 ++++++++++++-------
.../Analysis/Inputs/system-header-simulator.h | 1 +
clang/test/Analysis/stream-error.c | 23 +++++
clang/test/Analysis/stream.c | 7 ++
4 files changed, 89 insertions(+), 36 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index eccb2063fad10d0..a4799b5f762caee 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -252,7 +252,10 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
std::bind(&StreamChecker::evalFreadFwrite, _1, _2, _3, _4, false), 3}},
{{{"fgetc"}, 1},
{std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, true),
- std::bind(&StreamChecker::evalFgetc, _1, _2, _3, _4), 0}},
+ std::bind(&StreamChecker::evalFgetx, _1, _2, _3, _4, true), 0}},
+ {{{"fgets"}, 3},
+ {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, true),
+ std::bind(&StreamChecker::evalFgetx, _1, _2, _3, _4, false), 2}},
{{{"fputc"}, 2},
{std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, false),
std::bind(&StreamChecker::evalFputx, _1, _2, _3, _4, true), 1}},
@@ -320,8 +323,8 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
void evalFreadFwrite(const FnDescription *Desc, const CallEvent &Call,
CheckerContext &C, bool IsFread) const;
- void evalFgetc(const FnDescription *Desc, const CallEvent &Call,
- CheckerContext &C) const;
+ void evalFgetx(const FnDescription *Desc, const CallEvent &Call,
+ CheckerContext &C, bool SingleChar) const;
void evalFputx(const FnDescription *Desc, const CallEvent &Call,
CheckerContext &C, bool IsSingleChar) const;
@@ -760,8 +763,8 @@ void StreamChecker::evalFreadFwrite(const FnDescription *Desc,
C.addTransition(StateFailed);
}
-void StreamChecker::evalFgetc(const FnDescription *Desc, const CallEvent &Call,
- CheckerContext &C) const {
+void StreamChecker::evalFgetx(const FnDescription *Desc, const CallEvent &Call,
+ CheckerContext &C, bool SingleChar) const {
ProgramStateRef State = C.getState();
SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol();
if (!StreamSym)
@@ -778,42 +781,61 @@ void StreamChecker::evalFgetc(const FnDescription *Desc, const CallEvent &Call,
assertStreamStateOpened(OldSS);
// `fgetc` returns the read character on success, otherwise returns EOF.
+ // `fgets` returns the read buffer address on success, otherwise returns NULL.
- // Generate a transition for the success state of `fgetc`.
- // If we know the state to be FEOF at fgetc, do not add a success state.
if (OldSS->ErrorState != ErrorFEof) {
- NonLoc RetVal = makeRetVal(C, CE).castAs<NonLoc>();
- ProgramStateRef StateNotFailed =
- State->BindExpr(CE, C.getLocationContext(), RetVal);
- SValBuilder &SVB = C.getSValBuilder();
- ASTContext &ASTC = C.getASTContext();
- // The returned 'unsigned char' of `fgetc` is converted to 'int',
- // so we need to check if it is in range [0, 255].
- auto CondLow =
- SVB.evalBinOp(State, BO_GE, RetVal, SVB.makeZeroVal(ASTC.IntTy),
- SVB.getConditionType())
- .getAs<DefinedOrUnknownSVal>();
- auto CondHigh =
- SVB.evalBinOp(State, BO_LE, RetVal,
- SVB.makeIntVal(SVB.getBasicValueFactory()
- .getMaxValue(ASTC.UnsignedCharTy)
- .getLimitedValue(),
- ASTC.IntTy),
- SVB.getConditionType())
- .getAs<DefinedOrUnknownSVal>();
- if (!CondLow || !CondHigh)
- return;
- StateNotFailed = StateNotFailed->assume(*CondLow, true);
- if (!StateNotFailed)
- return;
- StateNotFailed = StateNotFailed->assume(*CondHigh, true);
- if (!StateNotFailed)
- return;
- C.addTransition(StateNotFailed);
+ if (SingleChar) {
+ // Generate a transition for the success state of `fgetc`.
+ NonLoc RetVal = makeRetVal(C, CE).castAs<NonLoc>();
+ ProgramStateRef StateNotFailed =
+ State->BindExpr(CE, C.getLocationContext(), RetVal);
+ SValBuilder &SVB = C.getSValBuilder();
+ ASTContext &ASTC = C.getASTContext();
+ // The returned 'unsigned char' of `fgetc` is converted to 'int',
+ // so we need to check if it is in range [0, 255].
+ auto CondLow =
+ SVB.evalBinOp(State, BO_GE, RetVal, SVB.makeZeroVal(ASTC.IntTy),
+ SVB.getConditionType())
+ .getAs<DefinedOrUnknownSVal>();
+ auto CondHigh =
+ SVB.evalBinOp(State, BO_LE, RetVal,
+ SVB.makeIntVal(SVB.getBasicValueFactory()
+ .getMaxValue(ASTC.UnsignedCharTy)
+ .getLimitedValue(),
+ ASTC.IntTy),
+ SVB.getConditionType())
+ .getAs<DefinedOrUnknownSVal>();
+ if (!CondLow || !CondHigh)
+ return;
+ StateNotFailed = StateNotFailed->assume(*CondLow, true);
+ if (!StateNotFailed)
+ return;
+ StateNotFailed = StateNotFailed->assume(*CondHigh, true);
+ if (!StateNotFailed)
+ return;
+ C.addTransition(StateNotFailed);
+ } else {
+ // Generate a transition for the success state of `fgets`.
+ std::optional<DefinedSVal> GetBuf =
+ Call.getArgSVal(0).getAs<DefinedSVal>();
+ if (!GetBuf)
+ return;
+ ProgramStateRef StateNotFailed =
+ State->BindExpr(CE, C.getLocationContext(), *GetBuf);
+ StateNotFailed = StateNotFailed->set<StreamMap>(
+ StreamSym, StreamState::getOpened(Desc));
+ C.addTransition(StateNotFailed);
+ }
}
// Add transition for the failed state.
- ProgramStateRef StateFailed = bindInt(*EofVal, State, C, CE);
+ ProgramStateRef StateFailed;
+ if (SingleChar)
+ StateFailed = bindInt(*EofVal, State, C, CE);
+ else
+ StateFailed =
+ State->BindExpr(CE, C.getLocationContext(),
+ C.getSValBuilder().makeNullWithType(CE->getType()));
// If a (non-EOF) error occurs, the resulting value of the file position
// indicator for the stream is indeterminate.
diff --git a/clang/test/Analysis/Inputs/system-header-simulator.h b/clang/test/Analysis/Inputs/system-header-simulator.h
index 7ef5f29fbf42cb1..7089bd8bfc9d983 100644
--- a/clang/test/Analysis/Inputs/system-header-simulator.h
+++ b/clang/test/Analysis/Inputs/system-header-simulator.h
@@ -49,6 +49,7 @@ int fclose(FILE *fp);
size_t fread(void *restrict, size_t, size_t, FILE *restrict);
size_t fwrite(const void *restrict, size_t, size_t, FILE *restrict);
int fgetc(FILE *stream);
+char *fgets(char *restrict str, int count, FILE *restrict stream);
int fputc(int ch, FILE *stream);
int fputs(const char *restrict s, FILE *restrict stream);
int fseek(FILE *__stream, long int __off, int __whence);
diff --git a/clang/test/Analysis/stream-error.c b/clang/test/Analysis/stream-error.c
index c90d0cdc5742d37..c8332bcbfa8ca72 100644
--- a/clang/test/Analysis/stream-error.c
+++ b/clang/test/Analysis/stream-error.c
@@ -123,6 +123,29 @@ void error_fgetc(void) {
fgetc(F); // expected-warning {{Stream might be already closed}}
}
+void error_fgets(void) {
+ FILE *F = tmpfile();
+ char Buf[256];
+ if (!F)
+ return;
+ char *Ret = fgets(Buf, sizeof(Buf), F);
+ if (Ret == Buf) {
+ clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}}
+ } else {
+ clang_analyzer_eval(Ret == NULL); // expected-warning {{TRUE}}
+ clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{TRUE}}
+ if (feof(F)) {
+ clang_analyzer_eval(ferror(F)); // expected-warning {{FALSE}}
+ fgets(Buf, sizeof(Buf), F); // expected-warning {{Read function called when stream is in EOF state}}
+ } else {
+ clang_analyzer_eval(ferror(F)); // expected-warning {{TRUE}}
+ fgets(Buf, sizeof(Buf), F); // expected-warning {{might be 'indeterminate'}}
+ }
+ }
+ fclose(F);
+ fgets(Buf, sizeof(Buf), F); // expected-warning {{Stream might be already closed}}
+}
+
void error_fputc(void) {
FILE *F = tmpfile();
if (!F)
diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c
index b0f39af8eb2f3a3..0b655dbdc571fe2 100644
--- a/clang/test/Analysis/stream.c
+++ b/clang/test/Analysis/stream.c
@@ -20,6 +20,13 @@ void check_fgetc(void) {
fclose(fp);
}
+void check_fgets(void) {
+ FILE *fp = tmpfile();
+ char buf[256];
+ fgets(buf, sizeof(buf), fp); // expected-warning {{Stream pointer might be NULL}}
+ fclose(fp);
+}
+
void check_fputc(void) {
FILE *fp = tmpfile();
fputc('A', fp); // expected-warning {{Stream pointer might be NULL}}
More information about the cfe-commits
mailing list