This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. balazske marked an inline comment as done. Closed by commit rG60f3b071185b: [clang][analyzer] Add checker for bad use of 'errno'. (authored by balazske).
Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122150/new/ https://reviews.llvm.org/D122150 Files: clang/docs/ReleaseNotes.rst clang/docs/analyzer/checkers.rst clang/include/clang/StaticAnalyzer/Checkers/Checkers.td clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h clang/lib/StaticAnalyzer/Checkers/ErrnoTesterChecker.cpp clang/test/Analysis/analyzer-config.c clang/test/Analysis/errno-notes.c clang/test/Analysis/errno-options.c clang/test/Analysis/errno.c
Index: clang/test/Analysis/errno.c =================================================================== --- clang/test/Analysis/errno.c +++ clang/test/Analysis/errno.c @@ -3,6 +3,7 @@ // RUN: -analyzer-checker=apiModeling.Errno \ // RUN: -analyzer-checker=debug.ExprInspection \ // RUN: -analyzer-checker=debug.ErrnoTest \ +// RUN: -analyzer-checker=alpha.unix.Errno \ // RUN: -DERRNO_VAR // RUN: %clang_analyze_cc1 -verify %s \ @@ -10,8 +11,10 @@ // RUN: -analyzer-checker=apiModeling.Errno \ // RUN: -analyzer-checker=debug.ExprInspection \ // RUN: -analyzer-checker=debug.ErrnoTest \ +// RUN: -analyzer-checker=alpha.unix.Errno \ // RUN: -DERRNO_FUNC +#include "Inputs/system-header-simulator.h" #ifdef ERRNO_VAR #include "Inputs/errno_var.h" #endif @@ -24,6 +27,7 @@ int ErrnoTesterChecker_getErrno(); int ErrnoTesterChecker_setErrnoIfError(); int ErrnoTesterChecker_setErrnoIfErrorRange(); +int ErrnoTesterChecker_setErrnoCheckState(); void something(); @@ -61,3 +65,199 @@ clang_analyzer_eval(errno == 1); // expected-warning{{FALSE}} expected-warning{{TRUE}} } } + +void testErrnoCheck0() { + // If the function returns a success result code, value of 'errno' + // is unspecified and it is unsafe to make any decision with it. + // The function did not promise to not change 'errno' if no failure happens. + int X = ErrnoTesterChecker_setErrnoCheckState(); + if (X == 0) { + if (errno) { // expected-warning{{An undefined value may be read from 'errno' [alpha.unix.Errno]}} + } + if (errno) { // no warning for second time (analysis stops at the first warning) + } + } + X = ErrnoTesterChecker_setErrnoCheckState(); + if (X == 0) { + if (errno) { // expected-warning{{An undefined value may be read from 'errno' [alpha.unix.Errno]}} + } + errno = 0; + } + X = ErrnoTesterChecker_setErrnoCheckState(); + if (X == 0) { + errno = 0; + if (errno) { // no warning after overwritten 'errno' + } + } +} + +void testErrnoCheck1() { + // If the function returns error result code that is out-of-band (not a valid + // non-error return value) the value of 'errno' can be checked but it is not + // required to do so. + int X = ErrnoTesterChecker_setErrnoCheckState(); + if (X == 1) { + if (errno) { // no warning + } + } + X = ErrnoTesterChecker_setErrnoCheckState(); + if (X == 1) { + errno = 0; // no warning + } +} + +void testErrnoCheck2() { + // If the function returns an in-band error result the value of 'errno' is + // required to be checked to verify if error happened. + // The same applies to other functions that can indicate failure only by + // change of 'errno'. + int X = ErrnoTesterChecker_setErrnoCheckState(); + if (X == 2) { + errno = 0; // expected-warning{{Value of 'errno' was not checked and is overwritten here [alpha.unix.Errno]}} + errno = 0; + } + X = ErrnoTesterChecker_setErrnoCheckState(); + if (X == 2) { + errno = 0; // expected-warning{{Value of 'errno' was not checked and is overwritten here [alpha.unix.Errno]}} + if (errno) { + } + } +} + +void testErrnoCheck3() { + int X = ErrnoTesterChecker_setErrnoCheckState(); + if (X == 2) { + if (errno) { + } + errno = 0; // no warning after 'errno' was read + } + X = ErrnoTesterChecker_setErrnoCheckState(); + if (X == 2) { + int A = errno; + errno = 0; // no warning after 'errno' was read + } +} + +void testErrnoCheckUndefinedLoad() { + int X = ErrnoTesterChecker_setErrnoCheckState(); + if (X == 0) { + if (errno) { // expected-warning{{An undefined value may be read from 'errno' [alpha.unix.Errno]}} + } + } +} + +void testErrnoNotCheckedAtSystemCall() { + int X = ErrnoTesterChecker_setErrnoCheckState(); + if (X == 2) { + printf("%i", 1); // expected-warning{{Value of 'errno' was not checked and may be overwritten by function 'printf' [alpha.unix.Errno]}} + printf("%i", 1); // no warning ('printf' does not change errno state) + } +} + +void testErrnoCheckStateInvalidate() { + int X = ErrnoTesterChecker_setErrnoCheckState(); + if (X == 0) { + something(); + if (errno) { // no warning after an invalidating function call + } + } + X = ErrnoTesterChecker_setErrnoCheckState(); + if (X == 0) { + printf("%i", 1); + if (errno) { // no warning after an invalidating standard function call + } + } +} + +void testErrnoCheckStateInvalidate1() { + int X = ErrnoTesterChecker_setErrnoCheckState(); + if (X == 2) { + clang_analyzer_eval(errno); // expected-warning{{TRUE}} + something(); + clang_analyzer_eval(errno); // expected-warning{{UNKNOWN}} + errno = 0; // no warning after invalidation + } +} + +void test_if_cond_in_expr() { + ErrnoTesterChecker_setErrnoIfError(); + if (errno + 10 > 2) { + // expected-warning@-1{{An undefined value may be read from 'errno'}} + } +} + +void test_for_cond() { + ErrnoTesterChecker_setErrnoIfError(); + for (; errno != 0;) { + // expected-warning@-1{{An undefined value may be read from 'errno'}} + } +} + +void test_do_cond() { + ErrnoTesterChecker_setErrnoIfError(); + do { + } while (errno != 0); + // expected-warning@-1{{An undefined value may be read from 'errno'}} +} + +void test_while_cond() { + ErrnoTesterChecker_setErrnoIfError(); + while (errno != 0) { + // expected-warning@-1{{An undefined value may be read from 'errno'}} + } +} + +void test_switch_cond() { + ErrnoTesterChecker_setErrnoIfError(); + switch (errno) {} + // expected-warning@-1{{An undefined value may be read from 'errno'}} +} + +void test_conditional_cond() { + ErrnoTesterChecker_setErrnoIfError(); + int A = errno ? 1 : 2; + // expected-warning@-1{{An undefined value may be read from 'errno'}} +} + +void test_binary_conditional_cond() { + ErrnoTesterChecker_setErrnoIfError(); + int A = errno ?: 2; + // expected-warning@-1{{An undefined value may be read from 'errno'}} +} + +void test_errno_store_into_variable() { + ErrnoTesterChecker_setErrnoIfError(); + int a = errno; // AllowNonConditionErrnoRead is on by default, no warning +} + +void test_errno_store_into_variable_in_expr() { + ErrnoTesterChecker_setErrnoIfError(); + int a = errno > 1; // AllowNonConditionErrnoRead is on by default, no warning +} + +int test_errno_return() { + ErrnoTesterChecker_setErrnoIfError(); + return errno; +} + +void test_errno_pointer1() { + ErrnoTesterChecker_setErrnoIfError(); + int *ErrnoP = &errno; + int A = errno ? 1 : 2; + // expected-warning@-1{{An undefined value may be read from 'errno'}} +} + +void test_errno_pointer2() { + ErrnoTesterChecker_setErrnoIfError(); + int *ErrnoP = &errno; + int A = (*ErrnoP) ? 1 : 2; + // expected-warning@-1{{An undefined value may be read from 'errno'}} +} + +int f(int); + +void test_errno_in_condition_in_function_call() { + ErrnoTesterChecker_setErrnoIfError(); + if (f(errno) != 0) { + } +} Index: clang/test/Analysis/errno-options.c =================================================================== --- /dev/null +++ clang/test/Analysis/errno-options.c @@ -0,0 +1,55 @@ +// RUN: %clang_analyze_cc1 -verify %s \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=apiModeling.Errno \ +// RUN: -analyzer-checker=debug.ErrnoTest \ +// RUN: -analyzer-checker=alpha.unix.Errno \ +// RUN: -analyzer-config alpha.unix.Errno:AllowErrnoReadOutsideConditionExpressions=false \ +// RUN: -DERRNO_VAR + +// RUN: %clang_analyze_cc1 -verify %s \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=apiModeling.Errno \ +// RUN: -analyzer-checker=debug.ErrnoTest \ +// RUN: -analyzer-checker=alpha.unix.Errno \ +// RUN: -analyzer-config alpha.unix.Errno:AllowErrnoReadOutsideConditionExpressions=false \ +// RUN: -DERRNO_FUNC + +#include "Inputs/system-header-simulator.h" +#ifdef ERRNO_VAR +#include "Inputs/errno_var.h" +#endif +#ifdef ERRNO_FUNC +#include "Inputs/errno_func.h" +#endif + +int ErrnoTesterChecker_setErrnoIfError(); + +void test_cond() { + ErrnoTesterChecker_setErrnoIfError(); + int A = errno ? 1 : 2; + // expected-warning@-1{{An undefined value may be read from 'errno'}} +} + +void test_errno_store_into_variable() { + ErrnoTesterChecker_setErrnoIfError(); + int a = errno; + // expected-warning@-1{{An undefined value may be read from 'errno'}} +} + +void test_errno_store_into_variable_in_expr() { + ErrnoTesterChecker_setErrnoIfError(); + int a = 4 + errno; + // expected-warning@-1{{An undefined value may be read from 'errno'}} +} + +int test_errno_return() { + ErrnoTesterChecker_setErrnoIfError(); + return errno; + // expected-warning@-1{{An undefined value may be read from 'errno'}} +} + +int test_errno_return_expr() { + ErrnoTesterChecker_setErrnoIfError(); + return errno > 10; + // expected-warning@-1{{An undefined value may be read from 'errno'}} +} Index: clang/test/Analysis/errno-notes.c =================================================================== --- /dev/null +++ clang/test/Analysis/errno-notes.c @@ -0,0 +1,62 @@ +// RUN: %clang_analyze_cc1 -verify -analyzer-output text %s \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=apiModeling.Errno \ +// RUN: -analyzer-checker=debug.ExprInspection \ +// RUN: -analyzer-checker=debug.ErrnoTest \ +// RUN: -analyzer-checker=alpha.unix.Errno \ +// RUN: -DERRNO_VAR + +// RUN: %clang_analyze_cc1 -verify -analyzer-output text %s \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=apiModeling.Errno \ +// RUN: -analyzer-checker=debug.ExprInspection \ +// RUN: -analyzer-checker=debug.ErrnoTest \ +// RUN: -analyzer-checker=alpha.unix.Errno \ +// RUN: -DERRNO_FUNC + +#include "Inputs/errno_var.h" +#include "Inputs/system-header-simulator.h" +#ifdef ERRNO_VAR +#include "Inputs/errno_var.h" +#endif +#ifdef ERRNO_FUNC +#include "Inputs/errno_func.h" +#endif + +int ErrnoTesterChecker_setErrnoCheckState(); + +void something(); + +void testErrnoCheckUndefRead() { + int X = ErrnoTesterChecker_setErrnoCheckState(); + something(); + X = ErrnoTesterChecker_setErrnoCheckState(); // expected-note{{Assuming that this function succeeds but sets 'errno' to an unspecified value}} + if (X == 0) { // expected-note{{'X' is equal to 0}} + // expected-note@-1{{Taking true branch}} + if (errno) { + } // expected-warning@-1{{An undefined value may be read from 'errno'}} + // expected-note@-2{{An undefined value may be read from 'errno'}} + } +} + +void testErrnoCheckOverwrite() { + int X = ErrnoTesterChecker_setErrnoCheckState(); + something(); + X = ErrnoTesterChecker_setErrnoCheckState(); // expected-note{{Assuming that this function returns 2. 'errno' should be checked to test for failure}} + if (X == 2) { // expected-note{{'X' is equal to 2}} + // expected-note@-1{{Taking true branch}} + errno = 0; // expected-warning{{Value of 'errno' was not checked and is overwritten here}} + // expected-note@-1{{Value of 'errno' was not checked and is overwritten here}} + } +} + +void testErrnoCheckOverwriteStdCall() { + int X = ErrnoTesterChecker_setErrnoCheckState(); + something(); + X = ErrnoTesterChecker_setErrnoCheckState(); // expected-note{{Assuming that this function returns 2. 'errno' should be checked to test for failure}} + if (X == 2) { // expected-note{{'X' is equal to 2}} + // expected-note@-1{{Taking true branch}} + printf(""); // expected-warning{{Value of 'errno' was not checked and may be overwritten by function 'printf'}} + // expected-note@-1{{Value of 'errno' was not checked and may be overwritten by function 'printf'}} + } +} Index: clang/test/Analysis/analyzer-config.c =================================================================== --- clang/test/Analysis/analyzer-config.c +++ clang/test/Analysis/analyzer-config.c @@ -12,6 +12,7 @@ // CHECK-NEXT: alpha.security.MmapWriteExec:MmapProtExec = 0x04 // CHECK-NEXT: alpha.security.MmapWriteExec:MmapProtRead = 0x01 // CHECK-NEXT: alpha.security.taint.TaintPropagation:Config = "" +// CHECK-NEXT: alpha.unix.Errno:AllowErrnoReadOutsideConditionExpressions = true // CHECK-NEXT: apiModeling.StdCLibraryFunctions:DisplayLoadedSummaries = false // CHECK-NEXT: apiModeling.StdCLibraryFunctions:ModelPOSIX = false // CHECK-NEXT: apply-fixits = false Index: clang/lib/StaticAnalyzer/Checkers/ErrnoTesterChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/ErrnoTesterChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/ErrnoTesterChecker.cpp @@ -20,6 +20,7 @@ using namespace clang; using namespace ento; +using namespace errno_modeling; namespace { @@ -28,11 +29,43 @@ bool evalCall(const CallEvent &Call, CheckerContext &C) const; private: + /// Evaluate function \code void ErrnoTesterChecker_setErrno(int) \endcode. + /// Set value of \c errno to the argument. static void evalSetErrno(CheckerContext &C, const CallEvent &Call); + /// Evaluate function \code int ErrnoTesterChecker_getErrno() \endcode. + /// Return the value of \c errno. static void evalGetErrno(CheckerContext &C, const CallEvent &Call); + /// Evaluate function \code int ErrnoTesterChecker_setErrnoIfError() \endcode. + /// Simulate a standard library function tha returns 0 on success and 1 on + /// failure. On the success case \c errno is not allowed to be used (may be + /// undefined). On the failure case \c errno is set to a fixed value 11 and + /// is not needed to be checked. static void evalSetErrnoIfError(CheckerContext &C, const CallEvent &Call); + /// Evaluate function \code int ErrnoTesterChecker_setErrnoIfErrorRange() + /// \endcode. Same as \c ErrnoTesterChecker_setErrnoIfError but \c errno is + /// set to a range (to be nonzero) at the failure case. static void evalSetErrnoIfErrorRange(CheckerContext &C, const CallEvent &Call); + /// Evaluate function \code int ErrnoTesterChecker_setErrnoCheckState() + /// \endcode. This function simulates the following: + /// - Return 0 and leave \c errno with undefined value. + /// This is the case of a successful standard function call. + /// For example if \c ftell returns not -1. + /// - Return 1 and sets \c errno to a specific error code (1). + /// This is the case of a failed standard function call. + /// The function indicates the failure by a special return value + /// that is returned only at failure. + /// \c errno can be checked but it is not required. + /// For example if \c ftell returns -1. + /// - Return 2 and may set errno to a value (actually it does not set it). + /// This is the case of a standard function call where the failure can only + /// be checked by reading from \c errno. The value of \c errno is changed by + /// the function only at failure, the user should set \c errno to 0 before + /// the call (\c ErrnoChecker does not check for this rule). + /// \c strtol is an example of this case, if it returns \c LONG_MIN (or + /// \c LONG_MAX). This case applies only if \c LONG_MIN or \c LONG_MAX is + /// returned, otherwise the first case in this list applies. + static void evalSetErrnoCheckState(CheckerContext &C, const CallEvent &Call); using EvalFn = std::function<void(CheckerContext &, const CallEvent &)>; const CallDescriptionMap<EvalFn> TestCalls{ @@ -41,22 +74,24 @@ {{"ErrnoTesterChecker_setErrnoIfError", 0}, &ErrnoTesterChecker::evalSetErrnoIfError}, {{"ErrnoTesterChecker_setErrnoIfErrorRange", 0}, - &ErrnoTesterChecker::evalSetErrnoIfErrorRange}}; + &ErrnoTesterChecker::evalSetErrnoIfErrorRange}, + {{"ErrnoTesterChecker_setErrnoCheckState", 0}, + &ErrnoTesterChecker::evalSetErrnoCheckState}}; }; } // namespace void ErrnoTesterChecker::evalSetErrno(CheckerContext &C, const CallEvent &Call) { - C.addTransition(errno_modeling::setErrnoValue( - C.getState(), C.getLocationContext(), Call.getArgSVal(0))); + C.addTransition(setErrnoValue(C.getState(), C.getLocationContext(), + Call.getArgSVal(0), Irrelevant)); } void ErrnoTesterChecker::evalGetErrno(CheckerContext &C, const CallEvent &Call) { ProgramStateRef State = C.getState(); - Optional<SVal> ErrnoVal = errno_modeling::getErrnoValue(State); + Optional<SVal> ErrnoVal = getErrnoValue(State); assert(ErrnoVal && "Errno value should be available."); State = State->BindExpr(Call.getOriginExpr(), C.getLocationContext(), *ErrnoVal); @@ -71,10 +106,11 @@ ProgramStateRef StateSuccess = State->BindExpr( Call.getOriginExpr(), C.getLocationContext(), SVB.makeIntVal(0, true)); + StateSuccess = setErrnoState(StateSuccess, MustNotBeChecked); ProgramStateRef StateFailure = State->BindExpr( Call.getOriginExpr(), C.getLocationContext(), SVB.makeIntVal(1, true)); - StateFailure = errno_modeling::setErrnoValue(StateFailure, C, 11); + StateFailure = setErrnoValue(StateFailure, C, 11, Irrelevant); C.addTransition(StateSuccess); C.addTransition(StateFailure); @@ -87,6 +123,7 @@ ProgramStateRef StateSuccess = State->BindExpr( Call.getOriginExpr(), C.getLocationContext(), SVB.makeIntVal(0, true)); + StateSuccess = setErrnoState(StateSuccess, MustNotBeChecked); ProgramStateRef StateFailure = State->BindExpr( Call.getOriginExpr(), C.getLocationContext(), SVB.makeIntVal(1, true)); @@ -94,13 +131,40 @@ nullptr, Call.getOriginExpr(), C.getLocationContext(), C.blockCount()); StateFailure = StateFailure->assume(ErrnoVal, true); assert(StateFailure && "Failed to assume on an initial value."); - StateFailure = errno_modeling::setErrnoValue( - StateFailure, C.getLocationContext(), ErrnoVal); + StateFailure = + setErrnoValue(StateFailure, C.getLocationContext(), ErrnoVal, Irrelevant); C.addTransition(StateSuccess); C.addTransition(StateFailure); } +void ErrnoTesterChecker::evalSetErrnoCheckState(CheckerContext &C, + const CallEvent &Call) { + ProgramStateRef State = C.getState(); + SValBuilder &SVB = C.getSValBuilder(); + + ProgramStateRef StateSuccess = State->BindExpr( + Call.getOriginExpr(), C.getLocationContext(), SVB.makeIntVal(0, true)); + StateSuccess = setErrnoState(StateSuccess, MustNotBeChecked); + + ProgramStateRef StateFailure1 = State->BindExpr( + Call.getOriginExpr(), C.getLocationContext(), SVB.makeIntVal(1, true)); + StateFailure1 = setErrnoValue(StateFailure1, C, 1, Irrelevant); + + ProgramStateRef StateFailure2 = State->BindExpr( + Call.getOriginExpr(), C.getLocationContext(), SVB.makeIntVal(2, true)); + StateFailure2 = setErrnoValue(StateFailure2, C, 2, MustBeChecked); + + C.addTransition(StateSuccess, + getErrnoNoteTag(C, "Assuming that this function succeeds but " + "sets 'errno' to an unspecified value.")); + C.addTransition(StateFailure1); + C.addTransition( + StateFailure2, + getErrnoNoteTag(C, "Assuming that this function returns 2. 'errno' " + "should be checked to test for failure.")); +} + bool ErrnoTesterChecker::evalCall(const CallEvent &Call, CheckerContext &C) const { const EvalFn *Fn = TestCalls.lookup(Call); Index: clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h =================================================================== --- clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h +++ clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h @@ -21,16 +21,55 @@ namespace ento { namespace errno_modeling { +enum ErrnoCheckState : unsigned { + /// We do not know anything about 'errno'. + Irrelevant = 0, + + /// Value of 'errno' should be checked to find out if a previous function call + /// has failed. + MustBeChecked = 1, + + /// Value of 'errno' is not allowed to be read, it can contain an unspecified + /// value. + MustNotBeChecked = 2 +}; + /// Returns the value of 'errno', if 'errno' was found in the AST. llvm::Optional<SVal> getErrnoValue(ProgramStateRef State); +/// Returns the errno check state, \c Errno_Irrelevant if 'errno' was not found +/// (this is not the only case for that value). +ErrnoCheckState getErrnoState(ProgramStateRef State); + +/// Returns the location that points to the \c MemoryRegion where the 'errno' +/// value is stored. Returns \c None if 'errno' was not found. Otherwise it +/// always returns a valid memory region in the system global memory space. +llvm::Optional<Loc> getErrnoLoc(ProgramStateRef State); + /// Set value of 'errno' to any SVal, if possible. +/// The errno check state is set always when the 'errno' value is set. ProgramStateRef setErrnoValue(ProgramStateRef State, - const LocationContext *LCtx, SVal Value); + const LocationContext *LCtx, SVal Value, + ErrnoCheckState EState); /// Set value of 'errno' to a concrete (signed) integer, if possible. +/// The errno check state is set always when the 'errno' value is set. ProgramStateRef setErrnoValue(ProgramStateRef State, CheckerContext &C, - uint64_t Value); + uint64_t Value, ErrnoCheckState EState); + +/// Set the errno check state, do not modify the errno value. +ProgramStateRef setErrnoState(ProgramStateRef State, ErrnoCheckState EState); + +/// Determine if a `Decl` node related to 'errno'. +/// This is true if the declaration is the errno variable or a function +/// that returns a pointer to the 'errno' value (usually the 'errno' macro is +/// defined with this function). \p D is not required to be a canonical +/// declaration. +bool isErrno(const Decl *D); + +/// Create a NoteTag that displays the message if the 'errno' memory region is +/// marked as interesting, and resets the interestingness. +const NoteTag *getErrnoNoteTag(CheckerContext &C, const std::string &Message); } // namespace errno_modeling } // namespace ento Index: clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp +++ clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp @@ -70,6 +70,8 @@ /// The value is null if the 'errno' value was not recognized in the AST. REGISTER_TRAIT_WITH_PROGRAMSTATE(ErrnoRegion, const MemRegion *) +REGISTER_TRAIT_WITH_PROGRAMSTATE(ErrnoState, errno_modeling::ErrnoCheckState) + /// Search for a variable called "errno" in the AST. /// Return nullptr if not found. static const VarDecl *getErrnoVar(ASTContext &ACtx) { @@ -141,7 +143,8 @@ State->getRegion(ErrnoVar, C.getLocationContext()); assert(ErrnoR && "Memory region should exist for the 'errno' variable."); State = State->set<ErrnoRegion>(ErrnoR); - State = errno_modeling::setErrnoValue(State, C, 0); + State = + errno_modeling::setErrnoValue(State, C, 0, errno_modeling::Irrelevant); C.addTransition(State); } else if (ErrnoDecl) { assert(isa<FunctionDecl>(ErrnoDecl) && "Invalid errno location function."); @@ -168,7 +171,8 @@ ACtx.IntTy, SVB.makeZeroArrayIndex(), RMgr.getSymbolicRegion(Sym, GlobalSystemSpace), C.getASTContext()); State = State->set<ErrnoRegion>(ErrnoR); - State = errno_modeling::setErrnoValue(State, C, 0); + State = + errno_modeling::setErrnoValue(State, C, 0, errno_modeling::Irrelevant); C.addTransition(State); } } @@ -195,7 +199,7 @@ void ErrnoModeling::checkLiveSymbols(ProgramStateRef State, SymbolReaper &SR) const { // The special errno region should never garbage collected. - if (const MemRegion *ErrnoR = State->get<ErrnoRegion>()) + if (const auto *ErrnoR = State->get<ErrnoRegion>()) SR.markLive(ErrnoR); } @@ -212,22 +216,63 @@ } ProgramStateRef setErrnoValue(ProgramStateRef State, - const LocationContext *LCtx, SVal Value) { + const LocationContext *LCtx, SVal Value, + ErrnoCheckState EState) { const MemRegion *ErrnoR = State->get<ErrnoRegion>(); if (!ErrnoR) return State; - return State->bindLoc(loc::MemRegionVal{ErrnoR}, Value, LCtx); + // First set the errno value, the old state is still available at 'checkBind' + // or 'checkLocation' for errno value. + State = State->bindLoc(loc::MemRegionVal{ErrnoR}, Value, LCtx); + return State->set<ErrnoState>(EState); } ProgramStateRef setErrnoValue(ProgramStateRef State, CheckerContext &C, - uint64_t Value) { + uint64_t Value, ErrnoCheckState EState) { const MemRegion *ErrnoR = State->get<ErrnoRegion>(); if (!ErrnoR) return State; - return State->bindLoc( + State = State->bindLoc( loc::MemRegionVal{ErrnoR}, C.getSValBuilder().makeIntVal(Value, C.getASTContext().IntTy), C.getLocationContext()); + return State->set<ErrnoState>(EState); +} + +Optional<Loc> getErrnoLoc(ProgramStateRef State) { + const MemRegion *ErrnoR = State->get<ErrnoRegion>(); + if (!ErrnoR) + return {}; + return loc::MemRegionVal{ErrnoR}; +} + +ProgramStateRef setErrnoState(ProgramStateRef State, ErrnoCheckState EState) { + return State->set<ErrnoState>(EState); +} + +ErrnoCheckState getErrnoState(ProgramStateRef State) { + return State->get<ErrnoState>(); +} + +bool isErrno(const Decl *D) { + if (const auto *VD = dyn_cast_or_null<VarDecl>(D)) + if (const IdentifierInfo *II = VD->getIdentifier()) + return II->getName() == ErrnoVarName; + if (const auto *FD = dyn_cast_or_null<FunctionDecl>(D)) + if (const IdentifierInfo *II = FD->getIdentifier()) + return llvm::is_contained(ErrnoLocationFuncNames, II->getName()); + return false; +} + +const NoteTag *getErrnoNoteTag(CheckerContext &C, const std::string &Message) { + return C.getNoteTag([Message](PathSensitiveBugReport &BR) -> std::string { + const MemRegion *ErrnoR = BR.getErrorNode()->getState()->get<ErrnoRegion>(); + if (ErrnoR && BR.isInteresting(ErrnoR)) { + BR.markNotInteresting(ErrnoR); + return Message; + } + return ""; + }); } } // namespace errno_modeling Index: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp =================================================================== --- /dev/null +++ clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp @@ -0,0 +1,249 @@ +//=== ErrnoChecker.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 +// +//===----------------------------------------------------------------------===// +// +// This defines an "errno checker" that can detect some invalid use of the +// system-defined value 'errno'. This checker works together with the +// ErrnoModeling checker and other checkers like StdCLibraryFunctions. +// +//===----------------------------------------------------------------------===// + +#include "ErrnoModeling.h" +#include "clang/AST/ParentMapContext.h" +#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" +#include "clang/StaticAnalyzer/Core/Checker.h" +#include "clang/StaticAnalyzer/Core/CheckerManager.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h" +#include "llvm/ADT/STLExtras.h" + +using namespace clang; +using namespace ento; +using namespace errno_modeling; + +namespace { + +class ErrnoChecker + : public Checker<check::Location, check::PreCall, check::RegionChanges> { +public: + void checkLocation(SVal Loc, bool IsLoad, const Stmt *S, + CheckerContext &) const; + void checkPreCall(const CallEvent &Call, CheckerContext &C) const; + ProgramStateRef + checkRegionChanges(ProgramStateRef State, + const InvalidatedSymbols *Invalidated, + ArrayRef<const MemRegion *> ExplicitRegions, + ArrayRef<const MemRegion *> Regions, + const LocationContext *LCtx, const CallEvent *Call) const; + void checkBranchCondition(const Stmt *Condition, CheckerContext &Ctx) const; + + /// Indicates if a read (load) of \c errno is allowed in a non-condition part + /// of \c if, \c switch, loop and conditional statements when the errno + /// value may be undefined. + bool AllowErrnoReadOutsideConditions = true; + +private: + void generateErrnoNotCheckedBug(CheckerContext &C, ProgramStateRef State, + const MemRegion *ErrnoRegion, + const CallEvent *CallMayChangeErrno) const; + + BugType BT_InvalidErrnoRead{this, "Value of 'errno' could be undefined", + "Error handling"}; + BugType BT_ErrnoNotChecked{this, "Value of 'errno' was not checked", + "Error handling"}; +}; + +} // namespace + +static ProgramStateRef setErrnoStateIrrelevant(ProgramStateRef State) { + return setErrnoState(State, Irrelevant); +} + +/// Check if a statement (expression) or an ancestor of it is in a condition +/// part of a (conditional, loop, switch) statement. +static bool isInCondition(const Stmt *S, CheckerContext &C) { + ParentMapContext &ParentCtx = C.getASTContext().getParentMapContext(); + bool CondFound = false; + while (S && !CondFound) { + const DynTypedNodeList Parents = ParentCtx.getParents(*S); + if (Parents.empty()) + break; + const auto *ParentS = Parents[0].get<Stmt>(); + if (!ParentS || isa<CallExpr>(ParentS)) + break; + switch (ParentS->getStmtClass()) { + case Expr::IfStmtClass: + CondFound = (S == cast<IfStmt>(ParentS)->getCond()); + break; + case Expr::ForStmtClass: + CondFound = (S == cast<ForStmt>(ParentS)->getCond()); + break; + case Expr::DoStmtClass: + CondFound = (S == cast<DoStmt>(ParentS)->getCond()); + break; + case Expr::WhileStmtClass: + CondFound = (S == cast<WhileStmt>(ParentS)->getCond()); + break; + case Expr::SwitchStmtClass: + CondFound = (S == cast<SwitchStmt>(ParentS)->getCond()); + break; + case Expr::ConditionalOperatorClass: + CondFound = (S == cast<ConditionalOperator>(ParentS)->getCond()); + break; + case Expr::BinaryConditionalOperatorClass: + CondFound = (S == cast<BinaryConditionalOperator>(ParentS)->getCommon()); + break; + default: + break; + } + S = ParentS; + } + return CondFound; +} + +void ErrnoChecker::generateErrnoNotCheckedBug( + CheckerContext &C, ProgramStateRef State, const MemRegion *ErrnoRegion, + const CallEvent *CallMayChangeErrno) const { + if (ExplodedNode *N = C.generateNonFatalErrorNode(State)) { + SmallString<100> StrBuf; + llvm::raw_svector_ostream OS(StrBuf); + if (CallMayChangeErrno) { + OS << "Value of 'errno' was not checked and may be overwritten by " + "function '"; + const auto *CallD = + dyn_cast_or_null<FunctionDecl>(CallMayChangeErrno->getDecl()); + assert(CallD && CallD->getIdentifier()); + OS << CallD->getIdentifier()->getName() << "'"; + } else { + OS << "Value of 'errno' was not checked and is overwritten here"; + } + auto BR = std::make_unique<PathSensitiveBugReport>(BT_ErrnoNotChecked, + OS.str(), N); + BR->markInteresting(ErrnoRegion); + C.emitReport(std::move(BR)); + } +} + +void ErrnoChecker::checkLocation(SVal Loc, bool IsLoad, const Stmt *S, + CheckerContext &C) const { + Optional<ento::Loc> ErrnoLoc = getErrnoLoc(C.getState()); + if (!ErrnoLoc) + return; + + auto L = Loc.getAs<ento::Loc>(); + if (!L || *ErrnoLoc != *L) + return; + + ProgramStateRef State = C.getState(); + ErrnoCheckState EState = getErrnoState(State); + + if (IsLoad) { + switch (EState) { + case MustNotBeChecked: + // Read of 'errno' when it may have undefined value. + if (!AllowErrnoReadOutsideConditions || isInCondition(S, C)) { + if (ExplodedNode *N = C.generateErrorNode()) { + auto BR = std::make_unique<PathSensitiveBugReport>( + BT_InvalidErrnoRead, + "An undefined value may be read from 'errno'", N); + BR->markInteresting(ErrnoLoc->getAsRegion()); + C.emitReport(std::move(BR)); + } + } + break; + case MustBeChecked: + // 'errno' has to be checked. A load is required for this, with no more + // information we can assume that it is checked somehow. + // After this place 'errno' is allowed to be read and written. + State = setErrnoStateIrrelevant(State); + C.addTransition(State); + break; + default: + break; + } + } else { + switch (EState) { + case MustBeChecked: + // 'errno' is overwritten without a read before but it should have been + // checked. + generateErrnoNotCheckedBug(C, setErrnoStateIrrelevant(State), + ErrnoLoc->getAsRegion(), nullptr); + break; + case MustNotBeChecked: + // Write to 'errno' when it is not allowed to be read. + // After this place 'errno' is allowed to be read and written. + State = setErrnoStateIrrelevant(State); + C.addTransition(State); + break; + default: + break; + } + } +} + +void ErrnoChecker::checkPreCall(const CallEvent &Call, + CheckerContext &C) const { + const auto *CallF = dyn_cast_or_null<FunctionDecl>(Call.getDecl()); + if (!CallF) + return; + + CallF = CallF->getCanonicalDecl(); + // If 'errno' must be checked, it should be done as soon as possible, and + // before any other call to a system function (something in a system header). + // To avoid use of a long list of functions that may change 'errno' + // (which may be different with standard library versions) assume that any + // function can change it. + // A list of special functions can be used that are allowed here without + // generation of diagnostic. For now the only such case is 'errno' itself. + // Probably 'strerror'? + if (CallF->isExternC() && CallF->isGlobal() && + C.getSourceManager().isInSystemHeader(CallF->getLocation()) && + !isErrno(CallF)) { + if (getErrnoState(C.getState()) == MustBeChecked) { + Optional<ento::Loc> ErrnoLoc = getErrnoLoc(C.getState()); + assert(ErrnoLoc && "ErrnoLoc should exist if an errno state is set."); + generateErrnoNotCheckedBug(C, setErrnoStateIrrelevant(C.getState()), + ErrnoLoc->getAsRegion(), &Call); + } + } +} + +ProgramStateRef ErrnoChecker::checkRegionChanges( + ProgramStateRef State, const InvalidatedSymbols *Invalidated, + ArrayRef<const MemRegion *> ExplicitRegions, + ArrayRef<const MemRegion *> Regions, const LocationContext *LCtx, + const CallEvent *Call) const { + Optional<ento::Loc> ErrnoLoc = getErrnoLoc(State); + if (!ErrnoLoc) + return State; + const MemRegion *ErrnoRegion = ErrnoLoc->getAsRegion(); + + // If 'errno' is invalidated we can not know if it is checked or written into, + // allow read and write without bug reports. + if (llvm::is_contained(Regions, ErrnoRegion)) + return setErrnoStateIrrelevant(State); + + // Always reset errno state when the system memory space is invalidated. + // The ErrnoRegion is not always found in the list in this case. + if (llvm::is_contained(Regions, ErrnoRegion->getMemorySpace())) + return setErrnoStateIrrelevant(State); + + return State; +} + +void ento::registerErrnoChecker(CheckerManager &mgr) { + const AnalyzerOptions &Opts = mgr.getAnalyzerOptions(); + auto *Checker = mgr.registerChecker<ErrnoChecker>(); + Checker->AllowErrnoReadOutsideConditions = Opts.getCheckerBooleanOption( + Checker, "AllowErrnoReadOutsideConditionExpressions"); +} + +bool ento::shouldRegisterErrnoChecker(const CheckerManager &mgr) { + return true; +} Index: clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt =================================================================== --- clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -40,6 +40,7 @@ DynamicTypePropagation.cpp DynamicTypeChecker.cpp EnumCastOutOfRangeChecker.cpp + ErrnoChecker.cpp ErrnoModeling.cpp ErrnoTesterChecker.cpp ExprInspectionChecker.cpp Index: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td =================================================================== --- clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -546,6 +546,18 @@ let ParentPackage = UnixAlpha in { +def ErrnoChecker : Checker<"Errno">, + HelpText<"Check for improper use of 'errno'">, + Dependencies<[ErrnoModeling]>, + CheckerOptions<[ + CmdLineOption<Boolean, + "AllowErrnoReadOutsideConditionExpressions", + "Allow read of undefined value from errno outside of conditions", + "true", + InAlpha>, + ]>, + Documentation<HasDocumentation>; + def ChrootChecker : Checker<"Chroot">, HelpText<"Check improper use of chroot">, Documentation<HasDocumentation>; Index: clang/docs/analyzer/checkers.rst =================================================================== --- clang/docs/analyzer/checkers.rst +++ clang/docs/analyzer/checkers.rst @@ -2520,6 +2520,75 @@ f(); // warn: no call of chdir("/") immediately after chroot } +.. _alpha-unix-Errno: + +alpha.unix.Errno (C) +"""""""""""""""""""" + +Check for improper use of ``errno``. +This checker implements partially CERT rule +`ERR30-C. Set errno to zero before calling a library function known to set errno, +and check errno only after the function returns a value indicating failure +<https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=87152351>`_. +The checker can find the first read of ``errno`` after successful standard +function calls. + +The C and POSIX standards often do not define if a standard library function +may change value of ``errno`` if the call does not fail. +Therefore, ``errno`` should only be used if it is known from the return value +of a function that the call has failed. +There are exceptions to this rule (for example ``strtol``) but the affected +functions are not yet supported by the checker. +The return values for the failure cases are documented in the standard Linux man +pages of the functions and in the `POSIX standard <https://pubs.opengroup.org/onlinepubs/9699919799/>`_. + +.. code-block:: c + + int unsafe_errno_read(int sock, void *data, int data_size) { + if (send(sock, data, data_size, 0) != data_size) { + // 'send' can be successful even if not all data was sent + if (errno == 1) { // An undefined value may be read from 'errno' + return 0; + } + } + return 1; + } + +The supported functions are the same that are modeled by checker +:ref:`alpha-unix-StdCLibraryFunctionArgs`. +The ``ModelPOSIX`` option of that checker affects the set of checked functions. + +**Parameters** + +The ``AllowErrnoReadOutsideConditionExpressions`` option allows read of the +errno value if the value is not used in a condition (in ``if`` statements, +loops, conditional expressions, ``switch`` statements). For example ``errno`` +can be stored into a variable without getting a warning by the checker. + +.. code-block:: c + + int unsafe_errno_read(int sock, void *data, int data_size) { + if (send(sock, data, data_size, 0) != data_size) { + int err = errno; + // warning if 'AllowErrnoReadOutsideConditionExpressions' is false + // no warning if 'AllowErrnoReadOutsideConditionExpressions' is true + } + return 1; + } + +Default value of this option is ``true``. This allows save of the errno value +for possible later error handling. + +**Limitations** + + - Only the very first usage of ``errno`` is checked after an affected function + call. Value of ``errno`` is not followed when it is stored into a variable + or returned from a function. + - Documentation of function ``lseek`` is not clear about what happens if the + function returns different value than the expected file position but not -1. + To avoid possible false-positives ``errno`` is allowed to be used in this + case. + .. _alpha-unix-PthreadLock: alpha.unix.PthreadLock (C) Index: clang/docs/ReleaseNotes.rst =================================================================== --- clang/docs/ReleaseNotes.rst +++ clang/docs/ReleaseNotes.rst @@ -581,6 +581,10 @@ `strcmp``, ``strncmp``, ``strcpy``, ``strlen``, ``strsep`` and many more. Although this checker currently is in list of alpha checkers due to a false positive. +- Added a new checker ``alpha.unix.Errno``. This can find the first read + of ``errno`` after successful standard function calls, such use of ``errno`` + could be unsafe. + - Deprecate the ``-analyzer-store region`` and ``-analyzer-opt-analyze-nested-blocks`` analyzer flags. These flags are still accepted, but a warning will be displayed.
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits