martong updated this revision to Diff 251648.
martong marked an inline comment as done.
martong added a comment.
- Use prefixes for -verify to check different things in the same test file
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73898/new/
https://reviews.llvm.org/D73898
Files:
clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
clang/test/Analysis/analyzer-enabled-checkers.c
clang/test/Analysis/std-c-library-functions-arg-constraints.c
clang/test/Analysis/std-c-library-functions.c
Index: clang/test/Analysis/std-c-library-functions.c
===================================================================
--- clang/test/Analysis/std-c-library-functions.c
+++ 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);
Index: clang/test/Analysis/std-c-library-functions-arg-constraints.c
===================================================================
--- /dev/null
+++ 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;
+ }
+}
Index: clang/test/Analysis/analyzer-enabled-checkers.c
===================================================================
--- clang/test/Analysis/analyzer-enabled-checkers.c
+++ 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
Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ 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<check::PostCall, eval::Call> {
+class StdLibraryFunctionsChecker
+ : public Checker<check::PreCall, check::PostCall, eval::Call> {
/// 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<ValueConstraint>;
+
/// 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<RangeConstraint>(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<ValueConstraint>;
/// The complete list of constraints that defines a single branch.
typedef std::vector<ValueConstraintPtr> ConstraintSet;
using ArgTypes = std::vector<QualType>;
using Cases = std::vector<ConstraintSet>;
- /// 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<Summaries> FunctionSummaryMap;
+ mutable std::unique_ptr<BugType> 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<Summary> findFunctionSummary(const FunctionDecl *FD,
const CallExpr *CE,
CheckerContext &C) const;
+ Optional<Summary> 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<BugType>(
+ CheckNames[CK_StdCLibraryFunctionArgsChecker],
+ "Unsatisfied argument constraints", categories::LogicError);
+ auto R = std::make_unique<PathSensitiveBugReport>(*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<FunctionDecl>(Call.getDecl());
- if (!FD)
+void StdLibraryFunctionsChecker::checkPreCall(const CallEvent &Call,
+ CheckerContext &C) const {
+ Optional<Summary> FoundSummary = findFunctionSummary(Call, C);
+ if (!FoundSummary)
return;
- const CallExpr *CE = dyn_cast_or_null<CallExpr>(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<Summary> FoundSummary = findFunctionSummary(FD, CE, C);
+void StdLibraryFunctionsChecker::checkPostCall(const CallEvent &Call,
+ CheckerContext &C) const {
+ Optional<Summary> 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<FunctionDecl>(Call.getDecl());
- if (!FD)
- return false;
-
- const auto *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
- if (!CE)
- return false;
-
- Optional<Summary> FoundSummary = findFunctionSummary(FD, CE, C);
+ Optional<Summary> 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<CallExpr>(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::Summary>
+StdLibraryFunctionsChecker::findFunctionSummary(const CallEvent &Call,
+ CheckerContext &C) const {
+ const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(Call.getDecl());
+ if (!FD)
+ return None;
+ const CallExpr *CE = dyn_cast_or_null<CallExpr>(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<StdLibraryFunctionsChecker>();
}
bool ento::shouldRegisterStdCLibraryFunctionsChecker(const LangOptions &LO) {
return true;
}
+
+#define REGISTER_CHECKER(name) \
+ void ento::register##name(CheckerManager &mgr) { \
+ StdLibraryFunctionsChecker *checker = \
+ mgr.getChecker<StdLibraryFunctionsChecker>(); \
+ 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)
Index: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
===================================================================
--- clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -295,6 +295,13 @@
HelpText<"Improve modeling of the C standard library functions">,
Documentation<NotDocumented>;
+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<NotDocumented>;
+
def TrustNonnullChecker : Checker<"TrustNonnull">,
HelpText<"Trust that returns from framework methods annotated with _Nonnull "
"are not null">,
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
