Add an unsigned shift base sanitizer

[jfb]

Summary

It's not undefined behavior for an unsigned left shift to overflow (i.e. to
shift bits out), but it has been the source of bugs and exploits in certain
codebases in the past. As we do in other parts of UBSan, this patch adds a
dynamic checker which acts beyond UBSan and checks other sources of errors. The
option is enabled as part of -fsanitize=integer.

The flag is named: -fsanitize=unsigned-shift-base
This matches shift-base and shift-exponent flags.

rdar://problem/46129047

[vsk]

It'd be nice to fold the new check into an existing sanitizer group to bring this to a wider audience. Do you foresee adoption issues for existing -fsanitize=integer adopters? Fwiw some recently-added implicit conversion checks were folded in without much/any pushback.

clang/test/Driver/fsanitize.c

911

Not sure I follow this one. Why is 'NORECOVER' not expecting to see "-fno-sanitize-recover=unsigned-shift-base"?

[jfb]

In D86000#2219288, @vsk wrote:

It'd be nice to fold the new check into an existing sanitizer group to bring this to a wider audience. Do you foresee adoption issues for existing -fsanitize=integer adopters? Fwiw some recently-added implicit conversion checks were folded in without much/any pushback.

integer does "not actually UB checks", right? I can certainly put it in there if you think I won't get yelled at 😄

clang/test/Driver/fsanitize.c

911

I have no idea! Other parts of this file do this:

// CHECK-implicit-conversion-NORECOVER-NOT: "-fno-sanitize-recover={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation|implicit-integer-sign-change),?){3}"}} // ???
// CHECK-implicit-integer-arithmetic-value-change-NORECOVER-NOT: "-fno-sanitize-recover={{((implicit-signed-integer-truncation|implicit-integer-sign-change),?){2}"}} // ???
// CHECK-implicit-integer-truncation-NORECOVER-NOT: "-fno-sanitize-recover={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}} // ???

I was hoping someone who's touched this before would know.

compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp

2 ↗

(On Diff #285767)

I don't understand this test... when I run it with lit it fails (and it seems like the bots agree), but manually it works. Am I doing it wrong?

[vsk]

clang/test/Driver/fsanitize.c

911

The CHECK-implicit-conversion-NORECOVER-NOT regex looks fishy. There's more parens in there than I'd expect: perhaps that explains why the test passes?

Just above your change, I see something that's closer to what I expect:

// RUN: %clang -fsanitize=float-divide-by-zero %s -fno-sanitize-recover=float-divide-by-zero -### 2>&1 | FileCheck %s --check-prefixes=CHECK-DIVBYZERO,CHECK-DIVBYZERO-NORECOVER
...
// CHECK-DIVBYZERO-NORECOVER-NOT: "-fsanitize-recover=float-divide-by-zero"

compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp

2 ↗

(On Diff #285767)

Do you need to explicitly return 1 or something to get a non-zero exit code?

6 ↗

(On Diff #285767)

Does the test not work without the volatiles?

[vsk]

In D86000#2219322, @jfb wrote:

In D86000#2219288, @vsk wrote:

It'd be nice to fold the new check into an existing sanitizer group to bring this to a wider audience. Do you foresee adoption issues for existing -fsanitize=integer adopters? Fwiw some recently-added implicit conversion checks were folded in without much/any pushback.

integer does "not actually UB checks", right? I can certainly put it in there if you think I won't get yelled at 😄

Can't guarantee you won't get yelled at, but it does seem like the natural fit. It already includes other unsigned overflow checks.

[eugenis]

In D86000#2219322, @jfb wrote:

In D86000#2219288, @vsk wrote:

It'd be nice to fold the new check into an existing sanitizer group to bring this to a wider audience. Do you foresee adoption issues for existing -fsanitize=integer adopters? Fwiw some recently-added implicit conversion checks were folded in without much/any pushback.

integer does "not actually UB checks", right? I can certainly put it in there if you think I won't get yelled at 😄

I'd support this.
"integer" includes unsigned-integer-overflow, it's only recommended to the truly fearless developers :)

clang/test/Driver/fsanitize.c

911

I think that + the following line mean that the frontend depends on the default behavior (neither recover nor no-recover).

[lebedev.ri]

What's next, a sanitizer that input to signed right-shift is always non-negative? :)

FWIW "fixing" these "bugs" via (x & ~(~1U << (32-shamt))) << shamt is going to be fine,
i've already taught instcombine to drop such pointless masking before left-shift
in PR42563: https://godbolt.org/z/5rar14

[jfb]

As Vedant suggested, make it part of 'integer' sanitizer.

[jfb]

Re-upload with full context.

[lebedev.ri]

Release notes missing.
I think the canonical way to silence it (via masking?) should be at least mentioned.

clang/lib/CodeGen/CGExprScalar.cpp

3872–3884 ↗

(On Diff #288133)

Why is this so complicated? Shouldn't this just be: https://alive2.llvm.org/ce/z/scTqfX

$ /repositories/alive2/build-Clang-release/alive-tv /tmp/test.ll --smt-to=100000 --disable-undef-input

----------------------------------------
@2 = global 32 bytes, align 16

define i32 @src(i32 %arg, i32 %arg1) {
%bb:
  %i = icmp ugt i32 %arg1, 31
  %i2 = sub nsw nuw i32 31, %arg1    ; NOPE
  %i3 = lshr i32 %arg, %i2           ; NOPE
  %i4 = icmp ult i32 %i3, 2          ; NOPE
  %i5 = or i1 %i, %i4
  br i1 %i5, label %bb9, label %bb6

%bb6:
  %i7 = zext i32 %arg to i64
  %i8 = zext i32 %arg1 to i64
  %__constexpr_0 = bitcast * @2 to *
  call void @__ubsan_handle_shift_out_of_bounds(* %__constexpr_0, i64 %i7, i64 %i8)
  br label %bb9

%bb9:
  %i10 = shl i32 %arg, %arg1
  ret i32 %i10
}
=>
@2 = global 32 bytes, align 16

define i32 @tgt(i32 %arg, i32 %arg1) {
%bb:
  %i = icmp ugt i32 %arg1, 31
  %iZZ0 = shl i32 %arg, %arg1        ; HI!
  %iZZ1 = lshr i32 %iZZ0, %arg1      ; OVER HERE
  %i4 = icmp eq i32 %arg, %iZZ1      ; LOOK!
  %i5 = or i1 %i, %i4
  br i1 %i5, label %bb9, label %bb6

%bb6:
  %i7 = zext i32 %arg to i64
  %i8 = zext i32 %arg1 to i64
  %__constexpr_0 = bitcast * @2 to *
  call void @__ubsan_handle_shift_out_of_bounds(* %__constexpr_0, i64 %i7, i64 %i8)
  br label %bb9

%bb9:
  ret i32 %iZZ0
}
Transformation seems to be correct!

which will then be migrated to use @llvm.ushl.with.overflow once it's there.

[jfb]

Address comments

[jfb]

clang/lib/CodeGen/CGExprScalar.cpp

3872–3884 ↗

(On Diff #288133)

Sure, but that's pre-existing and I'd rather not change it in this patch.

compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp

2 ↗

(On Diff #285767)

That should be implicit, but I added it regardless to make sure. It's happy now 🤷‍♂️

6 ↗

(On Diff #285767)

It seems that LLVM sees too much without volatile, yes.

[lebedev.ri]

llvm/docs/ReleaseNotes.rst

153 ↗

(On Diff #288383)

Surely not ~1U.

[jfb]

Fix notes.

llvm/docs/ReleaseNotes.rst

153 ↗

(On Diff #288383)

Indeed, fixed.

[vsk]

compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp

6 ↗

(On Diff #285767)

The optimizer isn't running. Perhaps this is necessary because clang's constant folder kicks in?

llvm/docs/ReleaseNotes.rst

151 ↗

(On Diff #288383)

This could use some more explanation, maybe s/with masking/by clearing bits that would be shifted out/?

153 ↗

(On Diff #288383)

I don't think this pattern works when rhs = 0 (https://godbolt.org/z/rvEGqh).

[lebedev.ri]

llvm/docs/ReleaseNotes.rst

153 ↗

(On Diff #288383)

Surely not 1U either :)

[jfb]

Remove the "suppress this" in release notes.

compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp

6 ↗

(On Diff #285767)

Probably, I haven't investigate. It's brittle is the main thing, and this forces it to not be.

llvm/docs/ReleaseNotes.rst

151 ↗

(On Diff #288383)

I just dropped it, I don't think release notes are the right place for this anyways.

153 ↗

(On Diff #288383)

Right, I don't think it's something we want to explain here.

[vsk]

Lgtm, thanks.

clang/include/clang/Basic/Sanitizers.def

 //                ImplicitIntegerConversion)
 
 SANITIZER_GROUP("implicit-conversion", ImplicitConversion,
                 ImplicitIntegerArithmeticValueChange |
                     ImplicitUnsignedIntegerTruncation)
 
 SANITIZER_GROUP("integer", Integer,
                 ImplicitConversion | IntegerDivideByZero | Shift |
-                    SignedIntegerOverflow | UnsignedIntegerOverflow)
+                    SignedIntegerOverflow | UnsignedIntegerOverflow |
+                    UnsignedShiftBase)
 
 SANITIZER("local-bounds", LocalBounds)
 SANITIZER_GROUP("bounds", Bounds, ArrayBounds | LocalBounds)
 
 // Scudo hardened allocator
 SANITIZER("scudo", Scudo)
 
 // Magic group, containing all sanitizers. For example, "-fno-sanitize=all"
 // can be used to disable all the sanitizers.
 SANITIZER_GROUP("all", All, ~SanitizerMask())
 
 #undef SANITIZER
 #undef SANITIZER_GROUP

clang/lib/Driver/SanitizerArgs.cpp

 #include <memory>
 
 using namespace clang;
 using namespace clang::driver;
 using namespace llvm::opt;
 
 static const SanitizerMask NeedsUbsanRt =
     SanitizerKind::Undefined | SanitizerKind::Integer |
-    SanitizerKind::UnsignedShiftBase | SanitizerKind::ImplicitConversion |
-    SanitizerKind::Nullability | SanitizerKind::CFI |
-    SanitizerKind::FloatDivideByZero | SanitizerKind::ObjCCast;
+    SanitizerKind::ImplicitConversion | SanitizerKind::Nullability |
+    SanitizerKind::CFI | SanitizerKind::FloatDivideByZero |
+    SanitizerKind::ObjCCast;
 static const SanitizerMask NeedsUbsanCxxRt =
     SanitizerKind::Vptr | SanitizerKind::CFI;
 static const SanitizerMask NotAllowedWithTrap = SanitizerKind::Vptr;
 static const SanitizerMask NotAllowedWithMinimalRuntime =
     SanitizerKind::Function | SanitizerKind::Vptr;
 static const SanitizerMask RequiresPIE =
     SanitizerKind::DataFlow | SanitizerKind::HWAddress | SanitizerKind::Scudo;
 static const SanitizerMask NeedsUnwindTables =
     SanitizerKind::Address | SanitizerKind::HWAddress | SanitizerKind::Thread |
     SanitizerKind::Memory | SanitizerKind::DataFlow;
 static const SanitizerMask SupportsCoverage =
     SanitizerKind::Address | SanitizerKind::HWAddress |
     SanitizerKind::KernelAddress | SanitizerKind::KernelHWAddress |
     SanitizerKind::MemTag | SanitizerKind::Memory |
     SanitizerKind::KernelMemory | SanitizerKind::Leak |
-    SanitizerKind::Undefined | SanitizerKind::Integer |
-    SanitizerKind::UnsignedShiftBase | SanitizerKind::Bounds |
+    SanitizerKind::Undefined | SanitizerKind::Integer | SanitizerKind::Bounds |
     SanitizerKind::ImplicitConversion | SanitizerKind::Nullability |
     SanitizerKind::DataFlow | SanitizerKind::Fuzzer |
     SanitizerKind::FuzzerNoLink | SanitizerKind::FloatDivideByZero |
     SanitizerKind::SafeStack | SanitizerKind::ShadowCallStack |
     SanitizerKind::Thread | SanitizerKind::ObjCCast;
 static const SanitizerMask RecoverableByDefault =
     SanitizerKind::Undefined | SanitizerKind::Integer |
-    SanitizerKind::UnsignedShiftBase | SanitizerKind::ImplicitConversion |
-    SanitizerKind::Nullability | SanitizerKind::FloatDivideByZero |
-    SanitizerKind::ObjCCast;
+    SanitizerKind::ImplicitConversion | SanitizerKind::Nullability |
+    SanitizerKind::FloatDivideByZero | SanitizerKind::ObjCCast;
 static const SanitizerMask Unrecoverable =
     SanitizerKind::Unreachable | SanitizerKind::Return;
 static const SanitizerMask AlwaysRecoverable =
     SanitizerKind::KernelAddress | SanitizerKind::KernelHWAddress;
 static const SanitizerMask NeedsLTO = SanitizerKind::CFI;
 static const SanitizerMask TrappingSupported =
     (SanitizerKind::Undefined & ~SanitizerKind::Vptr) |
-    SanitizerKind::UnsignedIntegerOverflow | SanitizerKind::UnsignedShiftBase |
-    SanitizerKind::ImplicitConversion | SanitizerKind::Nullability |
-    SanitizerKind::LocalBounds | SanitizerKind::CFI |
-    SanitizerKind::FloatDivideByZero | SanitizerKind::ObjCCast;
+    SanitizerKind::UnsignedIntegerOverflow | SanitizerKind::ImplicitConversion |
+    SanitizerKind::Nullability | SanitizerKind::LocalBounds |
+    SanitizerKind::CFI | SanitizerKind::FloatDivideByZero |
+    SanitizerKind::ObjCCast;
 static const SanitizerMask TrappingDefault = SanitizerKind::CFI;
 static const SanitizerMask CFIClasses =
     SanitizerKind::CFIVCall | SanitizerKind::CFINVCall |
     SanitizerKind::CFIMFCall | SanitizerKind::CFIDerivedCast |
     SanitizerKind::CFIUnrelatedCast;
 static const SanitizerMask CompatibleWithMinimalRuntime =
     TrappingSupported | SanitizerKind::Scudo | SanitizerKind::ShadowCallStack |
     SanitizerKind::MemTag;
SanitizerMask Mask;
                     {"hwasan_blacklist.txt", SanitizerKind::HWAddress},
                     {"memtag_blacklist.txt", SanitizerKind::MemTag},
                     {"msan_blacklist.txt", SanitizerKind::Memory},
                     {"tsan_blacklist.txt", SanitizerKind::Thread},
                     {"dfsan_abilist.txt", SanitizerKind::DataFlow},
                     {"cfi_blacklist.txt", SanitizerKind::CFI},
                     {"ubsan_blacklist.txt",
                      SanitizerKind::Undefined | SanitizerKind::Integer |
-                         SanitizerKind::UnsignedShiftBase |
                          SanitizerKind::Nullability |
                          SanitizerKind::FloatDivideByZero}};
 
   for (auto BL : Blacklists) {
     if (!(Kinds & BL.Mask))
       continue;
 
     clang::SmallString<64> Path(D.ResourceDir);

clang/test/Driver/fsanitize.c

 // CHECK-UNDEFINED-WIN-SAME: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|builtin|returns-nonnull-attribute|nonnull-attribute),?){17}"}}
 
 // RUN: %clang -target i386-pc-win32 -fsanitize-coverage=bb %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-COVERAGE-WIN32
 // CHECK-COVERAGE-WIN32: "--dependent-lib={{[^"]*}}ubsan_standalone-i386.lib"
 // RUN: %clang -target x86_64-pc-win32 -fsanitize-coverage=bb %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-COVERAGE-WIN64
 // CHECK-COVERAGE-WIN64: "--dependent-lib={{[^"]*}}ubsan_standalone-x86_64.lib"
 
 // RUN: %clang -target %itanium_abi_triple -fsanitize=integer %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-INTEGER -implicit-check-not="-fsanitize-address-use-after-scope"
-// CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent|implicit-unsigned-integer-truncation|implicit-signed-integer-truncation|implicit-integer-sign-change),?){8}"}}
+// CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent|implicit-unsigned-integer-truncation|implicit-signed-integer-truncation|implicit-integer-sign-change|unsigned-shift-base),?){9}"}}
 
 // RUN: %clang -fsanitize=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-RECOVER
 // RUN: %clang -fsanitize=implicit-conversion -fsanitize-recover=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-RECOVER
 // RUN: %clang -fsanitize=implicit-conversion -fno-sanitize-recover=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-NORECOVER
 // RUN: %clang -fsanitize=implicit-conversion -fsanitize-trap=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-TRAP
 // CHECK-implicit-conversion: "-fsanitize={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation|implicit-integer-sign-change),?){3}"}}
 // CHECK-implicit-conversion-RECOVER: "-fsanitize-recover={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation|implicit-integer-sign-change),?){3}"}}
 // CHECK-implicit-conversion-RECOVER-NOT: "-fno-sanitize-recover={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation|implicit-integer-sign-change),?){3}"}}

 // CHECK-DIVBYZERO-RECOVER: "-fsanitize-recover=float-divide-by-zero"
 // CHECK-DIVBYZERO-NORECOVER-NOT: "-fsanitize-recover=float-divide-by-zero"
 // CHECK-DIVBYZERO-TRAP: "-fsanitize-trap=float-divide-by-zero"
 
 // RUN: %clang -fsanitize=float-divide-by-zero -fsanitize-minimal-runtime %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-DIVBYZERO,CHECK-DIVBYZERO-MINIMAL
 // CHECK-DIVBYZERO-MINIMAL: "-fsanitize-minimal-runtime"
 
 // RUN: %clang -fsanitize=undefined,float-divide-by-zero %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-DIVBYZERO-UBSAN
 // CHECK-DIVBYZERO-UBSAN: "-fsanitize={{.*}},float-divide-by-zero,{{.*}}"
-
-// RUN: %clang -fsanitize=unsigned-shift-base %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-unsigned-shift-base,CHECK-unsigned-shift-base-RECOVER
-// RUN: %clang -fsanitize=unsigned-shift-base -fsanitize-recover=unsigned-shift-base %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-unsigned-shift-base,CHECK-unsigned-shift-base-RECOVER
-// RUN: %clang -fsanitize=unsigned-shift-base -fno-sanitize-recover=unsigned-shift-base %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-unsigned-shift-base,CHECK-unsigned-shift-base-NORECOVER
-// RUN: %clang -fsanitize=unsigned-shift-base -fsanitize-trap=unsigned-shift-base %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-unsigned-shift-base,CHECK-unsigned-shift-base-TRAP
-// CHECK-unsigned-shift-base: "-fsanitize=unsigned-shift-base"
-// CHECK-unsigned-shift-base-RECOVER: "-fsanitize-recover=unsigned-shift-base"
-// CHECK-unsigned-shift-base-RECOVER-NOT: "-fno-sanitize-recover=unsigned-shift-base"
-// CHECK-unsigned-shift-base-RECOVER-NOT: "-fsanitize-trap=unsigned-shift-base"
-// CHECK-unsigned-shift-base-NORECOVER-NOT: "-fno-sanitize-recover=unsigned-shift-base"
-// CHECK-unsigned-shift-base-NORECOVER-NOT: "-fsanitize-recover=unsigned-shift-base"
-// CHECK-unsigned-shift-base-NORECOVER-NOT: "-fsanitize-trap=unsigned-shift-base"
-// CHECK-unsigned-shift-base-TRAP: "-fsanitize-trap=unsigned-shift-base"
-// CHECK-unsigned-shift-base-TRAP-NOT: "-fsanitize-recover=unsigned-shift-base"
-// CHECK-unsigned-shift-base-TRAP-NOT: "-fno-sanitize-recover=unsigned-shift-base"