llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-static-analyzer-1 Author: Benedek Kaibas (benedekaibas) <details> <summary>Changes</summary> The `SimpleStreamChecker` is a demo checker that was implemented for the 2012 LLVM Developers' Meeting talk. The checker served as an educational checker for newcomers, students, etc. to the Clang Static Analyzer. Since then there has been a lots of improvements within CSA and the `SimpleStreamChecker` lost its purpose because there are other more modern checkers that are great source for newcomers to learn the fundamentals. By removing the checker users do not lose anything since the `StreamChecker` already covers what `SimpleStreamChecker` has covered. Discussion about the removal of this checker started in https://github.com/llvm/llvm-project/pull/230288 --- Full diff: https://github.com/llvm/llvm-project/pull/238415.diff 8 Files Affected: - (modified) clang/docs/analyzer/checkers.md (-24) - (modified) clang/include/clang/StaticAnalyzer/Checkers/Checkers.td (-4) - (modified) clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt (-1) - (removed) clang/lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp (-223) - (modified) clang/test/Analysis/simple-stream-checks.c (+9-9) - (modified) clang/test/Analysis/stream-non-posix-function.c (-1) - (modified) clang/utils/analyzer/SATestBuild.py (-1) - (modified) llvm/utils/gn/secondary/clang/lib/StaticAnalyzer/Checkers/BUILD.gn (-1) ``````````diff diff --git a/clang/docs/analyzer/checkers.md b/clang/docs/analyzer/checkers.md index e2bbf52943bb87..bef63c0b0222d0 100644 --- a/clang/docs/analyzer/checkers.md +++ b/clang/docs/analyzer/checkers.md @@ -3774,30 +3774,6 @@ void test() { } ``` -(alpha-unix-simplestream)= - -#### alpha.unix.SimpleStream (C) - -Check for misuses of stream APIs. Check for misuses of stream APIs: `fopen, fclose` -(demo checker, the subject of the demo ([Slides](https://llvm.org/devmtg/2012-11/Zaks-Rose-Checker24Hours.pdf) , -[Video](https://youtu.be/kdxlsP5QVPw)) by Anna Zaks and Jordan Rose presented at the -[2012 LLVM Developers' Meeting](https://llvm.org/devmtg/2012-11/)). - -```c -void test() { - FILE *F = fopen("myfile.txt", "w"); -} // warn: opened file is never closed - -void test() { - FILE *F = fopen("myfile.txt", "w"); - - if (F) - fclose(F); - - fclose(F); // warn: closing a previously closed file stream -} -``` - (alpha-unix-cstring-bufferoverlap)= #### alpha.unix.cstring.BufferOverlap (C) diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td index 9905649801e85b..27a12bedd86231 100644 --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -627,10 +627,6 @@ def PthreadLockChecker : Checker<"PthreadLock">, Dependencies<[PthreadLockBase]>, Documentation<HasDocumentation>; -def SimpleStreamChecker : Checker<"SimpleStream">, - HelpText<"Check for misuses of stream APIs">, - Documentation<HasDocumentation>; - } // end "alpha.unix" //===----------------------------------------------------------------------===// diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt index f136bd5434113b..496a9b3fef7648 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -102,7 +102,6 @@ add_clang_library(clangStaticAnalyzerCheckers ReturnValueChecker.cpp RunLoopAutoreleaseLeakChecker.cpp SetgidSetuidOrderChecker.cpp - SimpleStreamChecker.cpp SmartPtrChecker.cpp SmartPtrModeling.cpp StackAddrEscapeChecker.cpp diff --git a/clang/lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp deleted file mode 100644 index 099ef37fd690be..00000000000000 --- a/clang/lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp +++ /dev/null @@ -1,223 +0,0 @@ -//===-- SimpleStreamChecker.cpp -----------------------------------*- C++ -*--// -// -// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. -// See https://llvm.org/LICENSE.txt for license information. -// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception -// -//===----------------------------------------------------------------------===// -// -// Defines a checker for proper use of fopen/fclose APIs. -// - If a file has been closed with fclose, it should not be accessed again. -// Accessing a closed file results in undefined behavior. -// - If a file was opened with fopen, it must be closed with fclose before -// the execution ends. Failing to do so results in a resource leak. -// -//===----------------------------------------------------------------------===// - -#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" -#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" -#include "clang/StaticAnalyzer/Core/Checker.h" -#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h" -#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" -#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" -#include <utility> - -using namespace clang; -using namespace ento; - -namespace { -typedef SmallVector<SymbolRef, 2> SymbolVector; - -struct StreamState { -private: - enum Kind { Opened, Closed } K; - StreamState(Kind InK) : K(InK) { } - -public: - bool isOpened() const { return K == Opened; } - bool isClosed() const { return K == Closed; } - - static StreamState getOpened() { return StreamState(Opened); } - static StreamState getClosed() { return StreamState(Closed); } - - bool operator==(const StreamState &X) const { - return K == X.K; - } - void Profile(llvm::FoldingSetNodeID &ID) const { - ID.AddInteger(K); - } -}; - -class SimpleStreamChecker : public Checker<check::PostCall, - check::PreCall, - check::DeadSymbols, - check::PointerEscape> { - const CallDescription OpenFn{CDM::CLibrary, {"fopen"}, 2}; - const CallDescription CloseFn{CDM::CLibrary, {"fclose"}, 1}; - - const BugType DoubleCloseBugType{this, "Double fclose", - "Unix Stream API Error"}; - const BugType LeakBugType{this, "Resource Leak", "Unix Stream API Error", - /*SuppressOnSink=*/true}; - - void reportDoubleClose(SymbolRef FileDescSym, - const CallEvent &Call, - CheckerContext &C) const; - - void reportLeaks(ArrayRef<SymbolRef> LeakedStreams, CheckerContext &C, - ExplodedNode *ErrNode) const; - -public: - /// Process fopen. - void checkPostCall(const CallEvent &Call, CheckerContext &C) const; - /// Process fclose. - void checkPreCall(const CallEvent &Call, CheckerContext &C) const; - - void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const; - - /// Stop tracking addresses which escape. - ProgramStateRef checkPointerEscape(ProgramStateRef State, - const InvalidatedSymbols &Escaped, - const CallEvent *Call, - PointerEscapeKind Kind) const; -}; - -} // end anonymous namespace - -/// The state of the checker is a map from tracked stream symbols to their -/// state. Let's store it in the ProgramState. -REGISTER_MAP_WITH_PROGRAMSTATE(StreamMap, SymbolRef, StreamState) - -void SimpleStreamChecker::checkPostCall(const CallEvent &Call, - CheckerContext &C) const { - if (!OpenFn.matches(Call)) - return; - - // Get the symbolic value corresponding to the file handle. - SymbolRef FileDesc = Call.getReturnValue().getAsSymbol(); - if (!FileDesc) - return; - - // Generate the next transition (an edge in the exploded graph). - ProgramStateRef State = C.getState(); - State = State->set<StreamMap>(FileDesc, StreamState::getOpened()); - C.addTransition(State); -} - -void SimpleStreamChecker::checkPreCall(const CallEvent &Call, - CheckerContext &C) const { - if (!CloseFn.matches(Call)) - return; - - // Get the symbolic value corresponding to the file handle. - SymbolRef FileDesc = Call.getArgSVal(0).getAsSymbol(); - if (!FileDesc) - return; - - // Check if the stream has already been closed. - ProgramStateRef State = C.getState(); - const StreamState *SS = State->get<StreamMap>(FileDesc); - if (SS && SS->isClosed()) { - reportDoubleClose(FileDesc, Call, C); - return; - } - - // Generate the next transition, in which the stream is closed. - State = State->set<StreamMap>(FileDesc, StreamState::getClosed()); - C.addTransition(State); -} - -static bool isLeaked(SymbolRef Sym, const StreamState &SS, - bool IsSymDead, ProgramStateRef State) { - if (IsSymDead && SS.isOpened()) { - // If a symbol is NULL, assume that fopen failed on this path. - // A symbol should only be considered leaked if it is non-null. - ConstraintManager &CMgr = State->getConstraintManager(); - ConditionTruthVal OpenFailed = CMgr.isNull(State, Sym); - return !OpenFailed.isConstrainedTrue(); - } - return false; -} - -void SimpleStreamChecker::checkDeadSymbols(SymbolReaper &SymReaper, - CheckerContext &C) const { - ProgramStateRef State = C.getState(); - SymbolVector LeakedStreams; - StreamMapTy TrackedStreams = State->get<StreamMap>(); - for (auto [Sym, StreamStatus] : TrackedStreams) { - bool IsSymDead = SymReaper.isDead(Sym); - - // Collect leaked symbols. - if (isLeaked(Sym, StreamStatus, IsSymDead, State)) - LeakedStreams.push_back(Sym); - - // Remove the dead symbol from the streams map. - if (IsSymDead) - State = State->remove<StreamMap>(Sym); - } - - ExplodedNode *N = C.generateNonFatalErrorNode(State); - if (!N) - return; - reportLeaks(LeakedStreams, C, N); -} - -void SimpleStreamChecker::reportDoubleClose(SymbolRef FileDescSym, - const CallEvent &Call, - CheckerContext &C) const { - // We reached a bug, stop exploring the path here by generating a sink. - ExplodedNode *ErrNode = C.addSink(); - // If we've already reached this node on another path, return. - if (!ErrNode) - return; - - // Generate the report. - auto R = std::make_unique<PathSensitiveBugReport>( - DoubleCloseBugType, "Closing a previously closed file stream", ErrNode); - R->addRange(Call.getSourceRange()); - R->markInteresting(FileDescSym); - C.emitReport(std::move(R)); -} - -void SimpleStreamChecker::reportLeaks(ArrayRef<SymbolRef> LeakedStreams, - CheckerContext &C, - ExplodedNode *ErrNode) const { - // Attach bug reports to the leak node. - // TODO: Identify the leaked file descriptor. - for (SymbolRef LeakedStream : LeakedStreams) { - auto R = std::make_unique<PathSensitiveBugReport>( - LeakBugType, "Opened file is never closed; potential resource leak", - ErrNode); - R->markInteresting(LeakedStream); - C.emitReport(std::move(R)); - } -} - -// If the pointer we are tracking escaped, do not track the symbol as -// we cannot reason about it anymore. -ProgramStateRef -SimpleStreamChecker::checkPointerEscape(ProgramStateRef State, - const InvalidatedSymbols &Escaped, - const CallEvent *Call, - PointerEscapeKind Kind) const { - // If we know that the call cannot close a file, there is nothing to do. - if (Kind == PSK_DirectEscapeOnCall && Call->isInSystemHeader()) { - return State; - } - - for (SymbolRef Sym : Escaped) { - // The symbol escaped. Optimistically, assume that the corresponding file - // handle will be closed somewhere else. - State = State->remove<StreamMap>(Sym); - } - return State; -} - -void ento::registerSimpleStreamChecker(CheckerManager &mgr) { - mgr.registerChecker<SimpleStreamChecker>(); -} - -// This checker should be enabled regardless of how language options are set. -bool ento::shouldRegisterSimpleStreamChecker(const CheckerManager &mgr) { - return true; -} diff --git a/clang/test/Analysis/simple-stream-checks.c b/clang/test/Analysis/simple-stream-checks.c index 767ffaf53fcbfd..4ad58e671d1a65 100644 --- a/clang/test/Analysis/simple-stream-checks.c +++ b/clang/test/Analysis/simple-stream-checks.c @@ -1,4 +1,4 @@ -// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.unix.SimpleStream -verify %s +// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Stream -verify %s #include "Inputs/system-header-simulator-for-simple-stream.h" @@ -10,7 +10,7 @@ void checkDoubleFClose(int *Data) { fclose(F); else fputc(*Data, F); - fclose(F); // expected-warning {{Closing a previously closed file stream}} + fclose(F); // expected-warning {{Use of a stream that might be already closed}} } } @@ -20,7 +20,7 @@ int checkLeak(int *Data) { fputs ("fopen example", F); } - if (Data) // expected-warning {{Opened file is never closed; potential resource leak}} + if (Data) // expected-warning {{Opened stream never closed. Potential resource leak}} return *Data; else return 0; @@ -45,11 +45,11 @@ void CloseOnlyOnValidFileHandle(void) { void leakOnEnfOfPath1(int *Data) { FILE *F = fopen("myfile.txt", "w"); -} // expected-warning {{Opened file is never closed; potential resource leak}} +} // expected-warning {{Opened stream never closed. Potential resource leak}} void leakOnEnfOfPath2(int *Data) { FILE *F = fopen("myfile.txt", "w"); - return; // expected-warning {{Opened file is never closed; potential resource leak}} + return; // expected-warning {{Opened stream never closed. Potential resource leak}} } FILE *leakOnEnfOfPath3(int *Data) { @@ -73,15 +73,15 @@ void SymbolEscapedThroughAssignmentToGlobal(void) { void SymbolDoesNotEscapeThoughStringAPIs(char *Data) { FILE *F = fopen("myfile.txt", "w"); - fputc(*Data, F); - return; // expected-warning {{Opened file is never closed; potential resource leak}} + fputc(*Data, F); // expected-warning {{Stream pointer might be NULL}} + return; // expected-warning {{Opened stream never closed. Potential resource leak}} } void passConstPointer(const FILE * F); void testPassConstPointer(void) { FILE *F = fopen("myfile.txt", "w"); passConstPointer(F); - return; // expected-warning {{Opened file is never closed; potential resource leak}} + return; // expected-warning {{Opened stream never closed. Potential resource leak}} } void testPassToSystemHeaderFunctionIndirectly(void) { @@ -93,4 +93,4 @@ void testPassToSystemHeaderFunctionIndirectly(void) { void testOverwrite(void) { FILE *fp = fopen("myfile.txt", "w"); fp = 0; -} // expected-warning {{Opened file is never closed; potential resource leak}} +} // expected-warning {{Opened stream never closed. Potential resource leak}} diff --git a/clang/test/Analysis/stream-non-posix-function.c b/clang/test/Analysis/stream-non-posix-function.c index b9ece31cde1568..9c5308424ade4d 100644 --- a/clang/test/Analysis/stream-non-posix-function.c +++ b/clang/test/Analysis/stream-non-posix-function.c @@ -1,5 +1,4 @@ // RUN: %clang_analyze_cc1 -fno-builtin -analyzer-checker=core,unix.Stream -verify %s -// RUN: %clang_analyze_cc1 -fno-builtin -analyzer-checker=core,alpha.unix.SimpleStream -verify %s // expected-no-diagnostics diff --git a/clang/utils/analyzer/SATestBuild.py b/clang/utils/analyzer/SATestBuild.py index 66e1ab72985cd9..2bf82bfcd757c3 100644 --- a/clang/utils/analyzer/SATestBuild.py +++ b/clang/utils/analyzer/SATestBuild.py @@ -175,7 +175,6 @@ def stdout(message: str): # checkers we don't want to regress on. CHECKERS = ",".join( [ - "alpha.unix.SimpleStream", "optin.taint", "cplusplus.NewDeleteLeaks", "core", diff --git a/llvm/utils/gn/secondary/clang/lib/StaticAnalyzer/Checkers/BUILD.gn b/llvm/utils/gn/secondary/clang/lib/StaticAnalyzer/Checkers/BUILD.gn index f048acd18d904e..69d95cbbee2cd8 100644 --- a/llvm/utils/gn/secondary/clang/lib/StaticAnalyzer/Checkers/BUILD.gn +++ b/llvm/utils/gn/secondary/clang/lib/StaticAnalyzer/Checkers/BUILD.gn @@ -111,7 +111,6 @@ static_library("Checkers") { "RunLoopAutoreleaseLeakChecker.cpp", "STLAlgorithmModeling.cpp", "SetgidSetuidOrderChecker.cpp", - "SimpleStreamChecker.cpp", "SmartPtrChecker.cpp", "SmartPtrModeling.cpp", "StackAddrEscapeChecker.cpp", `````````` </details> https://github.com/llvm/llvm-project/pull/238415 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
