diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -295,6 +295,13 @@ HelpText<"Improve modeling of the C standard library functions">, Documentation; +def StdCLibraryFunctionArgsChecker : Checker<"StdCLibraryFunctionArgs">, + HelpText<"Check constraints of arguments of C standard library functions, " + "such as whether the parameter of isalpha is in the range [0, 255] " + "or is EOF.">, + Dependencies<[StdCLibraryFunctionsChecker]>, + Documentation; + def TrustNonnullChecker : Checker<"TrustNonnull">, HelpText<"Trust that returns from framework methods annotated with _Nonnull " "are not null">, diff --git a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp --- a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp @@ -7,7 +7,6 @@ //===----------------------------------------------------------------------===// // // This checker improves modeling of a few simple library functions. -// It does not generate warnings. // // This checker provides a specification format - `Summary' - and // contains descriptions of some library functions in this format. Each @@ -51,6 +50,7 @@ //===----------------------------------------------------------------------===// #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" #include "clang/StaticAnalyzer/Core/CheckerManager.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" @@ -61,7 +61,8 @@ using namespace clang::ento; namespace { -class StdLibraryFunctionsChecker : public Checker { +class StdLibraryFunctionsChecker + : public Checker { /// Below is a series of typedefs necessary to define function specs. /// We avoid nesting types here because each additional qualifier /// would need to be repeated in every function spec. @@ -87,6 +88,15 @@ typedef uint32_t ArgNo; static const ArgNo Ret; + class ValueConstraint; + + // Pointer to the ValueConstraint. We need a copyable, polymorphic and + // default initialize able type (vector needs that). A raw pointer was good, + // however, we cannot default initialize that. unique_ptr makes the Summary + // class non-copyable, therefore not an option. Releasing the copyability + // requirement would render the initialization of the Summary map infeasible. + using ValueConstraintPtr = std::shared_ptr; + /// Polymorphic base class that represents a constraint on a given argument /// (or return value) of a function. Derived classes implement different kind /// of constraints, e.g range constraints or correlation between two @@ -99,6 +109,9 @@ /// is returned then the constraint is not feasible. virtual ProgramStateRef apply(ProgramStateRef State, const CallEvent &Call, const Summary &Summary) const = 0; + virtual ValueConstraintPtr negate() const { + llvm_unreachable("Not implemented"); + }; ArgNo getArgNo() const { return ArgN; } protected: @@ -139,6 +152,21 @@ } llvm_unreachable("Unknown range kind!"); } + + ValueConstraintPtr negate() const override { + RangeConstraint Tmp(*this); + switch (Kind) { + case OutOfRange: + Tmp.Kind = WithinRange; + break; + case WithinRange: + Tmp.Kind = OutOfRange; + break; + default: + llvm_unreachable("Unknown RangeConstraint kind!"); + } + return std::make_shared(Tmp); + } }; class ComparisonConstraint : public ValueConstraint { @@ -155,22 +183,31 @@ const Summary &Summary) const override; }; - // Pointer to the ValueConstraint. We need a copyable, polymorphic and - // default initialize able type (vector needs that). A raw pointer was good, - // however, we cannot default initialize that. unique_ptr makes the Summary - // class non-copyable, therefore not an option. Releasing the copyability - // requirement would render the initialization of the Summary map infeasible. - using ValueConstraintPtr = std::shared_ptr; /// The complete list of constraints that defines a single branch. typedef std::vector ConstraintSet; using ArgTypes = std::vector; using Cases = std::vector; - /// Includes information about function prototype (which is necessary to - /// ensure we're modeling the right function and casting values properly), - /// approach to invalidation, and a list of branches - essentially, a list - /// of list of ranges - essentially, a list of lists of lists of segments. + /// Includes information about + /// * function prototype (which is necessary to + /// ensure we're modeling the right function and casting values properly), + /// * approach to invalidation, + /// * a list of branches - a list of list of ranges - + /// A branch represents a path in the exploded graph of a function (which + /// is a tree). So, a branch is a series of assumptions. In other words, + /// branches represent split states and additional assumptions on top of + /// the splitting assumption. + /// For example, consider the branches in `isalpha(x)` + /// Branch 1) + /// x is in range ['A', 'Z'] or in ['a', 'z'] + /// then the return value is not 0. (I.e. out-of-range [0, 0]) + /// Branch 2) + /// x is out-of-range ['A', 'Z'] and out-of-range ['a', 'z'] + /// then the return value is 0. + /// * a list of argument constraints, that must be true on every branch. + /// If these constraints are not satisfied that means a fatal error + /// usually resulting in undefined behaviour. struct Summary { const ArgTypes ArgTys; const QualType RetTy; @@ -185,6 +222,10 @@ CaseConstraints.push_back(std::move(CS)); return *this; } + Summary &ArgConstraint(ValueConstraintPtr VC) { + ArgConstraints.push_back(VC); + return *this; + } private: static void assertTypeSuitableForSummary(QualType T) { @@ -220,6 +261,8 @@ // lazily, and it doesn't change after initialization. mutable llvm::StringMap FunctionSummaryMap; + mutable std::unique_ptr BT_InvalidArg; + // Auxiliary functions to support ArgNo within all structures // in a unified manner. static QualType getArgType(const Summary &Summary, ArgNo ArgN) { @@ -238,15 +281,37 @@ } public: + void checkPreCall(const CallEvent &Call, CheckerContext &C) const; void checkPostCall(const CallEvent &Call, CheckerContext &C) const; bool evalCall(const CallEvent &Call, CheckerContext &C) const; + enum CheckKind { CK_StdCLibraryFunctionArgsChecker, CK_NumCheckKinds }; + DefaultBool ChecksEnabled[CK_NumCheckKinds]; + CheckerNameRef CheckNames[CK_NumCheckKinds]; + private: Optional findFunctionSummary(const FunctionDecl *FD, const CallExpr *CE, CheckerContext &C) const; + Optional findFunctionSummary(const CallEvent &Call, + CheckerContext &C) const; void initFunctionSummaries(CheckerContext &C) const; + + void reportBug(const CallEvent &Call, ExplodedNode *N, + CheckerContext &C) const { + if (!ChecksEnabled[CK_StdCLibraryFunctionArgsChecker]) + return; + // TODO Add detailed diagnostic. + StringRef Msg = "Function argument constraint is not satisfied"; + if (!BT_InvalidArg) + BT_InvalidArg = std::make_unique( + CheckNames[CK_StdCLibraryFunctionArgsChecker], + "Unsatisfied argument constraints", categories::LogicError); + auto R = std::make_unique(*BT_InvalidArg, Msg, N); + bugreporter::trackExpressionValue(N, Call.getArgExpr(0), *R); + C.emitReport(std::move(R)); + } }; const StdLibraryFunctionsChecker::ArgNo StdLibraryFunctionsChecker::Ret = @@ -360,17 +425,37 @@ return State; } -void StdLibraryFunctionsChecker::checkPostCall(const CallEvent &Call, - CheckerContext &C) const { - const FunctionDecl *FD = dyn_cast_or_null(Call.getDecl()); - if (!FD) +void StdLibraryFunctionsChecker::checkPreCall(const CallEvent &Call, + CheckerContext &C) const { + Optional FoundSummary = findFunctionSummary(Call, C); + if (!FoundSummary) return; - const CallExpr *CE = dyn_cast_or_null(Call.getOriginExpr()); - if (!CE) - return; + const Summary &Summary = *FoundSummary; + ProgramStateRef State = C.getState(); + + for (const ValueConstraintPtr& VC : Summary.ArgConstraints) { + ProgramStateRef SuccessSt = VC->apply(State, Call, Summary); + ProgramStateRef FailureSt = VC->negate()->apply(State, Call, Summary); + // The argument constraint is not satisfied. + if (FailureSt && !SuccessSt) { + if (ExplodedNode *N = C.generateErrorNode(State)) + reportBug(Call, N, C); + break; + } else { + // Apply the constraint even if we cannot reason about the argument. This + // means both SuccessSt and FailureSt can be true. If we weren't applying + // the constraint that would mean that symbolic execution continues on a + // code whose behaviour is undefined. + assert(SuccessSt); + C.addTransition(SuccessSt); + } + } +} - Optional FoundSummary = findFunctionSummary(FD, CE, C); +void StdLibraryFunctionsChecker::checkPostCall(const CallEvent &Call, + CheckerContext &C) const { + Optional FoundSummary = findFunctionSummary(Call, C); if (!FoundSummary) return; @@ -394,15 +479,7 @@ bool StdLibraryFunctionsChecker::evalCall(const CallEvent &Call, CheckerContext &C) const { - const auto *FD = dyn_cast_or_null(Call.getDecl()); - if (!FD) - return false; - - const auto *CE = dyn_cast_or_null(Call.getOriginExpr()); - if (!CE) - return false; - - Optional FoundSummary = findFunctionSummary(FD, CE, C); + Optional FoundSummary = findFunctionSummary(Call, C); if (!FoundSummary) return false; @@ -411,6 +488,7 @@ case EvalCallAsPure: { ProgramStateRef State = C.getState(); const LocationContext *LC = C.getLocationContext(); + const auto *CE = cast_or_null(Call.getOriginExpr()); SVal V = C.getSValBuilder().conjureSymbolVal( CE, LC, CE->getType().getCanonicalType(), C.blockCount()); State = State->BindExpr(CE, LC, V); @@ -490,6 +568,18 @@ return None; } +Optional +StdLibraryFunctionsChecker::findFunctionSummary(const CallEvent &Call, + CheckerContext &C) const { + const FunctionDecl *FD = dyn_cast_or_null(Call.getDecl()); + if (!FD) + return None; + const CallExpr *CE = dyn_cast_or_null(Call.getOriginExpr()); + if (!CE) + return None; + return findFunctionSummary(FD, CE, C); +} + void StdLibraryFunctionsChecker::initFunctionSummaries( CheckerContext &C) const { if (!FunctionSummaryMap.empty()) @@ -584,7 +674,6 @@ auto LessThanOrEq = BO_LE; using RetType = QualType; - // Templates for summaries that are reused by many functions. auto Getc = [&]() { return Summary(ArgTypes{Irrelevant}, RetType{IntTy}, NoEvalCall) @@ -612,6 +701,9 @@ FunctionSummaryMap = { // The isascii() family of functions. + // The behavior is undefined if the value of the argument is not + // representable as unsigned char or is not equal to EOF. See e.g. C99 + // 7.4.1.2 The isalpha function (p: 181-182). { "isalnum", Summaries{ @@ -631,7 +723,9 @@ {'A', 'Z'}, {'a', 'z'}, {128, UCharRangeMax}}), - ReturnValueCondition(WithinRange, SingleValue(0))})}, + ReturnValueCondition(WithinRange, SingleValue(0))}) + .ArgConstraint(ArgumentCondition( + 0U, WithinRange, {{EOFv, EOFv}, {0, UCharRangeMax}}))}, }, { "isalpha", @@ -809,13 +903,22 @@ } void ento::registerStdCLibraryFunctionsChecker(CheckerManager &mgr) { - // If this checker grows large enough to support C++, Objective-C, or other - // standard libraries, we could use multiple register...Checker() functions, - // which would register various checkers with the help of the same Checker - // class, turning on different function summaries. mgr.registerChecker(); } bool ento::shouldRegisterStdCLibraryFunctionsChecker(const LangOptions &LO) { return true; } + +#define REGISTER_CHECKER(name) \ + void ento::register##name(CheckerManager &mgr) { \ + StdLibraryFunctionsChecker *checker = \ + mgr.getChecker(); \ + checker->ChecksEnabled[StdLibraryFunctionsChecker::CK_##name] = true; \ + checker->CheckNames[StdLibraryFunctionsChecker::CK_##name] = \ + mgr.getCurrentCheckerName(); \ + } \ + \ + bool ento::shouldRegister##name(const LangOptions &LO) { return true; } + +REGISTER_CHECKER(StdCLibraryFunctionArgsChecker) diff --git a/clang/test/Analysis/analyzer-enabled-checkers.c b/clang/test/Analysis/analyzer-enabled-checkers.c --- a/clang/test/Analysis/analyzer-enabled-checkers.c +++ b/clang/test/Analysis/analyzer-enabled-checkers.c @@ -7,6 +7,7 @@ // CHECK: OVERVIEW: Clang Static Analyzer Enabled Checkers List // CHECK-EMPTY: // CHECK-NEXT: apiModeling.StdCLibraryFunctions +// CHECK-NEXT: apiModeling.StdCLibraryFunctionArgs // CHECK-NEXT: apiModeling.TrustNonnull // CHECK-NEXT: apiModeling.llvm.CastValue // CHECK-NEXT: apiModeling.llvm.ReturnValue diff --git a/clang/test/Analysis/std-c-library-functions-arg-constraints.c b/clang/test/Analysis/std-c-library-functions-arg-constraints.c new file mode 100644 --- /dev/null +++ b/clang/test/Analysis/std-c-library-functions-arg-constraints.c @@ -0,0 +1,61 @@ +// Check the basic reporting/warning and the application of constraints. +// RUN: %clang_analyze_cc1 %s \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=apiModeling.StdCLibraryFunctions \ +// RUN: -analyzer-checker=apiModeling.StdCLibraryFunctionArgs \ +// RUN: -analyzer-checker=debug.ExprInspection \ +// RUN: -triple x86_64-unknown-linux-gnu \ +// RUN: -verify=report + +// Check the bugpath related to the reports. +// RUN: %clang_analyze_cc1 %s \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=apiModeling.StdCLibraryFunctions \ +// RUN: -analyzer-checker=apiModeling.StdCLibraryFunctionArgs \ +// RUN: -analyzer-checker=debug.ExprInspection \ +// RUN: -triple x86_64-unknown-linux-gnu \ +// RUN: -analyzer-output=text \ +// RUN: -verify=bugpath + +void clang_analyzer_eval(int); + +int glob; + +#define EOF -1 + +int isalnum(int); + +void test_alnum_concrete(int v) { + int ret = isalnum(256); // \ + // report-warning{{Function argument constraint is not satisfied}} \ + // bugpath-warning{{Function argument constraint is not satisfied}} \ + // bugpath-note{{Function argument constraint is not satisfied}} + (void)ret; +} + +void test_alnum_symbolic(int x) { + int ret = isalnum(x); + (void)ret; + + clang_analyzer_eval(EOF <= x && x <= 255); // \ + // report-warning{{TRUE}} \ + // bugpath-warning{{TRUE}} \ + // bugpath-note{{TRUE}} \ + // bugpath-note{{Left side of '&&' is true}} \ + // bugpath-note{{'x' is <= 255}} + +} + +void test_alnum_symbolic2(int x) { + if (x > 255) { // \ + // bugpath-note{{Assuming 'x' is > 255}} \ + // bugpath-note{{Taking true branch}} + + int ret = isalnum(x); // \ + // report-warning{{Function argument constraint is not satisfied}} \ + // bugpath-warning{{Function argument constraint is not satisfied}} \ + // bugpath-note{{Function argument constraint is not satisfied}} + + (void)ret; + } +} diff --git a/clang/test/Analysis/std-c-library-functions.c b/clang/test/Analysis/std-c-library-functions.c --- a/clang/test/Analysis/std-c-library-functions.c +++ b/clang/test/Analysis/std-c-library-functions.c @@ -1,8 +1,34 @@ -// RUN: %clang_analyze_cc1 -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s -// RUN: %clang_analyze_cc1 -triple i686-unknown-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s -// RUN: %clang_analyze_cc1 -triple x86_64-unknown-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s -// RUN: %clang_analyze_cc1 -triple armv7-a15-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s -// RUN: %clang_analyze_cc1 -triple thumbv7-a15-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s +// RUN: %clang_analyze_cc1 %s \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=apiModeling.StdCLibraryFunctions \ +// RUN: -analyzer-checker=debug.ExprInspection \ +// RUN: -analyzer-config eagerly-assume=false \ +// RUN: -triple i686-unknown-linux \ +// RUN: -verify + +// RUN: %clang_analyze_cc1 %s \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=apiModeling.StdCLibraryFunctions \ +// RUN: -analyzer-checker=debug.ExprInspection \ +// RUN: -analyzer-config eagerly-assume=false \ +// RUN: -triple x86_64-unknown-linux \ +// RUN: -verify + +// RUN: %clang_analyze_cc1 %s \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=apiModeling.StdCLibraryFunctions \ +// RUN: -analyzer-checker=debug.ExprInspection \ +// RUN: -analyzer-config eagerly-assume=false \ +// RUN: -triple armv7-a15-linux \ +// RUN: -verify + +// RUN: %clang_analyze_cc1 %s \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=apiModeling.StdCLibraryFunctions \ +// RUN: -analyzer-checker=debug.ExprInspection \ +// RUN: -analyzer-config eagerly-assume=false \ +// RUN: -triple thumbv7-a15-linux \ +// RUN: -verify void clang_analyzer_eval(int);