[C++ BoundsSafety] Fix false positives when pointer argument is a function call

[ziqingluo-90]

This PR attempts to fix false positives in such an example:

void cb(int *__counted_by(count) p, size_t count);
int *__counted_by(n) f(size_t n);

void test(size_t len) {
  cb(f(len), len); // false positive
}

The analysis needs to compare the size of f(len), which is specified by __counted_by(n), with the expected count of the first argument of cb, which is specified by __counted_by(count). The comparison interprets the two comparands at two "call contexts" respectively: the count n needs to be interpreted at the call f(len) with a mapping {n -> len} and the count count needs to be interpreted at the call cb(f(len), len) with a mapping {p -> f(len), count -> len}.

The existing compare algorithm is extended from assuming only one comparand needs a substitution map to assuming both comparands need substitution maps.

rdar://155952016

[ziqingluo-90]

@patrykstefanski This is my attempt to extend the existing CompatibleCountExprVisitor to take two substitution maps, though I think we've reached the limit of this Visitor-based comparison approach. It traverses only one of the comparands so it does not know when to apply substitution for the other comparand. The result is that I have to trySubstitute at each of the overloaded methods.
I'd like to refactor this comparison code later.

[patrykstefanski]

It seems that both trySimplifyDerefAddressof and trySubstitute attempt to simplify and substitute expressions. Maybe we could consider combining them into a single function?

I was thinking about something like this:

  // Simplify '*&e' and/or substitute decl. We want to perform both at the same
  // time, since this pattern might occur after a substitution:
  //   E: *x, DependentValues: {x->&y} which results in *&y
  std::pair<const Expr *, bool>
  trySimplifyAndSubstitute(const Expr *const E, bool AllowSubst,
                           const DependentValuesTy *DependentValues) const {
    const Expr *X = E->IgnoreParenImpCasts();

    bool HasDeref = false;
    if (const auto *DerefOp = dyn_cast<UnaryOperator>(X);
        DerefOp && DerefOp->getOpcode() == UO_Deref) {
      HasDeref = true;
      X = DerefOp->getSubExpr()->IgnoreParenImpCasts();
    }

    auto trySubstitute = [&](const Expr *E) -> std::pair<const Expr *, bool> {
      if (AllowSubst && DependentValues) {
        if (const auto *DRE = dyn_cast<DeclRefExpr>(E)) {
          if (auto It = DependentValues->find(DRE->getDecl());
              It != DependentValues->end())
            return {It->second->IgnoreParenImpCasts(), true};
        }
      }
      return {E, false};
    };

    bool Substituted = false;
    std::tie(X, Substituted) = trySubstitute(X);

    if (HasDeref) {
      const auto *AddrOfOp = dyn_cast<UnaryOperator>(X);
      if (!AddrOfOp || AddrOfOp->getOpcode() != UO_AddrOf)
        return {E, false};
      X = AddrOfOp->getSubExpr()->IgnoreParenImpCasts();
    }

    if (!Substituted)
      std::tie(X, Substituted) = trySubstitute(X);

    return {X, Substituted};
  }

In that case, VisitDeclRefExpr and VisitUnaryOperator could look like this:

  bool VisitDeclRefExpr(const DeclRefExpr *SelfDRE, const Expr *Other,
                        bool hasSelfBeenSubstituted,
                        bool hasOtherBeenSubstituted) {
    const ValueDecl *SelfVD = SelfDRE->getDecl();

    const auto [Self, Substituted] = trySimplifyAndSubstitute(
        SelfDRE, !hasSelfBeenSubstituted, DependentValuesSelf);
    if (Self != SelfDRE) {
      // We only simplify '*&e', this couldn't happen here, since we have a DRE.
      assert(Substituted);
      // We substituted the expr, now recurse.
      return Visit(Self, Other, true, hasOtherBeenSubstituted);
    }

    std::tie(Other, hasOtherBeenSubstituted) = trySimplifyAndSubstitute(
        Other, !hasOtherBeenSubstituted, DependentValuesOther);

    const auto *O = Other->IgnoreParenImpCasts();

    if (const auto *OtherDRE = dyn_cast<DeclRefExpr>(O)) {
      // Both SelfDRE and OtherDRE can be transformed from member expressions:
      if (OtherDRE->getDecl() == SelfVD)
        return true;
      return false;
    }

    const auto *OtherME = dyn_cast<MemberExpr>(O);
    if (MemberBase && OtherME) {
      return OtherME->getMemberDecl() == SelfVD &&
             Visit(OtherME->getBase(), MemberBase, hasSelfBeenSubstituted,
                   hasOtherBeenSubstituted);
    }

    return false;
  }

  bool VisitUnaryOperator(const UnaryOperator *SelfUO, const Expr *Other,
                          bool hasSelfBeenSubstituted,
                          bool hasOtherBeenSubstituted) {
    if (SelfUO->getOpcode() != UO_Deref)
      return false; // We don't support any other unary operator

    const auto &[SimplifiedSelf, Substituted] = trySimplifyAndSubstitute(
        SelfUO, !hasSelfBeenSubstituted, DependentValuesSelf);
    if (SelfUO != SimplifiedSelf) {
      return Visit(SimplifiedSelf, Other, Substituted, hasOtherBeenSubstituted);
    }

    std::tie(Other, hasOtherBeenSubstituted) = trySimplifyAndSubstitute(
        Other, !hasOtherBeenSubstituted, DependentValuesOther);

    if (const auto *OtherUO =
            dyn_cast<UnaryOperator>(Other->IgnoreParenImpCasts())) {
      if (SelfUO->getOpcode() == OtherUO->getOpcode())
        return Visit(SelfUO->getSubExpr(), OtherUO->getSubExpr(),
                     hasSelfBeenSubstituted, hasOtherBeenSubstituted);
    }

    return false;
  }

What do you think?

[ziqingluo-90]

Yep, this would work! I'm fine with keeping this traverse-based comparison until we run into more challenges (if there would be any).

On the other hand, I made an experimental change: #11148 for potentially more intuitive substitution and comparison.

[patrykstefanski]

Was there anything wrong with my version of trySimplifyAndSubstitute that you decided to implement this differently?

[patrykstefanski]

Do we have tests for such casts?

[ziqingluo-90]

Oh, this is part of the refactoring I plan to do after this patch. I forgot to clean it up for now.

We have three functions doing similar job:

llvm-project/clang/lib/Analysis/UnsafeBufferUsage.cpp

Line 1303 in 876e890

static bool isPtrBufferSafe(const Expr *Ptr, const Expr *Size,

llvm-project/clang/lib/Analysis/UnsafeBufferUsage.cpp

Line 1443 in 876e890

static bool isSafeSpanTwoParamConstruct(const CXXConstructExpr &Node,

and

llvm-project/clang/lib/Analysis/UnsafeBufferUsage.cpp

Line 1168 in 876e890

static bool isHardcodedCountedByPointerArgumentSafe(

I plan to merge them into one.

[ziqingluo-90]

This is causing some issues like the pattern of (p, strlen(p)) is only supported by span construction but not __sized_by parameters.

[patrykstefanski]

Was there anything wrong with my version of trySimplifyAndSubstitute that you decided to implement this differently?

[patrykstefanski]

We could also avoid duplicating this pattern:

        auto I = DependentValues->find(DRE->getDecl());
        if (I != DependentValues->end())
         ...

We have this 2x in this function + one more in VisitDeclRefExpr().

[patrykstefanski]

Nit: A matter of taste, but if we pass !hasOtherBeenSubstituted to trySubstituteAndSimplify, we could avoid guarding this with if (!hasOtherBeenSubstituted) in every place.

[patrykstefanski]

const auto

[patrykstefanski]

const auto

[patrykstefanski]

This is returned by value, we could remove const.

[patrykstefanski]

The function could be const though: trySubstituteAndSimplify(...) const {.

[patrykstefanski]

We could move

    if (SelfUO->getOpcode() != UO_Deref)
      return false; // We don't support any other unary operator

before

    if (!hasOtherBeenSubstituted)
      std::tie(Other, hasOtherBeenSubstituted) =
          trySubstituteAndSimplify(Other, DependentValuesOther);

In this case, if opcode is not UO_Deref, we could return early without doing substitution and hashmap lookup.