=?utf-8?q?Balázs_Kéri?= <balazs.k...@ericsson.com>, =?utf-8?q?Balázs_Kéri?= <balazs.k...@ericsson.com> Message-ID: In-Reply-To: <llvm.org/llvm/llvm-project/pull/77...@github.com>
https://github.com/hctim updated https://github.com/llvm/llvm-project/pull/77331 >From 9bcc43b5c62ba969f91c495d4d570c5c4337aca2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= <balazs.k...@ericsson.com> Date: Mon, 8 Jan 2024 16:42:58 +0100 Subject: [PATCH 1/3] [clang][analyzer] Add 'ungetc' to StreamChecker. --- clang/docs/ReleaseNotes.rst | 4 +- .../Checkers/StdLibraryFunctionsChecker.cpp | 19 ++++++++ .../StaticAnalyzer/Checkers/StreamChecker.cpp | 44 +++++++++++++++++++ .../Analysis/Inputs/system-header-simulator.h | 1 + clang/test/Analysis/stream-error.c | 16 +++++++ clang/test/Analysis/stream-noopen.c | 25 +++++++++++ clang/test/Analysis/stream.c | 6 +++ 7 files changed, 113 insertions(+), 2 deletions(-) diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index c7bf162426a68c..3bfcd7997faa19 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -1162,8 +1162,8 @@ Improvements (`c3a87ddad62a <https://github.com/llvm/llvm-project/commit/c3a87ddad62a6cc01acaccc76592bc6730c8ac3c>`_, `0954dc3fb921 <https://github.com/llvm/llvm-project/commit/0954dc3fb9214b994623f5306473de075f8e3593>`_) -- Improved the ``alpha.unix.Stream`` checker by modeling more functions like, - ``fflush``, ``fputs``, ``fgetc``, ``fputc``, ``fopen``, ``fdopen``, ``fgets``, ``tmpfile``. +- Improved the ``alpha.unix.Stream`` checker by modeling more functions: + ``fputs``, ``fputc``, ``fgets``, ``fgetc``, ``fdopen``, ``ungetc``, ``fflush``. (`#76776 <https://github.com/llvm/llvm-project/pull/76776>`_, `#74296 <https://github.com/llvm/llvm-project/pull/74296>`_, `#73335 <https://github.com/llvm/llvm-project/pull/73335>`_, diff --git a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp index 20068653d530a3..e16bf9feb5b1c4 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp @@ -2201,6 +2201,25 @@ void StdLibraryFunctionsChecker::initFunctionSummaries( ErrnoNEZeroIrrelevant, GenericFailureMsg) .ArgConstraint(NotNull(ArgNo(0)))); + // int ungetc(int c, FILE *stream); + addToFunctionSummaryMap( + "ungetc", Signature(ArgTypes{IntTy, FilePtrTy}, RetType{IntTy}), + Summary(NoEvalCall) + .Case({ReturnValueCondition(BO_EQ, ArgNo(0)), + ArgumentCondition(0, WithinRange, {{0, UCharRangeMax}})}, + ErrnoMustNotBeChecked, GenericSuccessMsg) + .Case({ReturnValueCondition(WithinRange, SingleValue(EOFv)), + ArgumentCondition(0, WithinRange, {{EOFv, EOFv}})}, + ErrnoNEZeroIrrelevant, + "Assuming that 'ungetc' fails because EOF was passed as " + "character") + .Case({ReturnValueCondition(WithinRange, SingleValue(EOFv)), + ArgumentCondition(0, WithinRange, {{0, UCharRangeMax}})}, + ErrnoNEZeroIrrelevant, GenericFailureMsg) + .ArgConstraint(ArgumentCondition( + 0, WithinRange, {{EOFv, EOFv}, {0, UCharRangeMax}})) + .ArgConstraint(NotNull(ArgNo(1)))); + // int fseek(FILE *stream, long offset, int whence); // FIXME: It can be possible to get the 'SEEK_' values (like EOFv) and use // these for condition of arg 2. diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp index 25da3c18e8519f..a3d1f34d3d1a9a 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp @@ -263,6 +263,9 @@ class StreamChecker : public Checker<check::PreCall, eval::Call, {{{"fputs"}, 2}, {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, false), std::bind(&StreamChecker::evalFputx, _1, _2, _3, _4, false), 1}}, + {{{"ungetc"}, 2}, + {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, false), + std::bind(&StreamChecker::evalUngetc, _1, _2, _3, _4), 1}}, {{{"fseek"}, 3}, {&StreamChecker::preFseek, &StreamChecker::evalFseek, 0}}, {{{"ftell"}, 1}, @@ -332,6 +335,9 @@ class StreamChecker : public Checker<check::PreCall, eval::Call, void evalFputx(const FnDescription *Desc, const CallEvent &Call, CheckerContext &C, bool IsSingleChar) const; + void evalUngetc(const FnDescription *Desc, const CallEvent &Call, + CheckerContext &C) const; + void preFseek(const FnDescription *Desc, const CallEvent &Call, CheckerContext &C) const; void evalFseek(const FnDescription *Desc, const CallEvent &Call, @@ -916,6 +922,44 @@ void StreamChecker::evalFputx(const FnDescription *Desc, const CallEvent &Call, C.addTransition(StateFailed); } +void StreamChecker::evalUngetc(const FnDescription *Desc, const CallEvent &Call, + CheckerContext &C) 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); + + // Generate a transition for the success state. + 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); + + // Add transition for the failed state. + // Failure of 'ungetc' does not result in feof or ferror state. + // If the PutVal has value of EofVal the function should "fail", but this is + // the same transition as the success state. + // FIXME: Is it possible that StateFailed == StateNotFailed ? + ProgramStateRef StateFailed = bindInt(*EofVal, State, C, CE); + StreamState NewSS = StreamState::getOpened(Desc); + 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 8c43c48c6a3e45..caae59c38a4c8e 100644 --- a/clang/test/Analysis/Inputs/system-header-simulator.h +++ b/clang/test/Analysis/Inputs/system-header-simulator.h @@ -53,6 +53,7 @@ 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 ungetc(int c, FILE *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 13c6684b5840af..c038348e799d29 100644 --- a/clang/test/Analysis/stream-error.c +++ b/clang/test/Analysis/stream-error.c @@ -191,6 +191,22 @@ void error_fputs(void) { fputs("ABC", F); // expected-warning {{Stream might be already closed}} } +void error_ungetc() { + FILE *F = tmpfile(); + if (!F) + return; + int Ret = ungetc('X', F); + clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}} + if (Ret == EOF) { + clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}} + } else { + clang_analyzer_eval(Ret == 'X'); // expected-warning {{TRUE}} + } + fputc('Y', F); // no-warning + fclose(F); + ungetc('A', F); // expected-warning {{Stream might be already closed}} +} + void write_after_eof_is_allowed(void) { FILE *F = tmpfile(); if (!F) diff --git a/clang/test/Analysis/stream-noopen.c b/clang/test/Analysis/stream-noopen.c index 2daf640c18a1d4..8ad101ee1e8c13 100644 --- a/clang/test/Analysis/stream-noopen.c +++ b/clang/test/Analysis/stream-noopen.c @@ -138,6 +138,31 @@ void test_rewind(FILE *F) { rewind(F); } +void test_ungetc(FILE *F) { + int Ret = ungetc('X', F); + clang_analyzer_eval(F != NULL); // expected-warning {{TRUE}} + if (Ret == 'X') { + if (errno) {} // expected-warning {{undefined}} + } else { + clang_analyzer_eval(Ret == EOF); // expected-warning {{TRUE}} + clang_analyzer_eval(errno != 0); // expected-warning {{TRUE}} + } + clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}} + clang_analyzer_eval(ferror(F)); // expected-warning {{UNKNOWN}} +} + +void test_ungetc_EOF(FILE *F, int C) { + int Ret = ungetc(EOF, F); + clang_analyzer_eval(F != NULL); // expected-warning {{TRUE}} + clang_analyzer_eval(Ret == EOF); // expected-warning {{TRUE}} + clang_analyzer_eval(errno != 0); // expected-warning {{TRUE}} + Ret = ungetc(C, F); + if (Ret == EOF) { + clang_analyzer_eval(C == EOF); // expected-warning {{TRUE}} + // expected-warning@-1{{FALSE}} + } +} + void test_feof(FILE *F) { errno = 0; feof(F); diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c index 060d561c1fe1c2..d8026247697a50 100644 --- a/clang/test/Analysis/stream.c +++ b/clang/test/Analysis/stream.c @@ -39,6 +39,12 @@ void check_fputs(void) { fclose(fp); } +void check_ungetc(void) { + FILE *fp = tmpfile(); + ungetc('A', 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 3081d968435de1a3ac948153a4fc6dc22c630655 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= <balazs.k...@ericsson.com> Date: Tue, 9 Jan 2024 10:03:23 +0100 Subject: [PATCH 2/3] updated release notes and code comment --- clang/docs/ReleaseNotes.rst | 8 +++++--- clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp | 3 ++- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 3bfcd7997faa19..64b80a96ef8b44 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -1162,8 +1162,9 @@ Improvements (`c3a87ddad62a <https://github.com/llvm/llvm-project/commit/c3a87ddad62a6cc01acaccc76592bc6730c8ac3c>`_, `0954dc3fb921 <https://github.com/llvm/llvm-project/commit/0954dc3fb9214b994623f5306473de075f8e3593>`_) -- Improved the ``alpha.unix.Stream`` checker by modeling more functions: - ``fputs``, ``fputc``, ``fgets``, ``fgetc``, ``fdopen``, ``ungetc``, ``fflush``. +- Improved the ``alpha.unix.Stream`` checker by modeling more functions + ``fputs``, ``fputc``, ``fgets``, ``fgetc``, ``fdopen``, ``ungetc``, ``fflush`` + and no not recognize alternative ``fopen`` and ``tmpfile`` implementations. (`#76776 <https://github.com/llvm/llvm-project/pull/76776>`_, `#74296 <https://github.com/llvm/llvm-project/pull/74296>`_, `#73335 <https://github.com/llvm/llvm-project/pull/73335>`_, @@ -1171,7 +1172,8 @@ Improvements `#71518 <https://github.com/llvm/llvm-project/pull/71518>`_, `#72016 <https://github.com/llvm/llvm-project/pull/72016>`_, `#70540 <https://github.com/llvm/llvm-project/pull/70540>`_, - `#73638 <https://github.com/llvm/llvm-project/pull/73638>`_) + `#73638 <https://github.com/llvm/llvm-project/pull/73638>`_, + `#77331 <https://github.com/llvm/llvm-project/pull/77331>`_) - The ``alpha.security.taint.TaintPropagation`` checker no longer propagates taint on ``strlen`` and ``strnlen`` calls, unless these are marked diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp index a3d1f34d3d1a9a..35ef953deafeec 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp @@ -953,7 +953,8 @@ void StreamChecker::evalUngetc(const FnDescription *Desc, const CallEvent &Call, // Failure of 'ungetc' does not result in feof or ferror state. // If the PutVal has value of EofVal the function should "fail", but this is // the same transition as the success state. - // FIXME: Is it possible that StateFailed == StateNotFailed ? + // In this case only one state transition is added by the analyzer (the two + // new states may be similar). ProgramStateRef StateFailed = bindInt(*EofVal, State, C, CE); StreamState NewSS = StreamState::getOpened(Desc); StateFailed = StateFailed->set<StreamMap>(StreamSym, NewSS); >From 92342982497d8ede71436b19cbaad833c4af9887 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= <balazs.k...@ericsson.com> Date: Tue, 9 Jan 2024 10:05:56 +0100 Subject: [PATCH 3/3] removed unneeded variable --- clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp index 35ef953deafeec..fbfa101257d5e1 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp @@ -956,8 +956,8 @@ void StreamChecker::evalUngetc(const FnDescription *Desc, const CallEvent &Call, // In this case only one state transition is added by the analyzer (the two // new states may be similar). ProgramStateRef StateFailed = bindInt(*EofVal, State, C, CE); - StreamState NewSS = StreamState::getOpened(Desc); - StateFailed = StateFailed->set<StreamMap>(StreamSym, NewSS); + StateFailed = + StateFailed->set<StreamMap>(StreamSym, StreamState::getOpened(Desc)); C.addTransition(StateFailed); } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
