From 22d40cc3a724fa3df259c52009571a21a3a3a632 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Bal=C3=A1zs=20K=C3=A9ri?= <1.int32@gmail.com> Date: Mon, 18 May 2020 15:07:32 +0200 Subject: [PATCH] [Analyzer][StreamChecker] Changed representation of stream error state - NFC. Summary: State of error flags for a stream is handled by having separate flags that allow combination of multiple error states to be described with one error state object. After a failed function the error state is set in the stream state and must not be determined later based on the last failed function like before this change. The error state can not always be determined from the last failed function and it was not the best design. Reviewers: Szelethus Reviewed By: Szelethus Subscribers: xazax.hun, baloghadamsoftware, szepet, a.sidorin, mikhail.ramalho, Szelethus, donat.nagy, dkrupp, gamesh411, Charusso, martong, ASDenysPetrov, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D80009 --- .../lib/StaticAnalyzer/Checkers/StreamChecker.cpp | 248 +++++++++------------ 1 file changed, 108 insertions(+), 140 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp index 76dd62d..d079951 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp @@ -19,14 +19,62 @@ #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h" #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h" #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h" +#include using namespace clang; using namespace ento; +using namespace std::placeholders; namespace { struct FnDescription; +/// State of the stream error flags. +/// Sometimes it is not known to the checker what error flags are set. +/// This is indicated by setting more than one flag to true. +/// This is an optimization to avoid state splits. +/// A stream can either be in FEOF or FERROR but not both at the same time. +/// Multiple flags are set to handle the corresponding states together. +struct StreamErrorState { + /// The stream can be in state where none of the error flags set. + bool NoError = true; + /// The stream can be in state where the EOF indicator is set. + bool FEof = false; + /// The stream can be in state where the error indicator is set. + bool FError = false; + + bool isNoError() const { return NoError && !FEof && !FError; } + bool isFEof() const { return !NoError && FEof && !FError; } + bool isFError() const { return !NoError && !FEof && FError; } + + bool operator==(const StreamErrorState &ES) const { + return NoError == ES.NoError && FEof == ES.FEof && FError == ES.FError; + } + + StreamErrorState operator|(const StreamErrorState &E) const { + return {NoError || E.NoError, FEof || E.FEof, FError || E.FError}; + } + + StreamErrorState operator&(const StreamErrorState &E) const { + return {NoError && E.NoError, FEof && E.FEof, FError && E.FError}; + } + + StreamErrorState operator~() const { return {!NoError, !FEof, !FError}; } + + /// Returns if the StreamErrorState is a valid object. + operator bool() const { return NoError || FEof || FError; } + + void Profile(llvm::FoldingSetNodeID &ID) const { + ID.AddBoolean(NoError); + ID.AddBoolean(FEof); + ID.AddBoolean(FError); + } +}; + +const StreamErrorState ErrorNone{true, false, false}; +const StreamErrorState ErrorFEof{false, true, false}; +const StreamErrorState ErrorFError{false, false, true}; + /// Full state information about a stream pointer. struct StreamState { /// The last file operation called in the stream. @@ -40,53 +88,24 @@ struct StreamState { OpenFailed /// The last open operation has failed. } State; - /// The error state of a stream. - /// Valid only if the stream is opened. - /// It is assumed that feof and ferror flags are never true at the same time. - enum ErrorKindTy { - /// No error flag is set (or stream is not open). - NoError, - /// EOF condition (`feof` is true). - FEof, - /// Other generic (non-EOF) error (`ferror` is true). - FError, - /// Unknown error flag is set (or none), the meaning depends on the last - /// operation. - Unknown - } ErrorState = NoError; + /// State of the error flags. + /// Ignored in non-opened stream state but must be NoError. + StreamErrorState ErrorState; bool isOpened() const { return State == Opened; } bool isClosed() const { return State == Closed; } bool isOpenFailed() const { return State == OpenFailed; } - bool isNoError() const { - assert(State == Opened && "Error undefined for closed stream."); - return ErrorState == NoError; - } - bool isFEof() const { - assert(State == Opened && "Error undefined for closed stream."); - return ErrorState == FEof; - } - bool isFError() const { - assert(State == Opened && "Error undefined for closed stream."); - return ErrorState == FError; - } - bool isUnknown() const { - assert(State == Opened && "Error undefined for closed stream."); - return ErrorState == Unknown; - } - bool operator==(const StreamState &X) const { - // In not opened state error should always NoError. + // In not opened state error state should always NoError, so comparison + // here is no problem. return LastOperation == X.LastOperation && State == X.State && ErrorState == X.ErrorState; } - static StreamState getOpened(const FnDescription *L) { - return StreamState{L, Opened}; - } - static StreamState getOpened(const FnDescription *L, ErrorKindTy E) { - return StreamState{L, Opened, E}; + static StreamState getOpened(const FnDescription *L, + const StreamErrorState &ES = {}) { + return StreamState{L, Opened, ES}; } static StreamState getClosed(const FnDescription *L) { return StreamState{L, Closed}; @@ -95,15 +114,6 @@ struct StreamState { return StreamState{L, OpenFailed}; } - /// Return if the specified error kind is possible on the stream in the - /// current state. - /// This depends on the stored `LastOperation` value. - /// If the error is not possible returns empty value. - /// If the error is possible returns the remaining possible error type - /// (after taking out `ErrorKind`). If a single error is possible it will - /// return that value, otherwise unknown error. - Optional getRemainingPossibleError(ErrorKindTy ErrorKind) const; - void Profile(llvm::FoldingSetNodeID &ID) const { ID.AddPointer(LastOperation); ID.AddInteger(State); @@ -122,28 +132,8 @@ struct FnDescription { FnCheck PreFn; FnCheck EvalFn; ArgNoTy StreamArgNo; - // What errors are possible after this operation. - // Used only if this operation resulted in Unknown state - // (otherwise there is a known single error). - // Must contain 2 or 3 elements, or zero. - llvm::SmallVector PossibleErrors = {}; }; -Optional -StreamState::getRemainingPossibleError(ErrorKindTy ErrorKind) const { - assert(ErrorState == Unknown && - "Function to be used only if error is unknown."); - llvm::SmallVector NewPossibleErrors; - for (StreamState::ErrorKindTy E : LastOperation->PossibleErrors) - if (E != ErrorKind) - NewPossibleErrors.push_back(E); - if (NewPossibleErrors.size() == LastOperation->PossibleErrors.size()) - return {}; - if (NewPossibleErrors.size() == 1) - return NewPossibleErrors.front(); - return Unknown; -} - /// Get the value of the stream argument out of the passed call event. /// The call should contain a function that is described by Desc. SVal getStreamArg(const FnDescription *Desc, const CallEvent &Call) { @@ -193,49 +183,42 @@ public: private: CallDescriptionMap FnDescriptions = { - {{"fopen"}, {nullptr, &StreamChecker::evalFopen, ArgNone, {}}}, + {{"fopen"}, {nullptr, &StreamChecker::evalFopen, ArgNone}}, {{"freopen", 3}, - {&StreamChecker::preFreopen, &StreamChecker::evalFreopen, 2, {}}}, - {{"tmpfile"}, {nullptr, &StreamChecker::evalFopen, ArgNone, {}}}, + {&StreamChecker::preFreopen, &StreamChecker::evalFreopen, 2}}, + {{"tmpfile"}, {nullptr, &StreamChecker::evalFopen, ArgNone}}, {{"fclose", 1}, - {&StreamChecker::preDefault, &StreamChecker::evalFclose, 0, {}}}, - {{"fread", 4}, {&StreamChecker::preDefault, nullptr, 3, {}}}, - {{"fwrite", 4}, {&StreamChecker::preDefault, nullptr, 3, {}}}, - {{"fseek", 3}, - {&StreamChecker::preFseek, - &StreamChecker::evalFseek, - 0, - {StreamState::FEof, StreamState::FError, StreamState::NoError}}}, - {{"ftell", 1}, {&StreamChecker::preDefault, nullptr, 0, {}}}, - {{"rewind", 1}, {&StreamChecker::preDefault, nullptr, 0, {}}}, - {{"fgetpos", 2}, {&StreamChecker::preDefault, nullptr, 0, {}}}, - {{"fsetpos", 2}, {&StreamChecker::preDefault, nullptr, 0, {}}}, + {&StreamChecker::preDefault, &StreamChecker::evalFclose, 0}}, + {{"fread", 4}, {&StreamChecker::preDefault, nullptr, 3}}, + {{"fwrite", 4}, {&StreamChecker::preDefault, nullptr, 3}}, + {{"fseek", 3}, {&StreamChecker::preFseek, &StreamChecker::evalFseek, 0}}, + {{"ftell", 1}, {&StreamChecker::preDefault, nullptr, 0}}, + {{"rewind", 1}, {&StreamChecker::preDefault, nullptr, 0}}, + {{"fgetpos", 2}, {&StreamChecker::preDefault, nullptr, 0}}, + {{"fsetpos", 2}, {&StreamChecker::preDefault, nullptr, 0}}, {{"clearerr", 1}, - {&StreamChecker::preDefault, &StreamChecker::evalClearerr, 0, {}}}, - // Note: feof can result in Unknown if at the call there is a - // PossibleErrors with all 3 error states (including NoError). - // Then if feof is false the remaining error could be FError or NoError. + {&StreamChecker::preDefault, &StreamChecker::evalClearerr, 0}}, {{"feof", 1}, {&StreamChecker::preDefault, - &StreamChecker::evalFeofFerror, - 0, - {StreamState::FError, StreamState::NoError}}}, - // Note: ferror can result in Unknown if at the call there is a - // PossibleErrors with all 3 error states (including NoError). - // Then if ferror is false the remaining error could be FEof or NoError. + std::bind(&StreamChecker::evalFeofFerror, _1, _2, _3, _4, ErrorFEof), + 0}}, {{"ferror", 1}, {&StreamChecker::preDefault, - &StreamChecker::evalFeofFerror, - 0, - {StreamState::FEof, StreamState::NoError}}}, - {{"fileno", 1}, {&StreamChecker::preDefault, nullptr, 0, {}}}, + std::bind(&StreamChecker::evalFeofFerror, _1, _2, _3, _4, ErrorFError), + 0}}, + {{"fileno", 1}, {&StreamChecker::preDefault, nullptr, 0}}, }; CallDescriptionMap FnTestDescriptions = { {{"StreamTesterChecker_make_feof_stream", 1}, - {nullptr, &StreamChecker::evalSetFeofFerror, 0}}, + {nullptr, + std::bind(&StreamChecker::evalSetFeofFerror, _1, _2, _3, _4, ErrorFEof), + 0}}, {{"StreamTesterChecker_make_ferror_stream", 1}, - {nullptr, &StreamChecker::evalSetFeofFerror, 0}}, + {nullptr, + std::bind(&StreamChecker::evalSetFeofFerror, _1, _2, _3, _4, + ErrorFError), + 0}}, }; void evalFopen(const FnDescription *Desc, const CallEvent &Call, @@ -260,13 +243,13 @@ private: void evalClearerr(const FnDescription *Desc, const CallEvent &Call, CheckerContext &C) const; - template void evalFeofFerror(const FnDescription *Desc, const CallEvent &Call, - CheckerContext &C) const; + CheckerContext &C, + const StreamErrorState &ErrorKind) const; - template void evalSetFeofFerror(const FnDescription *Desc, const CallEvent &Call, - CheckerContext &C) const; + CheckerContext &C, + const StreamErrorState &ErrorKind) const; /// Check that the stream (in StreamVal) is not NULL. /// If it can only be NULL a fatal error is emitted and nullptr returned. @@ -482,8 +465,10 @@ void StreamChecker::evalFseek(const FnDescription *Desc, const CallEvent &Call, StateNotFailed = StateNotFailed->set(StreamSym, StreamState::getOpened(Desc)); // We get error. + // It is possible that fseek fails but sets none of the error flags. StateFailed = StateFailed->set( - StreamSym, StreamState::getOpened(Desc, StreamState::Unknown)); + StreamSym, + StreamState::getOpened(Desc, ErrorNone | ErrorFEof | ErrorFError)); C.addTransition(StateNotFailed); C.addTransition(StateFailed); @@ -507,10 +492,9 @@ void StreamChecker::evalClearerr(const FnDescription *Desc, C.addTransition(State); } -template void StreamChecker::evalFeofFerror(const FnDescription *Desc, - const CallEvent &Call, - CheckerContext &C) const { + const CallEvent &Call, CheckerContext &C, + const StreamErrorState &ErrorKind) const { ProgramStateRef State = C.getState(); SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol(); if (!StreamSym) @@ -520,42 +504,27 @@ void StreamChecker::evalFeofFerror(const FnDescription *Desc, if (!CE) return; - auto AddTransition = [&C, Desc, StreamSym](ProgramStateRef State, - StreamState::ErrorKindTy SSError) { - StreamState SSNew = StreamState::getOpened(Desc, SSError); - State = State->set(StreamSym, SSNew); - C.addTransition(State); - }; - const StreamState *SS = State->get(StreamSym); if (!SS) return; assertStreamStateOpened(SS); - if (!SS->isUnknown()) { - // The stream is in exactly known state (error or not). - // Check if it is in error of kind `ErrorKind`. - if (SS->ErrorState == ErrorKind) - State = bindAndAssumeTrue(State, C, CE); - else - State = bindInt(0, State, C, CE); - // Error state is not changed in the new state. - AddTransition(State, SS->ErrorState); - } else { - // Stream is in unknown state, check if error `ErrorKind` is possible. - Optional NewError = - SS->getRemainingPossibleError(ErrorKind); - if (!NewError) { - // This kind of error is not possible, function returns zero. - // Error state remains unknown. - AddTransition(bindInt(0, State, C, CE), StreamState::Unknown); - } else { - // Add state with true returned. - AddTransition(bindAndAssumeTrue(State, C, CE), ErrorKind); - // Add state with false returned and the new remaining error type. - AddTransition(bindInt(0, State, C, CE), *NewError); - } + if (SS->ErrorState & ErrorKind) { + // Execution path with error of ErrorKind. + // Function returns true. + // From now on it is the only one error state. + ProgramStateRef TrueState = bindAndAssumeTrue(State, C, CE); + C.addTransition(TrueState->set( + StreamSym, StreamState::getOpened(Desc, ErrorKind))); + } + if (StreamErrorState NewES = SS->ErrorState & (~ErrorKind)) { + // Execution path(s) with ErrorKind not set. + // Function returns false. + // New error state is everything before minus ErrorKind. + ProgramStateRef FalseState = bindInt(0, State, C, CE); + C.addTransition(FalseState->set( + StreamSym, StreamState::getOpened(Desc, NewES))); } } @@ -573,17 +542,16 @@ void StreamChecker::preDefault(const FnDescription *Desc, const CallEvent &Call, C.addTransition(State); } -template void StreamChecker::evalSetFeofFerror(const FnDescription *Desc, - const CallEvent &Call, - CheckerContext &C) const { + const CallEvent &Call, CheckerContext &C, + const StreamErrorState &ErrorKind) const { ProgramStateRef State = C.getState(); SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol(); assert(StreamSym && "Operation not permitted on non-symbolic stream value."); const StreamState *SS = State->get(StreamSym); assert(SS && "Stream should be tracked by the checker."); - State = State->set(StreamSym, - StreamState::getOpened(SS->LastOperation, EK)); + State = State->set( + StreamSym, StreamState::getOpened(SS->LastOperation, ErrorKind)); C.addTransition(State); } -- 2.7.4