vsk created this revision.
The pointer overflow check gives false negatives when dealing with
expressions in which an unsigned value is subtracted from a pointer.
This is summarized in PR33430 [1]: ubsan permits the result of the
submission to be greater than "p", but it should not.
To fix the issue, we should track whether or not the pointer expression
is a subtraction. If it is, and the indices are unsigned, we know to
expect "p - <unsigned> <= p".
I've tested this by running check-{llvm,clang} with a stage2 ubsan-enabled
build. I've also added some tests to compiler-rt, which I'll upload in a
separate patch.
[1] https://bugs.llvm.org/show_bug.cgi?id=33430
https://reviews.llvm.org/D34121
Files:
lib/CodeGen/CGExpr.cpp
lib/CodeGen/CGExprScalar.cpp
lib/CodeGen/CodeGenFunction.h
test/CodeGen/ubsan-pointer-overflow.m
Index: test/CodeGen/ubsan-pointer-overflow.m
===================================================================
--- test/CodeGen/ubsan-pointer-overflow.m
+++ test/CodeGen/ubsan-pointer-overflow.m
@@ -10,16 +10,20 @@
++p;
// CHECK: ptrtoint i8* {{.*}} to i64, !nosanitize
- // CHECK-NEXT: add i64 {{.*}}, -1, !nosanitize
- // CHECK: select i1 false{{.*}}, !nosanitize
+ // CHECK-NEXT: [[COMPGEP:%.*]] = add i64 {{.*}}, -1, !nosanitize
+ // CHECK: [[NEGVALID:%.*]] = icmp ule i64 [[COMPGEP]], {{.*}}, !nosanitize
+ // CHECK-NOT: select
+ // CHECK: br i1 [[NEGVALID]]{{.*}}, !nosanitize
// CHECK: call void @__ubsan_handle_pointer_overflow{{.*}}
--p;
+ // CHECK: icmp uge i64
// CHECK-NOT: select
// CHECK: call void @__ubsan_handle_pointer_overflow{{.*}}
p++;
- // CHECK: select
+ // CHECK: icmp ule i64
+ // CHECK-NOT: select
// CHECK: call void @__ubsan_handle_pointer_overflow{{.*}}
p--;
}
@@ -64,7 +68,8 @@
// CHECK: [[OFFSET:%.*]] = sub i64 0, {{.*}}
// CHECK-NEXT: getelementptr inbounds {{.*}} [[OFFSET]]
- // CHECK: select
+ // CHECK: icmp ule i64
+ // CHECK-NOT: select
// CHECK: call void @__ubsan_handle_pointer_overflow{{.*}}
p - i;
}
Index: lib/CodeGen/CodeGenFunction.h
===================================================================
--- lib/CodeGen/CodeGenFunction.h
+++ lib/CodeGen/CodeGenFunction.h
@@ -3555,9 +3555,12 @@
/// 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::Value *Ptr,
ArrayRef<llvm::Value *> IdxList,
bool SignedIndices,
+ bool IsSubtraction,
SourceLocation Loc,
const Twine &Name = "");
Index: lib/CodeGen/CGExprScalar.cpp
===================================================================
--- lib/CodeGen/CGExprScalar.cpp
+++ lib/CodeGen/CGExprScalar.cpp
@@ -1851,7 +1851,6 @@
llvm::Value *input;
int amount = (isInc ? 1 : -1);
- bool signedIndex = !isInc;
if (const AtomicType *atomicTy = type->getAs<AtomicType>()) {
type = atomicTy->getValueType();
@@ -1941,8 +1940,9 @@
if (CGF.getLangOpts().isSignedOverflowDefined())
value = Builder.CreateGEP(value, numElts, "vla.inc");
else
- value = CGF.EmitCheckedInBoundsGEP(value, numElts, signedIndex,
- E->getExprLoc(), "vla.inc");
+ value = CGF.EmitCheckedInBoundsGEP(
+ value, numElts, /*SignedIndices=*/false, /*IsSubtraction=*/!isInc,
+ E->getExprLoc(), "vla.inc");
// Arithmetic on function pointers (!) is just +-1.
} else if (type->isFunctionType()) {
@@ -1952,7 +1952,8 @@
if (CGF.getLangOpts().isSignedOverflowDefined())
value = Builder.CreateGEP(value, amt, "incdec.funcptr");
else
- value = CGF.EmitCheckedInBoundsGEP(value, amt, signedIndex,
+ value = CGF.EmitCheckedInBoundsGEP(value, amt, /*SignedIndices=*/false,
+ /*IsSubtraction=*/!isInc,
E->getExprLoc(), "incdec.funcptr");
value = Builder.CreateBitCast(value, input->getType());
@@ -1962,7 +1963,8 @@
if (CGF.getLangOpts().isSignedOverflowDefined())
value = Builder.CreateGEP(value, amt, "incdec.ptr");
else
- value = CGF.EmitCheckedInBoundsGEP(value, amt, signedIndex,
+ value = CGF.EmitCheckedInBoundsGEP(value, amt, /*SignedIndices=*/false,
+ /*IsSubtraction=*/!isInc,
E->getExprLoc(), "incdec.ptr");
}
@@ -2044,8 +2046,9 @@
if (CGF.getLangOpts().isSignedOverflowDefined())
value = Builder.CreateGEP(value, sizeValue, "incdec.objptr");
else
- value = CGF.EmitCheckedInBoundsGEP(value, sizeValue, signedIndex,
- E->getExprLoc(), "incdec.objptr");
+ value = CGF.EmitCheckedInBoundsGEP(
+ value, sizeValue, /*SignedIndices=*/false, /*IsSubtraction=*/!isInc,
+ E->getExprLoc(), "incdec.objptr");
value = Builder.CreateBitCast(value, input->getType());
}
@@ -2663,7 +2666,6 @@
}
bool isSigned = indexOperand->getType()->isSignedIntegerOrEnumerationType();
- bool mayHaveNegativeGEPIndex = isSigned || isSubtraction;
unsigned width = cast<llvm::IntegerType>(index->getType())->getBitWidth();
auto &DL = CGF.CGM.getDataLayout();
@@ -2715,7 +2717,7 @@
} else {
index = CGF.Builder.CreateNSWMul(index, numElements, "vla.index");
pointer =
- CGF.EmitCheckedInBoundsGEP(pointer, index, mayHaveNegativeGEPIndex,
+ CGF.EmitCheckedInBoundsGEP(pointer, index, isSigned, isSubtraction,
op.E->getExprLoc(), "add.ptr");
}
return pointer;
@@ -2733,7 +2735,7 @@
if (CGF.getLangOpts().isSignedOverflowDefined())
return CGF.Builder.CreateGEP(pointer, index, "add.ptr");
- return CGF.EmitCheckedInBoundsGEP(pointer, index, mayHaveNegativeGEPIndex,
+ return CGF.EmitCheckedInBoundsGEP(pointer, index, isSigned, isSubtraction,
op.E->getExprLoc(), "add.ptr");
}
@@ -3853,6 +3855,7 @@
Value *CodeGenFunction::EmitCheckedInBoundsGEP(Value *Ptr,
ArrayRef<Value *> IdxList,
bool SignedIndices,
+ bool IsSubtraction,
SourceLocation Loc,
const Twine &Name) {
Value *GEPVal = Builder.CreateInBoundsGEP(Ptr, IdxList, Name);
@@ -3958,15 +3961,19 @@
// pointer matches the sign of the total offset.
llvm::Value *ValidGEP;
auto *NoOffsetOverflow = Builder.CreateNot(OffsetOverflows);
- auto *PosOrZeroValid = Builder.CreateICmpUGE(ComputedGEP, IntPtr);
if (SignedIndices) {
+ auto *PosOrZeroValid = Builder.CreateICmpUGE(ComputedGEP, IntPtr);
auto *PosOrZeroOffset = Builder.CreateICmpSGE(TotalOffset, Zero);
llvm::Value *NegValid = Builder.CreateICmpULT(ComputedGEP, IntPtr);
ValidGEP = Builder.CreateAnd(
Builder.CreateSelect(PosOrZeroOffset, PosOrZeroValid, NegValid),
NoOffsetOverflow);
- } else {
+ } else if (!SignedIndices && !IsSubtraction) {
+ auto *PosOrZeroValid = Builder.CreateICmpUGE(ComputedGEP, IntPtr);
ValidGEP = Builder.CreateAnd(PosOrZeroValid, NoOffsetOverflow);
+ } else if (!SignedIndices && IsSubtraction) {
+ auto *NegOrZeroValid = Builder.CreateICmpULE(ComputedGEP, IntPtr);
+ ValidGEP = Builder.CreateAnd(NegOrZeroValid, NoOffsetOverflow);
}
llvm::Constant *StaticArgs[] = {EmitCheckSourceLocation(Loc)};
Index: lib/CodeGen/CGExpr.cpp
===================================================================
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -3006,7 +3006,8 @@
SourceLocation loc,
const llvm::Twine &name = "arrayidx") {
if (inbounds) {
- return CGF.EmitCheckedInBoundsGEP(ptr, indices, signedIndices, loc, name);
+ return CGF.EmitCheckedInBoundsGEP(ptr, indices, signedIndices,
+ /*IsSubtraction=*/false, loc, name);
} else {
return CGF.Builder.CreateGEP(ptr, indices, name);
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
