[analyzer] ApiModeling: Add buffer size arg constraint with multiplier involved

[martong]

Summary

Further develop the buffer size argumentum constraint so it can handle sizes
that we can get by multiplying two variables.

[balazske]

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

155

s <= 3 is better here? We know that the buffer has space for 3 short's. But it is better to assume nothing about sizeof(short).

[martong]

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

155

That would have been better if the range based constraint solver was able to solve the in-equation to s. So, even though the analyzer knows that s * 2 <= 6 it cannot reason about s itself. This is the limitation of the solver. I believe @baloghadamsoftware has a few patches that directs this deficiency: D49074 and D50256

[Szelethus]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

229–234

I don't know, these constructors don't look overly talkative.

1041

So this is supposed to mean that the 1st argument has the size of the argument of 2nd argument times the 3rd argument? Unlike the other summaries, this feels a bit cryptic. Something like this might look better:

.ArgConstraint(0, BufferSize(/*BufSize*/1, /*BufSizeMultiplier*/))
.ArgConstraint(BufferSize(/*Buffer*/0, /*BufSize*/1, /*BufSizeMultiplier*/2))
.ArgConstraint(0, BufferSizeMul(1, 2))

WDYT?

[martong]

• Rebase to latest parent patch
• Create BufSize constraints in a more descriptive way

[martong]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

229–234

Yeah, renamed the params a bit.

1041

Ok, I updated with the 2nd option. :)

[Szelethus]

LGTM!

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

222

fread-like

1041

Very well!

[xazax.hun]

Overall looks good to me, I have one question inline.

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

1026

Why do we need these test functions? Above I saw fread as an example that requires this capability. Wouldn't it be better to make its summary utilize the new feature and use fread in tests? Do I miss something?

[martong]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

1026

Yeah, we could test that with fread. However, in fread there are multiple argument constraints for the different args. I wanted a test function which has this arg constraint in an isolation. In my opinion it is good to have small isolated unit tests that test only one functionality. Also if we decide to remove or modify freads summary for any reason, then we should modify this test too, on the other hand, with this test function it is not a problem.

[xazax.hun]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

1026

I see. The plan in the future is to split this checker up a bit. In this case I'd like to have these test functions in a separate test checker. Until then I'm fine with a FIXME/TODO.

[martong]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

1026

They are already enabled only if a test checker is enabled, check out the enclosing if of this block:

if (ChecksEnabled[CK_StdCLibraryFunctionsTesterChecker]) {

[xazax.hun]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

1026

Oh, sorry, my bad :)

[martong]

• Rebase to D77066

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

ValueConstraintPtr negate() const override {
       Tmp.CannotBeNull = !this->CannotBeNull;
       return std::make_shared<NotNullConstraint>(Tmp);
     }
   };
 
   // Represents a buffer argument with an additional size argument.
   // E.g. the first two arguments here:
   //   ctime_s(char *buffer, rsize_t bufsz, const time_t *time);
+  // Another example:
+  //   size_t fread(void *ptr, size_t size, size_t nmemb, FILE *stream);
+  //   // Here, ptr is the buffer, and its minimum size is `size * nmemb`.
   class BufferSizeConstraint : public ValueConstraint {
     // The argument which holds the size of the buffer.
     ArgNo SizeArgN;
+    // The argument which is a multiplier to size. This is set in case of
+    // `fread` like functions where the size is computed as a multiplication of
+    // two arguments.
+    llvm::Optional<ArgNo> SizeMultiplierArgN;
     // The operator we use in apply. This is negated in negate().
     BinaryOperator::Opcode Op = BO_LE;
 
   public:
     BufferSizeConstraint(ArgNo BufArgN, ArgNo SizeArgN)
         : ValueConstraint(BufArgN), SizeArgN(SizeArgN) {}
 
+    BufferSizeConstraint(ArgNo BufArgN, ArgNo SizeArgN, ArgNo SizeMulArgN)
+        : ValueConstraint(BufArgN), SizeArgN(SizeArgN),
+          SizeMultiplierArgN(SizeMulArgN) {}
+
     ProgramStateRef apply(ProgramStateRef State, const CallEvent &Call,
                           const Summary &Summary,
                           CheckerContext &C) const override {
+      SValBuilder &SvalBuilder = C.getSValBuilder();
       // The buffer argument.
       SVal BufV = getArgSVal(Call, getArgNo());
       // The size argument.
       SVal SizeV = getArgSVal(Call, SizeArgN);
+      // Multiply with another argument if given.
+      if (SizeMultiplierArgN) {
+        SVal SizeMulV = getArgSVal(Call, *SizeMultiplierArgN);
+        SizeV = SvalBuilder.evalBinOp(State, BO_Mul, SizeV, SizeMulV,
+                                      Summary.getArgType(SizeArgN));
+      }
       // The dynamic size of the buffer argument, got from the analyzer engine.
       SVal BufDynSize = getBufferDynamicSize(BufV, State, C);
 
-      SValBuilder &SvalBuilder = C.getSValBuilder();
       SVal Feasible = SvalBuilder.evalBinOp(State, Op, SizeV, BufDynSize,
                                             SvalBuilder.getContext().BoolTy);
       if (auto F = Feasible.getAs<DefinedOrUnknownSVal>())
         return State->assume(*F, true);
 
       // We can get here only if the size argument or the dynamic size is
       // undefined. But the dynamic size should never be undefined, only
       // unknown. So, here, the size of the argument is undefined, i.e. we
void StdLibraryFunctionsChecker::initFunctionSummaries(
   // Please update the list of functions in the header after editing!
   //
 
   // Below are helpers functions to create the summaries.
   auto ArgumentCondition = [](ArgNo ArgN, RangeKind Kind,
                               IntRangeVector Ranges) {
     return std::make_shared<RangeConstraint>(ArgN, Kind, Ranges);
   };
-  auto BufferSize = [](ArgNo BufArgN, ArgNo SizeArgN) {
-    return std::make_shared<BufferSizeConstraint>(BufArgN, SizeArgN);
+  auto BufferSize = [](auto ...Args) {
+    return std::make_shared<BufferSizeConstraint>(Args...);
   };
   struct {
     auto operator()(RangeKind Kind, IntRangeVector Ranges) {
       return std::make_shared<RangeConstraint>(Ret, Kind, Ranges);
     }
     auto operator()(BinaryOperator::Opcode Op, ArgNo OtherArgN) {
       return std::make_shared<ComparisonConstraint>(Ret, Op, OtherArgN);
     }
void StdLibraryFunctionsChecker::initFunctionSummaries(
   // Functions for testing.
   if (ChecksEnabled[CK_StdCLibraryFunctionsTesterChecker]) {
     llvm::StringMap<Summaries> TestFunctionSummaryMap = {
         {"__two_constrained_args",
          Summaries{
              Summary(ArgTypes{IntTy, IntTy}, RetType{IntTy}, EvalCallAsPure)
                  .ArgConstraint(
                      ArgumentCondition(0U, WithinRange, SingleValue(1)))
                  .ArgConstraint(
                      ArgumentCondition(1U, WithinRange, SingleValue(1)))}},
         {"__arg_constrained_twice",
          Summaries{Summary(ArgTypes{IntTy}, RetType{IntTy}, EvalCallAsPure)
                        .ArgConstraint(
                            ArgumentCondition(0U, OutOfRange, SingleValue(1)))
                        .ArgConstraint(
                            ArgumentCondition(0U, OutOfRange, SingleValue(2)))}},
         {"__buf_size_arg_constraint",
          Summaries{Summary(ArgTypes{ConstVoidPtrTy, SizeTy}, RetType{IntTy},
                            EvalCallAsPure)
                        .ArgConstraint(BufferSize(0, 1))}},
+        {"__buf_size_arg_constraint_mul",
+         Summaries{Summary(ArgTypes{ConstVoidPtrTy, SizeTy, SizeTy}, RetType{IntTy},
+                           EvalCallAsPure)
+                       .ArgConstraint(BufferSize(0, 1, 2))}},
     };
     for (auto &E : TestFunctionSummaryMap) {
       auto InsertRes =
           FunctionSummaryMap.insert({std::string(E.getKey()), E.getValue()});
       assert(InsertRes.second &&
              "Test functions must not clash with modeled functions");
       (void)InsertRes;
     }

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

void test_buf_size_symbolic_and_offset(int s) {
   char buf[3];
   __buf_size_arg_constraint(buf + 1, s);
   clang_analyzer_eval(s <= 2); // \
   // report-warning{{TRUE}} \
   // bugpath-warning{{TRUE}} \
   // bugpath-note{{TRUE}} \
   // bugpath-note{{'s' is <= 2}}
 }
+int __buf_size_arg_constraint_mul(const void *, size_t, size_t);
+void test_buf_size_concrete_with_multiplication() {
+  short buf[3];         // bugpath-note{{'buf' initialized here}}
+  __buf_size_arg_constraint_mul(buf, 4, sizeof(short)); // \
+  // 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 test_buf_size_symbolic_with_multiplication(size_t s) {
+  short buf[3];
+  __buf_size_arg_constraint_mul(buf, s, sizeof(short));
+  clang_analyzer_eval(s * sizeof(short) <= 6); // \
+  // report-warning{{TRUE}} \
+  // bugpath-warning{{TRUE}} \
+  // bugpath-note{{TRUE}}
+}
+void test_buf_size_symbolic_and_offset_with_multiplication(size_t s) {
+  short buf[3];
+  __buf_size_arg_constraint_mul(buf + 1, s, sizeof(short));
+  clang_analyzer_eval(s * sizeof(short) <= 4); // \
+  // report-warning{{TRUE}} \
+  // bugpath-warning{{TRUE}} \
+  // bugpath-note{{TRUE}}
+}