From: Richard Smith Date: Wed, 23 Jan 2019 03:37:29 +0000 (+0000) Subject: [ubsan] Check the correct size when sanitizing array new. X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=125802222ee9a90f861c231e986a7005e0622b0a;p=clang [ubsan] Check the correct size when sanitizing array new. We previously forgot to multiply the element size by the array bound. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@351924 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/CodeGen/CGExpr.cpp b/lib/CodeGen/CGExpr.cpp index c8df3a4f64..be30e8215a 100644 --- a/lib/CodeGen/CGExpr.cpp +++ b/lib/CodeGen/CGExpr.cpp @@ -652,7 +652,8 @@ bool CodeGenFunction::sanitizePerformTypeCheck() const { void CodeGenFunction::EmitTypeCheck(TypeCheckKind TCK, SourceLocation Loc, llvm::Value *Ptr, QualType Ty, CharUnits Alignment, - SanitizerSet SkippedChecks) { + SanitizerSet SkippedChecks, + llvm::Value *ArraySize) { if (!sanitizePerformTypeCheck()) return; @@ -710,21 +711,27 @@ void CodeGenFunction::EmitTypeCheck(TypeCheckKind TCK, SourceLocation Loc, if (SanOpts.has(SanitizerKind::ObjectSize) && !SkippedChecks.has(SanitizerKind::ObjectSize) && !Ty->isIncompleteType()) { - uint64_t Size = getContext().getTypeSizeInChars(Ty).getQuantity(); - - // The glvalue must refer to a large enough storage region. - // FIXME: If Address Sanitizer is enabled, insert dynamic instrumentation - // to check this. - // FIXME: Get object address space - llvm::Type *Tys[2] = { IntPtrTy, Int8PtrTy }; - llvm::Value *F = CGM.getIntrinsic(llvm::Intrinsic::objectsize, Tys); - llvm::Value *Min = Builder.getFalse(); - llvm::Value *NullIsUnknown = Builder.getFalse(); - llvm::Value *CastAddr = Builder.CreateBitCast(Ptr, Int8PtrTy); - llvm::Value *LargeEnough = Builder.CreateICmpUGE( - Builder.CreateCall(F, {CastAddr, Min, NullIsUnknown}), - llvm::ConstantInt::get(IntPtrTy, Size)); - Checks.push_back(std::make_pair(LargeEnough, SanitizerKind::ObjectSize)); + uint64_t TySize = getContext().getTypeSizeInChars(Ty).getQuantity(); + llvm::Value *Size = llvm::ConstantInt::get(IntPtrTy, TySize); + if (ArraySize) + Size = Builder.CreateMul(Size, ArraySize); + + // Degenerate case: new X[0] does not need an objectsize check. + llvm::Constant *ConstantSize = dyn_cast(Size); + if (!ConstantSize || !ConstantSize->isNullValue()) { + // The glvalue must refer to a large enough storage region. + // FIXME: If Address Sanitizer is enabled, insert dynamic instrumentation + // to check this. + // FIXME: Get object address space + llvm::Type *Tys[2] = { IntPtrTy, Int8PtrTy }; + llvm::Value *F = CGM.getIntrinsic(llvm::Intrinsic::objectsize, Tys); + llvm::Value *Min = Builder.getFalse(); + llvm::Value *NullIsUnknown = Builder.getFalse(); + llvm::Value *CastAddr = Builder.CreateBitCast(Ptr, Int8PtrTy); + llvm::Value *LargeEnough = Builder.CreateICmpUGE( + Builder.CreateCall(F, {CastAddr, Min, NullIsUnknown}), Size); + Checks.push_back(std::make_pair(LargeEnough, SanitizerKind::ObjectSize)); + } } uint64_t AlignVal = 0; diff --git a/lib/CodeGen/CGExprCXX.cpp b/lib/CodeGen/CGExprCXX.cpp index 76cb94618d..d2a7f11982 100644 --- a/lib/CodeGen/CGExprCXX.cpp +++ b/lib/CodeGen/CGExprCXX.cpp @@ -1714,10 +1714,16 @@ llvm::Value *CodeGenFunction::EmitCXXNewExpr(const CXXNewExpr *E) { result.getAlignment()); // Emit sanitizer checks for pointer value now, so that in the case of an - // array it was checked only once and not at each constructor call. + // array it was checked only once and not at each constructor call. We may + // have already checked that the pointer is non-null. + // FIXME: If we have an array cookie and a potentially-throwing allocator, + // we'll null check the wrong pointer here. + SanitizerSet SkippedChecks; + SkippedChecks.set(SanitizerKind::Null, nullCheck); EmitTypeCheck(CodeGenFunction::TCK_ConstructorCall, - E->getAllocatedTypeSourceInfo()->getTypeLoc().getBeginLoc(), - result.getPointer(), allocType); + E->getAllocatedTypeSourceInfo()->getTypeLoc().getBeginLoc(), + result.getPointer(), allocType, result.getAlignment(), + SkippedChecks, numElements); EmitNewInitializer(*this, E, allocType, elementTy, result, numElements, allocSizeWithoutCookie); diff --git a/lib/CodeGen/CodeGenFunction.h b/lib/CodeGen/CodeGenFunction.h index 792caa9c08..90c99e09fc 100644 --- a/lib/CodeGen/CodeGenFunction.h +++ b/lib/CodeGen/CodeGenFunction.h @@ -2620,10 +2620,12 @@ public: bool sanitizePerformTypeCheck() const; /// Emit a check that \p V is the address of storage of the - /// appropriate size and alignment for an object of type \p Type. + /// appropriate size and alignment for an object of type \p Type + /// (or if ArraySize is provided, for an array of that bound). void EmitTypeCheck(TypeCheckKind TCK, SourceLocation Loc, llvm::Value *V, QualType Type, CharUnits Alignment = CharUnits::Zero(), - SanitizerSet SkippedChecks = SanitizerSet()); + SanitizerSet SkippedChecks = SanitizerSet(), + llvm::Value *ArraySize = nullptr); /// Emit a check that \p Base points into an array object, which /// we can access at index \p Index. \p Accessed should be \c false if we diff --git a/test/CodeGenCXX/catch-undef-behavior.cpp b/test/CodeGenCXX/catch-undef-behavior.cpp index 0e8d4fa51a..ba72af038a 100644 --- a/test/CodeGenCXX/catch-undef-behavior.cpp +++ b/test/CodeGenCXX/catch-undef-behavior.cpp @@ -533,6 +533,7 @@ namespace NothrowNew { // CHECK: [[nonnull]]: // CHECK: llvm.objectsize + // CHECK: icmp uge i64 {{.*}}, 123456, // CHECK: br i1 // // CHECK: call {{.*}}__ubsan_handle_type_mismatch @@ -550,6 +551,7 @@ namespace NothrowNew { // CHECK: [[nonnull]]: // CHECK: llvm.objectsize + // CHECK: icmp uge i64 {{.*}}, 123456, // CHECK: br i1 // // CHECK: call {{.*}}__ubsan_handle_type_mismatch @@ -561,6 +563,47 @@ namespace NothrowNew { // CHECK: ret return new (nothrow{}) X[123456]; } + + // CHECK-LABEL: define{{.*}}throwing_new + void *throwing_new(int size) { + // CHECK: icmp ne i8*{{.*}}, null + // CHECK: %[[size:.*]] = mul + // CHECK: llvm.objectsize + // CHECK: icmp uge i64 {{.*}}, %[[size]], + // CHECK: %[[ok:.*]] = and + // CHECK: br i1 %[[ok]], label %[[good:.*]], label %[[bad:[^,]*]] + // + // CHECK: [[bad]]: + // CHECK: call {{.*}}__ubsan_handle_type_mismatch + // + // CHECK: [[good]]: + // CHECK-NOT: {{ }}br{{ }} + // CHECK: ret + return new char[size]; + } + + // CHECK-LABEL: define{{.*}}nothrow_new_zero_size + void *nothrow_new_zero_size() { + // CHECK: %[[nonnull:.*]] = icmp ne i8*{{.*}}, null + // CHECK-NOT: llvm.objectsize + // CHECK: br i1 %[[nonnull]], label %[[good:.*]], label %[[bad:[^,]*]] + // + // CHECK: [[bad]]: + // CHECK: call {{.*}}__ubsan_handle_type_mismatch + // + // CHECK: [[good]]: + // CHECK-NOT: {{ }}br{{ }} + // CHECK: ret + return new char[0]; + } + + // CHECK-LABEL: define{{.*}}throwing_new_zero_size + void *throwing_new_zero_size() { + // Nothing to check here. + // CHECK-NOT: __ubsan_handle_type_mismatch + return new (nothrow{}) char[0]; + // CHECK: ret + } } struct ThisAlign {