https://github.com/Zonotora updated https://github.com/llvm/llvm-project/pull/75481
>From 3b0af99a0e13ed9d6b8c0365a9fa7ffffea68ec5 Mon Sep 17 00:00:00 2001 From: Zonotora <zonot...@hotmail.com> Date: Sat, 16 Dec 2023 19:33:21 +0100 Subject: [PATCH 1/2] [clang] Extract negativity check lambda to function --- clang/lib/CodeGen/CGExprScalar.cpp | 47 ++++++++++++++++-------------- 1 file changed, 25 insertions(+), 22 deletions(-) diff --git a/clang/lib/CodeGen/CGExprScalar.cpp b/clang/lib/CodeGen/CGExprScalar.cpp index df8f71cf1d900..b8caafb00847d 100644 --- a/clang/lib/CodeGen/CGExprScalar.cpp +++ b/clang/lib/CodeGen/CGExprScalar.cpp @@ -1097,6 +1097,27 @@ void ScalarExprEmitter::EmitIntegerTruncationCheck(Value *Src, QualType SrcType, {Src, Dst}); } +static llvm::Value *EmitIsNegativeTestHelper(Value *V, QualType VType, + const char *Name, + CGBuilderTy &Builder) { + // NOTE: zero value is considered to be non-negative. + // Is this value a signed type? + bool VSigned = VType->isSignedIntegerOrEnumerationType(); + llvm::Type *VTy = V->getType(); + if (!VSigned) { + // If the value is unsigned, then it is never negative. + // FIXME: can we encounter non-scalar VTy here? + return llvm::ConstantInt::getFalse(VTy->getContext()); + } + // Get the zero of the same type with which we will be comparing. + llvm::Constant *Zero = llvm::ConstantInt::get(VTy, 0); + // %V.isnegative = icmp slt %V, 0 + // I.e is %V *strictly* less than zero, does it have negative value? + return Builder.CreateICmp(llvm::ICmpInst::ICMP_SLT, V, Zero, + llvm::Twine(Name) + "." + V->getName() + + ".negativitycheck"); +}; + // Should be called within CodeGenFunction::SanitizerScope RAII scope. // Returns 'i1 false' when the conversion Src -> Dst changed the sign. static std::pair<ScalarExprEmitter::ImplicitConversionCheckKind, @@ -1121,30 +1142,12 @@ EmitIntegerSignChangeCheckHelper(Value *Src, QualType SrcType, Value *Dst, assert(((SrcBits != DstBits) || (SrcSigned != DstSigned)) && "either the widths should be different, or the signednesses."); - // NOTE: zero value is considered to be non-negative. - auto EmitIsNegativeTest = [&Builder](Value *V, QualType VType, - const char *Name) -> Value * { - // Is this value a signed type? - bool VSigned = VType->isSignedIntegerOrEnumerationType(); - llvm::Type *VTy = V->getType(); - if (!VSigned) { - // If the value is unsigned, then it is never negative. - // FIXME: can we encounter non-scalar VTy here? - return llvm::ConstantInt::getFalse(VTy->getContext()); - } - // Get the zero of the same type with which we will be comparing. - llvm::Constant *Zero = llvm::ConstantInt::get(VTy, 0); - // %V.isnegative = icmp slt %V, 0 - // I.e is %V *strictly* less than zero, does it have negative value? - return Builder.CreateICmp(llvm::ICmpInst::ICMP_SLT, V, Zero, - llvm::Twine(Name) + "." + V->getName() + - ".negativitycheck"); - }; - // 1. Was the old Value negative? - llvm::Value *SrcIsNegative = EmitIsNegativeTest(Src, SrcType, "src"); + llvm::Value *SrcIsNegative = + EmitIsNegativeTestHelper(Src, SrcType, "src", Builder); // 2. Is the new Value negative? - llvm::Value *DstIsNegative = EmitIsNegativeTest(Dst, DstType, "dst"); + llvm::Value *DstIsNegative = + EmitIsNegativeTestHelper(Dst, DstType, "dst", Builder); // 3. Now, was the 'negativity status' preserved during the conversion? // NOTE: conversion from negative to zero is considered to change the sign. // (We want to get 'false' when the conversion changed the sign) >From 49e3c6be7ed4e3dcb403b2ea1994dafb6e3d638e Mon Sep 17 00:00:00 2001 From: Zonotora <zonot...@hotmail.com> Date: Sat, 16 Dec 2023 19:36:04 +0100 Subject: [PATCH 2/2] [clang][UBSan] Add implicit conversion check for bitfields This patch implements the implicit truncation and implicit sign change checks for bitfields. --- clang/docs/ReleaseNotes.rst | 25 ++ clang/docs/UndefinedBehaviorSanitizer.rst | 47 ++- clang/include/clang/Basic/Sanitizers.def | 32 +- clang/lib/CodeGen/CGExpr.cpp | 33 +- clang/lib/CodeGen/CGExprScalar.cpp | 332 +++++++++++++++++- clang/lib/CodeGen/CodeGenFunction.h | 14 + .../test/CodeGen/ubsan-bitfield-conversion.c | 32 ++ compiler-rt/lib/ubsan/ubsan_handlers.cpp | 64 +++- compiler-rt/lib/ubsan/ubsan_handlers.h | 12 + compiler-rt/lib/ubsan/ubsan_interface.inc | 2 + 10 files changed, 553 insertions(+), 40 deletions(-) create mode 100644 clang/test/CodeGen/ubsan-bitfield-conversion.c diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 802c44b6c8608..0f7465f66b53e 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -119,6 +119,26 @@ Non-comprehensive list of changes in this release New Compiler Flags ------------------ +- ``-fsanitize=implicit-unsigned-bitfield-truncation`` catches implicit + unsigned conversions involving bitfields. +- ``-fsanitize=implicit-signed-bitfield-truncation`` catches implicit + signed conversions involving bitfields. +- ``-fsanitize=implicit-bitfield-sign-change`` catches implicit + conversions involving bitfields that result in a sign change. +- ``-fsanitize=implicit-bitfield-truncation`` a group to include both + ``-fsanitize=implicit-unsigned-bitfield-truncation`` and + ``-fsanitize=implicit-signed-bitfield-truncation``. +- ``-fsanitize=implicit-bitfield-arithmetic-value-change`` a group to + include both ``implicit-signed-bitfield-truncation`` and + ``implicit-bitfield-sign-change``. +- ``-fsanitize=implicit-bitfield-conversion`` a group to include + ``-fsanitize=implicit-unsigned-bitfield-truncation``, + ``-fsanitize=implicit-signed-bitfield-truncation`` and + ``implicit-bitfield-sign-change``. +- ``-fsanitize=implicit-integer-conversion`` a group to include + ``-fsanitize=implicit-unsigned-integer-truncation``, + ``-fsanitize=implicit-signed-integer-truncation`` and + ``implicit-integer-sign-change``. .. _target_os_detail: @@ -136,6 +156,11 @@ Deprecated Compiler Flags Modified Compiler Flags ----------------------- +- ``-fsanitize=implicit-conversion`` is now a group for both + ``-fsanitize=implicit-integer-conversion`` and + ``-fsanitize=implicit-bitfield-conversion``. Hence, + ``-fsanitize=implicit-integer-conversion`` has replaced what previously + was ``-fsanitize=implicit-conversion``. Removed Compiler Flags ------------------------- diff --git a/clang/docs/UndefinedBehaviorSanitizer.rst b/clang/docs/UndefinedBehaviorSanitizer.rst index b8ad3804f1890..856cb726eb446 100644 --- a/clang/docs/UndefinedBehaviorSanitizer.rst +++ b/clang/docs/UndefinedBehaviorSanitizer.rst @@ -148,6 +148,23 @@ Available checks are: Issues caught by this sanitizer are not undefined behavior, but are often unintentional. - ``-fsanitize=integer-divide-by-zero``: Integer division by zero. + - ``-fsanitize=implicit-unsigned-bitfield-truncation``, + ``-fsanitize=implicit-signed-bitfield-truncation``: Implicit conversion from + integer of larger bit width to smaller bitfield, if that results in data + loss. That is, if the demoted value, after casting back to the original + width, is not equal to the original value before the downcast. + The ``-fsanitize=implicit-unsigned-bitfield-truncation`` handles conversions + between two ``unsigned`` types, while + ``-fsanitize=implicit-signed-bitfield-truncation`` handles the rest of the + conversions - when either one, or both of the types are signed. + Issues caught by these sanitizers are not undefined behavior, + but are often unintentional. + - ``-fsanitize=implicit-bitfield-sign-change``: Implicit conversion from + integer of larger bit width to smaller bitfield, if that changes the + sign of the value. That is, if the original value was negative and the + new value is positive (or zero), or the original value was positive, + and the new value is negative. Issues caught by this sanitizer are not + undefined behavior, but are often unintentional. - ``-fsanitize=nonnull-attribute``: Passing null pointer as a function parameter which is declared to never be null. - ``-fsanitize=null``: Use of a null pointer or creation of a null @@ -192,8 +209,8 @@ Available checks are: This includes all the checks covered by ``-ftrapv``, as well as checks for signed division overflow (``INT_MIN/-1``), but not checks for lossy implicit conversions performed before the computation - (see ``-fsanitize=implicit-conversion``). Both of these two issues are - handled by ``-fsanitize=implicit-conversion`` group of checks. + (see ``-fsanitize=implicit-integer-conversion``). Both of these two issues are + handled by ``-fsanitize=implicit-integer-conversion`` group of checks. - ``-fsanitize=unreachable``: If control flow reaches an unreachable program point. - ``-fsanitize=unsigned-integer-overflow``: Unsigned integer overflow, where @@ -201,7 +218,7 @@ Available checks are: type. Unlike signed integer overflow, this is not undefined behavior, but it is often unintentional. This sanitizer does not check for lossy implicit conversions performed before such a computation - (see ``-fsanitize=implicit-conversion``). + (see ``-fsanitize=implicit-integer-conversion``). - ``-fsanitize=vla-bound``: A variable-length array whose bound does not evaluate to a positive value. - ``-fsanitize=vptr``: Use of an object whose vptr indicates that it is of @@ -213,8 +230,9 @@ Available checks are: You can also use the following check groups: - ``-fsanitize=undefined``: All of the checks listed above other than ``float-divide-by-zero``, ``unsigned-integer-overflow``, - ``implicit-conversion``, ``local-bounds`` and the ``nullability-*`` group - of checks. + ``implicit-integer-conversion``, ``implicit-bitfield-conversion``, + ``implicit-conversion``, ``local-bounds`` and the ``nullability-*`` + group of checks. - ``-fsanitize=undefined-trap``: Deprecated alias of ``-fsanitize=undefined``. - ``-fsanitize=implicit-integer-truncation``: Catches lossy integral @@ -223,11 +241,26 @@ You can also use the following check groups: - ``-fsanitize=implicit-integer-arithmetic-value-change``: Catches implicit conversions that change the arithmetic value of the integer. Enables ``implicit-signed-integer-truncation`` and ``implicit-integer-sign-change``. - - ``-fsanitize=implicit-conversion``: Checks for suspicious - behavior of implicit conversions. Enables + - ``-fsanitize=implicit-integer-conversion``: Checks for suspicious + behavior of implicit integer conversions. Enables ``implicit-unsigned-integer-truncation``, ``implicit-signed-integer-truncation``, and ``implicit-integer-sign-change``. + - ``-fsanitize=implicit-bitfield-truncation``: Catches lossy bitfield + conversions. Enables ``implicit-signed-bitfield-truncation`` and + ``implicit-unsigned-bitfield-truncation``. + - ``-fsanitize=implicit-bitfield-arithmetic-value-change``: Catches implicit + conversions that change the arithmetic value of the bitfield. Enables + ``implicit-signed-bitfield-truncation`` and ``implicit-bitfield-sign-change``. + - ``-fsanitize=implicit-bitfield-conversion``: Checks for suspicious + behavior of implicit bitfield conversions. Enables + ``implicit-unsigned-bitfield-truncation``, + ``implicit-signed-bitfield-truncation``, and + ``implicit-bitfield-sign-change``. + - ``-fsanitize=implicit-conversion``: Checks for suspicious + behavior of implicit conversions. Enables + ``implicit-integer-conversion``, and + ``implicit-bitfield-conversion``. - ``-fsanitize=integer``: Checks for undefined or suspicious integer behavior (e.g. unsigned integer overflow). Enables ``signed-integer-overflow``, ``unsigned-integer-overflow``, diff --git a/clang/include/clang/Basic/Sanitizers.def b/clang/include/clang/Basic/Sanitizers.def index c2137e3f61f64..54ccf6d355741 100644 --- a/clang/include/clang/Basic/Sanitizers.def +++ b/clang/include/clang/Basic/Sanitizers.def @@ -163,24 +163,38 @@ SANITIZER_GROUP("implicit-integer-arithmetic-value-change", ImplicitIntegerArithmeticValueChange, ImplicitIntegerSignChange | ImplicitSignedIntegerTruncation) -SANITIZER("objc-cast", ObjCCast) +SANITIZER_GROUP("implicit-integer-conversion", ImplicitIntegerConversion, + ImplicitIntegerArithmeticValueChange | + ImplicitUnsignedIntegerTruncation) + +// Implicit bitfield sanitizers +SANITIZER("implicit-unsigned-bitfield-truncation", ImplicitUnsignedBitfieldTruncation) +SANITIZER("implicit-signed-bitfield-truncation", ImplicitSignedBitfieldTruncation) +SANITIZER_GROUP("implicit-bitfield-truncation", ImplicitBitfieldTruncation, + ImplicitUnsignedBitfieldTruncation | + ImplicitSignedBitfieldTruncation) + +SANITIZER("implicit-bitfield-sign-change", ImplicitBitfieldSignChange) -// FIXME: -//SANITIZER_GROUP("implicit-integer-conversion", ImplicitIntegerConversion, -// ImplicitIntegerArithmeticValueChange | -// ImplicitUnsignedIntegerTruncation) -//SANITIZER_GROUP("implicit-conversion", ImplicitConversion, -// ImplicitIntegerConversion) +SANITIZER_GROUP("implicit-bitfield-arithmetic-value-change", + ImplicitBitfieldArithmeticValueChange, + ImplicitBitfieldSignChange | ImplicitSignedBitfieldTruncation) + +SANITIZER_GROUP("implicit-bitfield-conversion", ImplicitBitfieldConversion, + ImplicitBitfieldArithmeticValueChange | + ImplicitUnsignedBitfieldTruncation) SANITIZER_GROUP("implicit-conversion", ImplicitConversion, - ImplicitIntegerArithmeticValueChange | - ImplicitUnsignedIntegerTruncation) + ImplicitIntegerConversion | + ImplicitBitfieldConversion) SANITIZER_GROUP("integer", Integer, ImplicitConversion | IntegerDivideByZero | Shift | SignedIntegerOverflow | UnsignedIntegerOverflow | UnsignedShiftBase) +SANITIZER("objc-cast", ObjCCast) + SANITIZER("local-bounds", LocalBounds) SANITIZER_GROUP("bounds", Bounds, ArrayBounds | LocalBounds) diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp index 4a2f3caad6588..10f9c90dbbca0 100644 --- a/clang/lib/CodeGen/CGExpr.cpp +++ b/clang/lib/CodeGen/CGExpr.cpp @@ -5570,11 +5570,40 @@ LValue CodeGenFunction::EmitBinaryOperatorLValue(const BinaryOperator *E) { break; } - RValue RV = EmitAnyExpr(E->getRHS()); + llvm::Value *Previous = nullptr; + RValue RV; + QualType SrcType = E->getRHS()->getType(); + // If LHS refers to a bitfield we want to retrieve the value before + // implicit conversion between the bitfield type and the RHS type + // and evaluate RHS without integer sanitizer checks (if passed) + if (auto *ICE = RetrieveImplicitCastExprForBitfieldSanitizer(E)) { + SrcType = ICE->getSubExpr()->getType(); + Previous = EmitScalarExpr(ICE->getSubExpr()); + // Pass default ScalarConversionOpts so that sanitizer check is + // not emitted here + llvm::Value *RHS = EmitScalarConversion(Previous, SrcType, ICE->getType(), + ICE->getExprLoc()); + RV = RValue::get(RHS); + } + + if (!Previous) + RV = EmitAnyExpr(E->getRHS()); + LValue LV = EmitCheckedLValue(E->getLHS(), TCK_Store); + if (RV.isScalar()) EmitNullabilityCheck(LV, RV.getScalarVal(), E->getExprLoc()); - EmitStoreThroughLValue(RV, LV); + + if (LV.isBitField() && RV.isScalar()) { + llvm::Value *Src = Previous ? Previous : RV.getScalarVal(); + QualType DstType = E->getLHS()->getType(); + llvm::Value *Result; + EmitStoreThroughBitfieldLValue(RV, LV, &Result); + EmitBitfieldConversionCheck(Src, SrcType, Result, DstType, + LV.getBitFieldInfo(), E->getExprLoc()); + } else + EmitStoreThroughLValue(RV, LV); + if (getLangOpts().OpenMP) CGM.getOpenMPRuntime().checkAndEmitLastprivateConditional(*this, E->getLHS()); diff --git a/clang/lib/CodeGen/CGExprScalar.cpp b/clang/lib/CodeGen/CGExprScalar.cpp index b8caafb00847d..dc7fe623ce08f 100644 --- a/clang/lib/CodeGen/CGExprScalar.cpp +++ b/clang/lib/CodeGen/CGExprScalar.cpp @@ -15,6 +15,7 @@ #include "CGDebugInfo.h" #include "CGObjCRuntime.h" #include "CGOpenMPRuntime.h" +#include "CGRecordLayout.h" #include "CodeGenFunction.h" #include "CodeGenModule.h" #include "ConstantEmitter.h" @@ -304,6 +305,7 @@ class ScalarExprEmitter llvm::Type *DstTy, SourceLocation Loc); /// Known implicit conversion check kinds. + /// This is used for bitfield conversion checks as well /// Keep in sync with the enum of the same name in ubsan_handlers.h enum ImplicitConversionCheckKind : unsigned char { ICCK_IntegerTruncation = 0, // Legacy, was only used by clang 7. @@ -324,6 +326,19 @@ class ScalarExprEmitter void EmitIntegerSignChangeCheck(Value *Src, QualType SrcType, Value *Dst, QualType DstType, SourceLocation Loc); + /// Emit a check that an [implicit] truncation of a bitfield does not + /// discard any bits. It is not UB, so we use the value after truncation. + void EmitBitfieldTruncationCheck(Value *Src, QualType SrcType, Value *Dst, + QualType DstType, const CGBitFieldInfo &Info, + SourceLocation Loc); + + /// Emit a check that an [implicit] conversion of a bitfield does not change + /// the sign of the value. It is not UB, so we use the value after conversion. + /// NOTE: Src and Dst may be the exact same value! (point to the same thing) + void EmitBitfieldSignChangeCheck(Value *Src, QualType SrcType, Value *Dst, + QualType DstType, const CGBitFieldInfo &Info, + SourceLocation Loc); + /// Emit a conversion from the specified type to the specified destination /// type, both of which are LLVM scalar types. struct ScalarConversionOpts { @@ -1035,7 +1050,7 @@ EmitIntegerTruncationCheckHelper(Value *Src, QualType SrcType, Value *Dst, } llvm::Value *Check = nullptr; - // 1. Extend the truncated value back to the same width as the Src. + // 1. Convert the Dst back to the same type as Src. Check = Builder.CreateIntCast(Dst, SrcTy, DstSigned, "anyext"); // 2. Equality-compare with the original source value Check = Builder.CreateICmpEQ(Check, Src, "truncheck"); @@ -1116,7 +1131,7 @@ static llvm::Value *EmitIsNegativeTestHelper(Value *V, QualType VType, return Builder.CreateICmp(llvm::ICmpInst::ICMP_SLT, V, Zero, llvm::Twine(Name) + "." + V->getName() + ".negativitycheck"); -}; +} // Should be called within CodeGenFunction::SanitizerScope RAII scope. // Returns 'i1 false' when the conversion Src -> Dst changed the sign. @@ -1242,6 +1257,228 @@ void ScalarExprEmitter::EmitIntegerSignChangeCheck(Value *Src, QualType SrcType, {Src, Dst}); } +// Should be called within CodeGenFunction::SanitizerScope RAII scope. +// Returns 'i1 false' when the truncation Src -> Dst was lossy. +static std::pair<ScalarExprEmitter::ImplicitConversionCheckKind, + std::pair<llvm::Value *, SanitizerMask>> +EmitBitfieldTruncationCheckHelper(Value *Src, QualType SrcType, Value *Dst, + QualType DstType, CGBuilderTy &Builder) { + + llvm::Type *SrcTy = Src->getType(); + llvm::Type *DstTy = Dst->getType(); + (void)SrcTy; // Only used in assert() + (void)DstTy; // Only used in assert() + + // This should be truncation of integral types. + assert(isa<llvm::IntegerType>(SrcTy) && isa<llvm::IntegerType>(DstTy) && + "non-integer llvm type"); + + bool SrcSigned = SrcType->isSignedIntegerOrEnumerationType(); + bool DstSigned = DstType->isSignedIntegerOrEnumerationType(); + + ScalarExprEmitter::ImplicitConversionCheckKind Kind; + SanitizerMask Mask; + if (!SrcSigned && !DstSigned) { + Kind = ScalarExprEmitter::ICCK_UnsignedIntegerTruncation; + Mask = SanitizerKind::ImplicitUnsignedBitfieldTruncation; + } else { + Kind = ScalarExprEmitter::ICCK_SignedIntegerTruncation; + Mask = SanitizerKind::ImplicitSignedBitfieldTruncation; + } + + llvm::Value *Check = nullptr; + // 1. Extend the truncated value back to the same width as the Src. + Check = Builder.CreateIntCast(Dst, SrcTy, DstSigned, "bf.anyext"); + // 2. Equality-compare with the original source value + Check = Builder.CreateICmpEQ(Check, Src, "bf.truncheck"); + // If the comparison result is 'i1 false', then the truncation was lossy. + + return std::make_pair(Kind, std::make_pair(Check, Mask)); +} + +void ScalarExprEmitter::EmitBitfieldTruncationCheck( + Value *Src, QualType SrcType, Value *Dst, QualType DstType, + const CGBitFieldInfo &Info, SourceLocation Loc) { + + if (!CGF.SanOpts.hasOneOf(SanitizerKind::ImplicitBitfieldTruncation)) + return; + + // TODO: Calculate src width to avoid emitting code + // for unecessary cases. + unsigned SrcBits = ConvertType(SrcType)->getScalarSizeInBits(); + unsigned DstBits = Info.Size; + // This must be truncation. Else we do not care. + if (SrcBits <= DstBits) + return; + + // If the bitfield sign change sanitizer is enabled, + // and we are truncating from larger unsigned type to smaller signed type, + // let that next sanitizer deal with it. + bool SrcSigned = SrcType->isSignedIntegerOrEnumerationType(); + bool DstSigned = DstType->isSignedIntegerOrEnumerationType(); + if (CGF.SanOpts.has(SanitizerKind::ImplicitBitfieldSignChange) && + (!SrcSigned && DstSigned)) + return; + + CodeGenFunction::SanitizerScope SanScope(&CGF); + + std::pair<ScalarExprEmitter::ImplicitConversionCheckKind, + std::pair<llvm::Value *, SanitizerMask>> + Check = EmitBitfieldTruncationCheckHelper(Src, SrcType, Dst, DstType, + Builder); + // If the comparison result is 'i1 false', then the truncation was lossy. + + llvm::Constant *StaticArgs[] = { + CGF.EmitCheckSourceLocation(Loc), CGF.EmitCheckTypeDescriptor(SrcType), + CGF.EmitCheckTypeDescriptor(DstType), + llvm::ConstantInt::get(Builder.getInt8Ty(), Check.first), + llvm::ConstantInt::get(Builder.getInt32Ty(), Info.Size)}; + CGF.EmitCheck(Check.second, SanitizerHandler::ImplicitBitfieldConversion, + StaticArgs, {Src, Dst}); +} + +// Should be called within CodeGenFunction::SanitizerScope RAII scope. +// Returns 'i1 false' when the conversion Src -> Dst changed the sign. +static std::pair<ScalarExprEmitter::ImplicitConversionCheckKind, + std::pair<llvm::Value *, SanitizerMask>> +EmitBitfieldSignChangeCheckHelper(Value *Src, QualType SrcType, + unsigned SrcBits, Value *Dst, + QualType DstType, unsigned DstBits, + CGBuilderTy &Builder) { + llvm::Type *SrcTy = Src->getType(); + llvm::Type *DstTy = Dst->getType(); + (void)SrcTy; // Only used in assert() + (void)DstTy; // Only used in assert() + + // This should be truncation of integral types. + assert(isa<llvm::IntegerType>(SrcTy) && isa<llvm::IntegerType>(DstTy) && + "non-integer llvm type"); + + bool SrcSigned = SrcType->isSignedIntegerOrEnumerationType(); + bool DstSigned = DstType->isSignedIntegerOrEnumerationType(); + (void)SrcSigned; // Only used in assert() + (void)DstSigned; // Only used in assert() + + assert(((SrcBits != DstBits) || (SrcSigned != DstSigned)) && + "either the widths should be different, or the signednesses."); + + // 1. Was the old Value negative? + llvm::Value *SrcIsNegative = + EmitIsNegativeTestHelper(Src, SrcType, "bf.src", Builder); + // 2. Is the new Value negative? + llvm::Value *DstIsNegative = + EmitIsNegativeTestHelper(Dst, DstType, "bf.dst", Builder); + // 3. Now, was the 'negativity status' preserved during the conversion? + // NOTE: conversion from negative to zero is considered to change the sign. + // (We want to get 'false' when the conversion changed the sign) + // So we should just equality-compare the negativity statuses. + llvm::Value *Check = nullptr; + Check = + Builder.CreateICmpEQ(SrcIsNegative, DstIsNegative, "bf.signchangecheck"); + // If the comparison result is 'false', then the conversion changed the sign. + return std::make_pair( + ScalarExprEmitter::ICCK_IntegerSignChange, + std::make_pair(Check, SanitizerKind::ImplicitBitfieldSignChange)); +} + +void ScalarExprEmitter::EmitBitfieldSignChangeCheck( + Value *Src, QualType SrcType, Value *Dst, QualType DstType, + const CGBitFieldInfo &Info, SourceLocation Loc) { + + if (!CGF.SanOpts.has(SanitizerKind::ImplicitBitfieldSignChange)) + return; + + bool SrcSigned = SrcType->isSignedIntegerOrEnumerationType(); + bool DstSigned = DstType->isSignedIntegerOrEnumerationType(); + unsigned SrcBits = ConvertType(SrcType)->getScalarSizeInBits(); + unsigned DstBits = Info.Size; + + // Now, we do not need to emit the check in *all* of the cases. + // We can avoid emitting it in some obvious cases where it would have been + // dropped by the opt passes (instcombine) always anyways. + // If it's a cast between effectively the same type, no check. + // NOTE: this is *not* equivalent to checking the canonical types. + if (SrcSigned == DstSigned && SrcBits == DstBits) + return; + // At least one of the values needs to have signed type. + // If both are unsigned, then obviously, neither of them can be negative. + if (!SrcSigned && !DstSigned) + return; + // If the conversion is to *larger* *signed* type, then no check is needed. + // Because either sign-extension happens (so the sign will remain), + // or zero-extension will happen (the sign bit will be zero.) + if ((DstBits > SrcBits) && DstSigned) + return; + if (CGF.SanOpts.has(SanitizerKind::ImplicitSignedBitfieldTruncation) && + (SrcBits > DstBits) && SrcSigned) { + // If the signed integer truncation sanitizer is enabled, + // and this is a truncation from signed type, then no check is needed. + // Because here sign change check is interchangeable with truncation check. + return; + } + // That's it. We can't rule out any more cases with the data we have. + + CodeGenFunction::SanitizerScope SanScope(&CGF); + + std::pair<ScalarExprEmitter::ImplicitConversionCheckKind, + std::pair<llvm::Value *, SanitizerMask>> + Check; + + // Each of these checks needs to return 'false' when an issue was detected. + ImplicitConversionCheckKind CheckKind; + llvm::SmallVector<std::pair<llvm::Value *, SanitizerMask>, 2> Checks; + // So we can 'and' all the checks together, and still get 'false', + // if at least one of the checks detected an issue. + + Check = EmitBitfieldSignChangeCheckHelper(Src, SrcType, SrcBits, Dst, DstType, + DstBits, Builder); + CheckKind = Check.first; + Checks.emplace_back(Check.second); + + if (CGF.SanOpts.has(SanitizerKind::ImplicitSignedBitfieldTruncation) && + (SrcBits > DstBits) && !SrcSigned && DstSigned) { + // If the signed integer truncation sanitizer was enabled, + // and we are truncating from larger unsigned type to smaller signed type, + // let's handle the case we skipped in that check. + Check = + EmitBitfieldTruncationCheckHelper(Src, SrcType, Dst, DstType, Builder); + CheckKind = ICCK_SignedIntegerTruncationOrSignChange; + Checks.emplace_back(Check.second); + // If the comparison result is 'i1 false', then the truncation was lossy. + } + + llvm::Constant *StaticArgs[] = { + CGF.EmitCheckSourceLocation(Loc), CGF.EmitCheckTypeDescriptor(SrcType), + CGF.EmitCheckTypeDescriptor(DstType), + llvm::ConstantInt::get(Builder.getInt8Ty(), CheckKind), + llvm::ConstantInt::get(Builder.getInt32Ty(), Info.Size)}; + // EmitCheck() will 'and' all the checks together. + CGF.EmitCheck(Checks, SanitizerHandler::ImplicitBitfieldConversion, + StaticArgs, {Src, Dst}); +} + +void CodeGenFunction::EmitBitfieldConversionCheck(Value *Src, QualType SrcType, + Value *Dst, QualType DstType, + const CGBitFieldInfo &Info, + SourceLocation Loc) { + + if (!SanOpts.hasOneOf(SanitizerKind::ImplicitBitfieldConversion)) + return; + + // We only care about int->int conversions here. + // We ignore conversions to/from pointer and/or bool. + if (!PromotionIsPotentiallyEligibleForImplicitIntegerConversionCheck(SrcType, + DstType)) + return; + + if (DstType->isBooleanType() || SrcType->isBooleanType()) + return; + + ScalarExprEmitter Scalar(*this); + Scalar.EmitBitfieldTruncationCheck(Src, SrcType, Dst, DstType, Info, Loc); + Scalar.EmitBitfieldSignChangeCheck(Src, SrcType, Dst, DstType, Info, Loc); +} + Value *ScalarExprEmitter::EmitScalarCast(Value *Src, QualType SrcType, QualType DstType, llvm::Type *SrcTy, llvm::Type *DstTy, @@ -2561,6 +2798,8 @@ ScalarExprEmitter::EmitScalarPrePostIncDec(const UnaryOperator *E, LValue LV, llvm::PHINode *atomicPHI = nullptr; llvm::Value *value; llvm::Value *input; + llvm::Value *Previous = nullptr; + QualType SrcType = E->getType(); int amount = (isInc ? 1 : -1); bool isSubtraction = !isInc; @@ -2649,7 +2888,8 @@ ScalarExprEmitter::EmitScalarPrePostIncDec(const UnaryOperator *E, LValue LV, "base or promoted) will be signed, or the bitwidths will match."); } if (CGF.SanOpts.hasOneOf( - SanitizerKind::ImplicitIntegerArithmeticValueChange) && + SanitizerKind::ImplicitIntegerArithmeticValueChange | + SanitizerKind::ImplicitBitfieldArithmeticValueChange) && canPerformLossyDemotionCheck) { // While `x += 1` (for `x` with width less than int) is modeled as // promotion+arithmetics+demotion, and we can catch lossy demotion with @@ -2660,13 +2900,27 @@ ScalarExprEmitter::EmitScalarPrePostIncDec(const UnaryOperator *E, LValue LV, // the increment/decrement in the wider type, and finally // perform the demotion. This will catch lossy demotions. + // We have a special case for bitfields defined using all the bits of the + // type. In this case we need to do the same trick as for the integer + // sanitizer checks, i.e., promotion -> increment/decrement -> demotion + value = EmitScalarConversion(value, type, promotedType, E->getExprLoc()); Value *amt = llvm::ConstantInt::get(value->getType(), amount, true); value = Builder.CreateAdd(value, amt, isInc ? "inc" : "dec"); // Do pass non-default ScalarConversionOpts so that sanitizer check is - // emitted. + // emitted if LV is not a bitfield, otherwise the bitfield sanitizer + // checks will take care of the conversion. + ScalarConversionOpts Opts; + if (!LV.isBitField()) + Opts = ScalarConversionOpts(CGF.SanOpts); + else if (CGF.SanOpts.hasOneOf( + SanitizerKind::ImplicitBitfieldArithmeticValueChange)) { + Previous = value; + SrcType = promotedType; + } + value = EmitScalarConversion(value, promotedType, type, E->getExprLoc(), - ScalarConversionOpts(CGF.SanOpts)); + Opts); // Note that signed integer inc/dec with width less than int can't // overflow because of promotion rules; we're just eliding a few steps @@ -2851,9 +3105,12 @@ ScalarExprEmitter::EmitScalarPrePostIncDec(const UnaryOperator *E, LValue LV, } // Store the updated result through the lvalue. - if (LV.isBitField()) + if (LV.isBitField()) { + Value *Src = Previous ? Previous : value; CGF.EmitStoreThroughBitfieldLValue(RValue::get(value), LV, &value); - else + CGF.EmitBitfieldConversionCheck(Src, SrcType, value, E->getType(), + LV.getBitFieldInfo(), E->getExprLoc()); + } else CGF.EmitStoreThroughLValue(RValue::get(value), LV); // If this is a postinc, return the value read from memory, otherwise use the @@ -3358,8 +3615,15 @@ LValue ScalarExprEmitter::EmitCompoundAssignLValue( // Convert the result back to the LHS type, // potentially with Implicit Conversion sanitizer check. - Result = EmitScalarConversion(Result, PromotionTypeCR, LHSTy, Loc, - ScalarConversionOpts(CGF.SanOpts)); + // If LHSLV is a bitfield, use default ScalarConversionOpts + // to avoid emit any implicit integer checks + Value *Previous = nullptr; + if (LHSLV.isBitField()) { + Previous = Result; + Result = EmitScalarConversion(Result, PromotionTypeCR, LHSTy, Loc); + } else + Result = EmitScalarConversion(Result, PromotionTypeCR, LHSTy, Loc, + ScalarConversionOpts(CGF.SanOpts)); if (atomicPHI) { llvm::BasicBlock *curBlock = Builder.GetInsertBlock(); @@ -3378,9 +3642,14 @@ LValue ScalarExprEmitter::EmitCompoundAssignLValue( // specially because the result is altered by the store, i.e., [C99 6.5.16p1] // 'An assignment expression has the value of the left operand after the // assignment...'. - if (LHSLV.isBitField()) + if (LHSLV.isBitField()) { + Value *Src = Previous ? Previous : Result; + QualType SrcType = E->getRHS()->getType(); + QualType DstType = E->getLHS()->getType(); CGF.EmitStoreThroughBitfieldLValue(RValue::get(Result), LHSLV, &Result); - else + CGF.EmitBitfieldConversionCheck(Src, SrcType, Result, DstType, + LHSLV.getBitFieldInfo(), E->getExprLoc()); + } else CGF.EmitStoreThroughLValue(RValue::get(Result), LHSLV); if (CGF.getLangOpts().OpenMP) @@ -4488,6 +4757,24 @@ Value *ScalarExprEmitter::EmitCompare(const BinaryOperator *E, E->getExprLoc()); } +ImplicitCastExpr *CodeGenFunction::RetrieveImplicitCastExprForBitfieldSanitizer( + const BinaryOperator *E) { + if (E->getLHS()->refersToBitField() && + (SanOpts.hasOneOf(SanitizerKind::ImplicitConversion | + SanitizerKind::ImplicitBitfieldConversion))) { + // Check if there is an implicit conversion, and visit the sub expr + // like VisitCastExpr but without integer sanitizer checks, + // then emit scalar conversion with bitfield information + if (auto *ICE = dyn_cast<ImplicitCastExpr>(E->getRHS())) { + CastKind Kind = ICE->getCastKind(); + if (Kind == CK_IntegralCast) { + return ICE; + } + } + } + return nullptr; +} + Value *ScalarExprEmitter::VisitBinAssign(const BinaryOperator *E) { bool Ignore = TestAndClearIgnoreResultAssign(); @@ -4516,7 +4803,23 @@ Value *ScalarExprEmitter::VisitBinAssign(const BinaryOperator *E) { case Qualifiers::OCL_None: // __block variables need to have the rhs evaluated first, plus // this should improve codegen just a little. - RHS = Visit(E->getRHS()); + Value *Previous = nullptr; + QualType SrcType = E->getRHS()->getType(); + // If LHS refers to a bitfield we want to retrieve the value before + // implicit conversion between the bitfield type and the RHS type + // and evaluate RHS without integer sanitizer checks (if passed) + if (auto *ICE = CGF.RetrieveImplicitCastExprForBitfieldSanitizer(E)) { + SrcType = ICE->getSubExpr()->getType(); + Previous = Visit(ICE->getSubExpr()); + // Pass default ScalarConversionOpts so that sanitizer check is + // not emitted here + RHS = EmitScalarConversion(Previous, SrcType, ICE->getType(), + ICE->getExprLoc()); + } + + if (!Previous) + RHS = Visit(E->getRHS()); + LHS = EmitCheckedLValue(E->getLHS(), CodeGenFunction::TCK_Store); // Store the value into the LHS. Bit-fields are handled specially @@ -4524,7 +4827,12 @@ Value *ScalarExprEmitter::VisitBinAssign(const BinaryOperator *E) { // 'An assignment expression has the value of the left operand after // the assignment...'. if (LHS.isBitField()) { + // Use the value returned before conversion + Value *Src = Previous ? Previous : RHS; + QualType DstType = E->getLHS()->getType(); CGF.EmitStoreThroughBitfieldLValue(RValue::get(RHS), LHS, &RHS); + CGF.EmitBitfieldConversionCheck(Src, SrcType, RHS, DstType, + LHS.getBitFieldInfo(), E->getExprLoc()); } else { CGF.EmitNullabilityCheck(LHS, RHS, E->getExprLoc()); CGF.EmitStoreThroughLValue(RValue::get(RHS), LHS); diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h index 143ad64e8816b..9eec22815bb93 100644 --- a/clang/lib/CodeGen/CodeGenFunction.h +++ b/clang/lib/CodeGen/CodeGenFunction.h @@ -119,6 +119,7 @@ enum TypeEvaluationKind { SANITIZER_CHECK(FloatCastOverflow, float_cast_overflow, 0) \ SANITIZER_CHECK(FunctionTypeMismatch, function_type_mismatch, 0) \ SANITIZER_CHECK(ImplicitConversion, implicit_conversion, 0) \ + SANITIZER_CHECK(ImplicitBitfieldConversion, implicit_bitfield_conversion, 0) \ SANITIZER_CHECK(InvalidBuiltin, invalid_builtin, 0) \ SANITIZER_CHECK(InvalidObjCCast, invalid_objc_cast, 0) \ SANITIZER_CHECK(LoadInvalidValue, load_invalid_value, 0) \ @@ -2693,6 +2694,19 @@ class CodeGenFunction : public CodeGenTypeCache { /// expression and compare the result against zero, returning an Int1Ty value. llvm::Value *EvaluateExprAsBool(const Expr *E); + /// Retrieve the implicit cast expression of the rhs in a binary operator + /// expression This is used for implicit bitfield conversion checks, which + /// must compare with the value before truncation + ImplicitCastExpr * + RetrieveImplicitCastExprForBitfieldSanitizer(const BinaryOperator *E); + + /// Emit a check that an [implicit] conversion of a bitfield. It is not UB, + /// so we use the value after conversion. + void EmitBitfieldConversionCheck(llvm::Value *Src, QualType SrcType, + llvm::Value *Dst, QualType DstType, + const CGBitFieldInfo &Info, + SourceLocation Loc); + /// EmitIgnoredExpr - Emit an expression in a context which ignores the result. void EmitIgnoredExpr(const Expr *E); diff --git a/clang/test/CodeGen/ubsan-bitfield-conversion.c b/clang/test/CodeGen/ubsan-bitfield-conversion.c new file mode 100644 index 0000000000000..ab346370d24c2 --- /dev/null +++ b/clang/test/CodeGen/ubsan-bitfield-conversion.c @@ -0,0 +1,32 @@ +// RUN: %clang -fsanitize=implicit-bitfield-conversion -target x86_64-linux -S -emit-llvm -o - %s | FileCheck %s +// +typedef struct _xx { + int x1:3; + char x2:2; +} xx, *pxx; + +xx vxx; + +// CHECK-LABEL: define{{.*}} void @foo1 +void foo1(int x) { + vxx.x1 = x; + // CHECK: call void @__ubsan_handle_implicit_bitfield_conversion +} + +// CHECK-LABEL: define{{.*}} void @foo2 +void foo2(int x) { + vxx.x2 = x; + // CHECK: call void @__ubsan_handle_implicit_bitfield_conversion +} + +// CHECK-LABEL: define{{.*}} void @foo3 +void foo3() { + vxx.x1++; + // CHECK: call void @__ubsan_handle_implicit_bitfield_conversion +} + +// CHECK-LABEL: define{{.*}} void @foo4 +void foo4(int x) { + vxx.x1 += x; + // CHECK: call void @__ubsan_handle_implicit_bitfield_conversion +} diff --git a/compiler-rt/lib/ubsan/ubsan_handlers.cpp b/compiler-rt/lib/ubsan/ubsan_handlers.cpp index 0f16507d5d88f..2c0fa40009519 100644 --- a/compiler-rt/lib/ubsan/ubsan_handlers.cpp +++ b/compiler-rt/lib/ubsan/ubsan_handlers.cpp @@ -551,19 +551,15 @@ void __ubsan::__ubsan_handle_load_invalid_value_abort(InvalidValueData *Data, Die(); } -static void handleImplicitConversion(ImplicitConversionData *Data, - ReportOptions Opts, ValueHandle Src, - ValueHandle Dst) { - SourceLocation Loc = Data->Loc.acquire(); - ErrorType ET = ErrorType::GenericUB; - - const TypeDescriptor &SrcTy = Data->FromType; - const TypeDescriptor &DstTy = Data->ToType; +static ErrorType getImplicitConversionError(const TypeDescriptor &SrcTy, + const TypeDescriptor &DstTy, + unsigned char Kind) { + ErrorType ET = ErrorType::GenericUB; bool SrcSigned = SrcTy.isSignedIntegerTy(); bool DstSigned = DstTy.isSignedIntegerTy(); - switch (Data->Kind) { + switch (Kind) { case ICCK_IntegerTruncation: { // Legacy, no longer used. // Let's figure out what it should be as per the new types, and upgrade. // If both types are unsigned, then it's an unsigned truncation. @@ -588,6 +584,19 @@ static void handleImplicitConversion(ImplicitConversionData *Data, ET = ErrorType::ImplicitSignedIntegerTruncationOrSignChange; break; } + return ET; +} + +static void handleImplicitConversion(ImplicitConversionData *Data, + ReportOptions Opts, ValueHandle Src, + ValueHandle Dst) { + + SourceLocation Loc = Data->Loc.acquire(); + const TypeDescriptor &SrcTy = Data->FromType; + const TypeDescriptor &DstTy = Data->ToType; + bool SrcSigned = SrcTy.isSignedIntegerTy(); + bool DstSigned = DstTy.isSignedIntegerTy(); + ErrorType ET = getImplicitConversionError(SrcTy, DstTy, Data->Kind); if (ignoreReport(Loc, Opts, ET)) return; @@ -595,7 +604,6 @@ static void handleImplicitConversion(ImplicitConversionData *Data, ScopedReport R(Opts, Loc, ET); // FIXME: is it possible to dump the values as hex with fixed width? - Diag(Loc, DL_Error, ET, "implicit conversion from type %0 of value %1 (%2-bit, %3signed) to " "type %4 changed the value to %5 (%6-bit, %7signed)") @@ -604,6 +612,30 @@ static void handleImplicitConversion(ImplicitConversionData *Data, << DstTy.getIntegerBitWidth() << (DstSigned ? "" : "un"); } +static void +handleImplicitBitfieldConversion(ImplicitBitfieldConversionData *Data, + ReportOptions Opts, ValueHandle Src, + ValueHandle Dst) { + SourceLocation Loc = Data->Loc.acquire(); + const TypeDescriptor &SrcTy = Data->FromType; + const TypeDescriptor &DstTy = Data->ToType; + bool SrcSigned = SrcTy.isSignedIntegerTy(); + bool DstSigned = DstTy.isSignedIntegerTy(); + ErrorType ET = getImplicitConversionError(SrcTy, DstTy, Data->Kind); + + if (ignoreReport(Loc, Opts, ET)) + return; + + ScopedReport R(Opts, Loc, ET); + // FIXME: is it possible to dump the values as hex with fixed width? + Diag(Loc, DL_Error, ET, + "implicit conversion from type %0 of value %1 (%2-bit, %3signed) to " + "type %4 changed the value to %5 (%6-bit bitfield, %7signed)") + << SrcTy << Value(SrcTy, Src) << SrcTy.getIntegerBitWidth() + << (SrcSigned ? "" : "un") << DstTy << Value(DstTy, Dst) + << Data->BitfieldBits << (DstSigned ? "" : "un"); +} + void __ubsan::__ubsan_handle_implicit_conversion(ImplicitConversionData *Data, ValueHandle Src, ValueHandle Dst) { @@ -617,6 +649,18 @@ void __ubsan::__ubsan_handle_implicit_conversion_abort( Die(); } +void __ubsan::__ubsan_handle_implicit_bitfield_conversion( + ImplicitBitfieldConversionData *Data, ValueHandle Src, ValueHandle Dst) { + GET_REPORT_OPTIONS(false); + handleImplicitBitfieldConversion(Data, Opts, Src, Dst); +} +void __ubsan::__ubsan_handle_implicit_bitfield_conversion_abort( + ImplicitBitfieldConversionData *Data, ValueHandle Src, ValueHandle Dst) { + GET_REPORT_OPTIONS(true); + handleImplicitBitfieldConversion(Data, Opts, Src, Dst); + Die(); +} + static void handleInvalidBuiltin(InvalidBuiltinData *Data, ReportOptions Opts) { SourceLocation Loc = Data->Loc.acquire(); ErrorType ET = ErrorType::InvalidBuiltin; diff --git a/compiler-rt/lib/ubsan/ubsan_handlers.h b/compiler-rt/lib/ubsan/ubsan_handlers.h index 3bd5046de3d7d..c734e6cd262d2 100644 --- a/compiler-rt/lib/ubsan/ubsan_handlers.h +++ b/compiler-rt/lib/ubsan/ubsan_handlers.h @@ -153,6 +153,18 @@ struct ImplicitConversionData { RECOVERABLE(implicit_conversion, ImplicitConversionData *Data, ValueHandle Src, ValueHandle Dst) +struct ImplicitBitfieldConversionData { + SourceLocation Loc; + const TypeDescriptor &FromType; + const TypeDescriptor &ToType; + /* ImplicitConversionCheckKind */ unsigned char Kind; + unsigned int BitfieldBits; +}; + +/// \brief Implict bitfield conversion that changed the value. +RECOVERABLE(implicit_bitfield_conversion, ImplicitBitfieldConversionData *Data, + ValueHandle Src, ValueHandle Dst) + /// Known builtin check kinds. /// Keep in sync with the enum of the same name in CodeGenFunction.h enum BuiltinCheckKind : unsigned char { diff --git a/compiler-rt/lib/ubsan/ubsan_interface.inc b/compiler-rt/lib/ubsan/ubsan_interface.inc index cb27feb5d7e99..3078cef40236a 100644 --- a/compiler-rt/lib/ubsan/ubsan_interface.inc +++ b/compiler-rt/lib/ubsan/ubsan_interface.inc @@ -25,6 +25,8 @@ INTERFACE_FUNCTION(__ubsan_handle_function_type_mismatch) INTERFACE_FUNCTION(__ubsan_handle_function_type_mismatch_abort) INTERFACE_FUNCTION(__ubsan_handle_implicit_conversion) INTERFACE_FUNCTION(__ubsan_handle_implicit_conversion_abort) +INTERFACE_FUNCTION(__ubsan_handle_implicit_bitfield_conversion) +INTERFACE_FUNCTION(__ubsan_handle_implicit_bitfield_conversion_abort) INTERFACE_FUNCTION(__ubsan_handle_invalid_builtin) INTERFACE_FUNCTION(__ubsan_handle_invalid_builtin_abort) INTERFACE_FUNCTION(__ubsan_handle_invalid_objc_cast) _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits