[clang][BoundsSafety][UBSan] Fix false-positive UBSan pointer overflow

[usama54321]

check for pointer subtraction

When -fbounds-safety and -fsanitize=pointer-overflow are both enabled, pointer subtraction unconditionally triggers a UBSan pointer overflow false positive. EmitCheckedBoundPointerArithmetic was passing the raw positive index to EmitCheckedInBoundsGEP without negating it first, causing the check to verify (base + offset) ule base instead of (base - offset) ule base, which is always false for any positive offset.

All other callers of EmitCheckedInBoundsGEP pre-negate the index for subtraction. Fix this by negating the index with CreateNeg before passing it to EmitCheckedInBoundsGEP when IsSub is true.

rdar://173944149

[usama54321]

@swift-ci please test

[usama54321]

@swift-ci please test

[rapidsna]

Thanks! LGTM

[rapidsna]

@swift-ci test llvm

[rapidsna]

FYI. For llvm changes in next, we just run @swift-ci test llvm, which I just invoked. swift-ci test doesn't make sense for next anyway because there's no matching swift tag for it.

[hnrklssn]

Do we want to use update_cc_test_checks?

[delcypher]

I think this would be a good idea so that this test is easier to update when upstream inevitably changes the IR.

[hnrklssn]

Note that this is an -O0 test, so the fragility is lower than the -O2 tests

[usama54321]

I would rather keep this as is. I have not used that script in the past, but it is creating around 30 CHECK lines which I think is much more brittle than this.

[delcypher]

All other callers of EmitCheckedInBoundsGEP pre-negate the index for subtraction. Fix this by negating the index with CreateNeg before passing it to EmitCheckedInBoundsGEP when IsSub is true.

Do you have concrete examples? When I look over the code it's not obvious to me that this is true.

E.g:

    // For everything else, we can just do a simple increment.
    } else {
      llvm::Value *amt = Builder.getInt32(amount);
      llvm::Type *elemTy = CGF.ConvertTypeForMem(type);
      if (CGF.getLangOpts().PointerOverflowDefined)
        value = Builder.CreateGEP(elemTy, value, amt, "incdec.ptr");
      else
        value = CGF.EmitCheckedInBoundsGEP(
            elemTy, value, amt, /*SignedIndices=*/false, isSubtraction,
            E->getExprLoc(), "incdec.ptr");
    }

[delcypher]

So I'm a little confused by this. IsSub is already being passed to the IsSubtraction parameter of EmitCheckedInBoundsGEP so the method knows we are doing a subtraction so it seems surprising that we would need to negate Idx.

Are we passing the wrong value to the SignedIndices parameter of EmitCheckedInBoundsGEP?

[usama54321]

I was pretty confused by this as well and looked at a number of callsites. Looking at the complete code you pasted above, it negates the index in the first case, does not in the second case (which might be a bug?). The last case is just a plain increment at least according to the comment.

I see the negation in other places as well.

if (const VariableArrayType *vla
          = CGF.getContext().getAsVariableArrayType(type)) {
      llvm::Value *numElts = CGF.getVLASize(vla).NumElts;
      if (!isInc) numElts = Builder.CreateNSWNeg(numElts, "vla.negsize");
      llvm::Type *elemTy = CGF.ConvertTypeForMem(vla->getElementType());
      if (CGF.getLangOpts().PointerOverflowDefined)
        value = Builder.CreateGEP(elemTy, value, numElts, "vla.inc");
      else
        value = CGF.EmitCheckedInBoundsGEP(
            elemTy, value, numElts, /*SignedIndices=*/false, isSubtraction,
            E->getExprLoc(), "vla.inc");

    // Arithmetic on function pointers (!) is just +-1.
    } else if (type->isFunctionType()) {
      llvm::Value *amt = Builder.getInt32(amount);

      if (CGF.getLangOpts().PointerOverflowDefined)
        value = Builder.CreateGEP(CGF.Int8Ty, value, amt, "incdec.funcptr");
      else
        value =
            CGF.EmitCheckedInBoundsGEP(CGF.Int8Ty, value, amt,
                                       /*SignedIndices=*/false, isSubtraction,
                                       E->getExprLoc(), "incdec.funcptr");

    // For everything else, we can just do a simple increment.
    } else {
      llvm::Value *amt = Builder.getInt32(amount);
      llvm::Type *elemTy = CGF.ConvertTypeForMem(type);
      if (CGF.getLangOpts().PointerOverflowDefined)
        value = Builder.CreateGEP(elemTy, value, amt, "incdec.ptr");
      else
        value = CGF.EmitCheckedInBoundsGEP(
            elemTy, value, amt, /*SignedIndices=*/false, isSubtraction,
            E->getExprLoc(), "incdec.ptr");
    }

SignedIndices is independent of this. For example in the test I wrote, SignedIndices = false and IsSub = true, while if i replace ptr - offset; with ptr - 3; SignedIndices = true and IsSub = false (which makes sense).

[usama54321]

I will double check this again and add some more tests

[delcypher]

The last case is just a plain increment at least according to the comment.

I think that comment is misleading because
isSubtraction is bool isSubtraction = !isInc; on that path I think so this can still be a decrement.

[delcypher]

SignedIndices = false and IsSub = true, while if i replace ptr - offset; with ptr - 3; SignedIndices = true and IsSub = false (which makes sense).

That interesting. Sounds like when we have a constant we effectively are treating this as ptr + (-3).

I'm still very suspicious because given that we are telling EmitCheckedInBoundsGEP:

• We are doing subtraction
• The index is an unsigned value

would suggest there isn't a need to negate the index at all. However a quick glance at the implementation shows:

Value *
CodeGenFunction::EmitCheckedInBoundsGEP(llvm::Type *ElemTy, Value *Ptr,
                                        ArrayRef<Value *> IdxList,
                                        bool SignedIndices, bool IsSubtraction,
                                        SourceLocation Loc, const Twine &Name) {
  llvm::Type *PtrTy = Ptr->getType();

  llvm::GEPNoWrapFlags NWFlags = llvm::GEPNoWrapFlags::inBounds();
  if (!SignedIndices && !IsSubtraction)
    NWFlags |= llvm::GEPNoWrapFlags::noUnsignedWrap();

  Value *GEPVal = Builder.CreateGEP(ElemTy, Ptr, IdxList, Name, NWFlags);

  // stuff that does use `IsSubtraction` for computing overflow checking.


  return GEPVal;

It seems at least for the computed value we return, the IsSubtraction isn't really used other than setting the wrapping flags. This makes it seem like IsSubtraction is a bit of a footgun.

[usama54321]

Yes this seems to be bad API design, and fixing it would mean updating all the callsites. If you want I can create a radar and fix that upstream but we would also need a separate patch in this repo for bounds-safety usages. What would you recommend?

[delcypher]

The origin of this seems to be from https://reviews.llvm.org/D34121.

So perhaps this is expected behavior because the doxygen comments say

  /// Same as IRBuilder::CreateInBoundsGEP, but additionally emits a check to
  /// detect undefined behavior when the pointer overflow sanitizer is enabled.
  /// \p SignedIndices indicates whether any of the GEP indices are signed.
  /// \p IsSubtraction indicates whether the expression used to form the GEP
  /// is a subtraction.
  llvm::Value *EmitCheckedInBoundsGEP(llvm::Type *ElemTy, llvm::Value *Ptr,
                                      ArrayRef<llvm::Value *> IdxList,
                                      bool SignedIndices, bool IsSubtraction,
                                      SourceLocation Loc,
                                      const Twine &Name = "");

and IRBuilder::CreateInBoundsGEP doesn't have any notion of subtraction. Everything is an addition and I think you have to manually negate any value in the IdxList first if you want to do negative indexing. Also because IdxList is a list and not a single llvm::Value* which of those should be subtracted?

That being said this is pretty confusing. I think we should at the least file an issue upstream and try to clean this up in a separate PR.

I don't want to block fixing the -fbounds-safety/ubsan bug on that though.

[delcypher]

Oh something to think about here too is how this interacts with SanitizerKind::ArrayBounds and if that expects Idx to be negated or not.

[usama54321]

Yep that also expects the index to be negated. I took a look at the code/few callsites.

[usama54321]

I will update with more tests and resolve this conversation afterwards