[clang] 94061df - [analyzer] StdLibraryFunctionsChecker: Add argument constraints

Gabor Marton via cfe-commits cfe-commits at lists.llvm.org
Fri Mar 20 08:33:34 PDT 2020


Author: Gabor Marton
Date: 2020-03-20T16:33:14+01:00
New Revision: 94061df6e5f24c2f25ad7c55af13dd17cccc5856

URL: https://github.com/llvm/llvm-project/commit/94061df6e5f24c2f25ad7c55af13dd17cccc5856
DIFF: https://github.com/llvm/llvm-project/commit/94061df6e5f24c2f25ad7c55af13dd17cccc5856.diff

LOG: [analyzer] StdLibraryFunctionsChecker: Add argument constraints

Differential Revision:
https://reviews.llvm.org/D73898

Added: 
    clang/test/Analysis/std-c-library-functions-arg-constraints.c

Modified: 
    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.c

Removed: 
    


################################################################################
diff  --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 91c0fc88f6e7..a21107cd4c2d 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -295,6 +295,13 @@ def StdCLibraryFunctionsChecker : Checker<"StdCLibraryFunctions">,
   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">,

diff  --git a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
index 210a4ff199c6..2e956cc2bfe7 100644
--- 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;
 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 @@ class StdLibraryFunctionsChecker : public Checker<check::PostCall, eval::Call> {
   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 
diff erent kind
   /// of constraints, e.g range constraints or correlation between two
@@ -99,6 +109,9 @@ class StdLibraryFunctionsChecker : public Checker<check::PostCall, eval::Call> {
     /// 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 @@ class StdLibraryFunctionsChecker : public Checker<check::PostCall, eval::Call> {
       }
       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 @@ class StdLibraryFunctionsChecker : public Checker<check::PostCall, eval::Call> {
                           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 @@ class StdLibraryFunctionsChecker : public Checker<check::PostCall, eval::Call> {
       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 @@ class StdLibraryFunctionsChecker : public Checker<check::PostCall, eval::Call> {
   // 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 @@ class StdLibraryFunctionsChecker : public Checker<check::PostCall, eval::Call> {
   }
 
 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 @@ ProgramStateRef StdLibraryFunctionsChecker::ComparisonConstraint::apply(
   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 @@ void StdLibraryFunctionsChecker::checkPostCall(const CallEvent &Call,
 
 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 @@ bool StdLibraryFunctionsChecker::evalCall(const CallEvent &Call,
   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 @@ StdLibraryFunctionsChecker::findFunctionSummary(const FunctionDecl *FD,
   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 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
   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 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
 
   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 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
                                             {'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 StdLibraryFunctionsChecker::initFunctionSummaries(
 }
 
 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 
diff erent 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)

diff  --git a/clang/test/Analysis/analyzer-enabled-checkers.c b/clang/test/Analysis/analyzer-enabled-checkers.c
index ba850ec6da48..55f15f6d37c8 100644
--- 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
index 000000000000..07a9df9526fc
--- /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
index 9fb8833175be..2a090f397714 100644
--- 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);
 


        


More information about the cfe-commits mailing list