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 @@ -292,6 +292,11 @@ HelpText<"Improve modeling of the C standard library functions">, Documentation; +def StdCLibraryFunctionArgsChecker : Checker<"StdCLibraryFunctionArgs">, + HelpText<"Check constraints of arguments of C standard library functions">, + 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 @@ -51,6 +51,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 +62,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,11 +89,16 @@ typedef uint32_t ArgNo; static const ArgNo Ret; + class ValueConstraint; + using ValueConstraintPtr = std::shared_ptr; class ValueConstraint { public: ValueConstraint(ArgNo ArgN) : ArgN(ArgN) {} 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: @@ -132,6 +139,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 { @@ -148,17 +170,21 @@ const Summary &Summary) const override; }; - 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 - + /// i.e. a list of lists of lists of segments, + /// * 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; @@ -173,6 +199,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) { @@ -208,6 +238,8 @@ // lazily, and it doesn't change after initialization. mutable llvm::StringMap FunctionSummaryMap; + BugType BT{this, "Unsatisfied argument constraints", categories::LogicError}; + // Auxiliary functions to support ArgNo within all structures // in a unified manner. static QualType getArgType(const Summary &Summary, ArgNo ArgN) { @@ -226,13 +258,20 @@ } 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; }; @@ -348,17 +387,47 @@ 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(); + + auto Report = [&](ExplodedNode *N) { + if (!ChecksEnabled[CK_StdCLibraryFunctionArgsChecker]) + return; + // FIXME Add detailed diagnostic. + StringRef Msg = "Function argument constraint is not satisfied"; + auto R = std::make_unique(BT, Msg, N); + bugreporter::trackExpressionValue(N, Call.getArgExpr(0), *R); + C.emitReport(std::move(R)); + }; - Optional FoundSummary = findFunctionSummary(FD, CE, C); + 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)) + Report(N); + 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); + } + } +} + +void StdLibraryFunctionsChecker::checkPostCall(const CallEvent &Call, + CheckerContext &C) const { + Optional FoundSummary = findFunctionSummary(Call, C); if (!FoundSummary) return; @@ -382,15 +451,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; @@ -399,6 +460,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); @@ -478,6 +540,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()) @@ -566,7 +640,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) @@ -594,6 +667,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{ @@ -612,7 +688,9 @@ {'A', 'Z'}, {'a', 'z'}, {128, UCharMax}}), - ReturnValueCondition(WithinRange, SingleValue(0))})}, + ReturnValueCondition(WithinRange, SingleValue(0))}) + .ArgConstraint(ArgumentCondition( + 0U, WithinRange, {{EOFv, EOFv}, {0, UCharMax}}))}, }, { "isalpha", @@ -785,13 +863,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-bug-path.c b/clang/test/Analysis/std-c-library-functions-arg-constraints-bug-path.c new file mode 100644 --- /dev/null +++ b/clang/test/Analysis/std-c-library-functions-arg-constraints-bug-path.c @@ -0,0 +1,29 @@ +// 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 + +void clang_analyzer_eval(int); + +int glob; + +#define EOF -1 + +int isalnum(int); + +void test_alnum_symbolic(int x) { + int ret = isalnum(x); + (void)ret; + clang_analyzer_eval(EOF <= x && x <= 255); // expected-warning{{TRUE}} expected-note{{TRUE}} expected-note{{Left side of '&&' is true}} expected-note{{'x' is <= 255}} +} + +void test_alnum_symbolic2(int x) { + if (x > 255) { // expected-note{{Assuming 'x' is > 255}} expected-note{{Taking true branch}} + int ret = isalnum(x); // expected-warning{{Function argument constraint is not satisfied}} expected-note{{Function argument constraint is not satisfied}} + (void)ret; + } +} 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,44 @@ +// 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 + +void clang_analyzer_eval(int); + +int glob; + +#define EOF -1 + +int isalnum(int); + +void test_alnum_concrete(int v) { + int ret = isalnum(256); // expected-warning{{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); // expected-warning{{TRUE}} +} + +void test_alnum_symbolic2(int x) { + if (x > 255) { + int ret = isalnum(x); // expected-warning{{Function argument constraint is not satisfied}} + (void)ret; + } +} + +void test_alnum_infeasible_path(int x, int y) { + int ret = isalnum(x); + y = 0; + clang_analyzer_eval(EOF <= x && x <= 255); // expected-warning{{TRUE}} + + if (x > 255) // This path is no longer feasible. + ret = x / y; // No warning here + + ret = x / y; // expected-warning{{Division by zero}} +} 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);