[PATCH] D86000: Add an unsigned shift base sanitizer

2020-08-27 Thread JF Bastien via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rG82d29b397bb2: Add an unsigned shift base sanitizer (authored 
by jfb).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86000/new/

https://reviews.llvm.org/D86000

Files:
  clang/docs/UndefinedBehaviorSanitizer.rst
  clang/include/clang/Basic/Sanitizers.def
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/Driver/ToolChain.cpp
  clang/test/CodeGen/unsigned-shift-base.c
  clang/test/Driver/fsanitize.c
  compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp
  llvm/docs/ReleaseNotes.rst

Index: llvm/docs/ReleaseNotes.rst
===
--- llvm/docs/ReleaseNotes.rst
+++ llvm/docs/ReleaseNotes.rst
@@ -157,6 +157,14 @@
 Changes to LLDB
 -
 
+Changes to Sanitizers
+-
+
+The integer sanitizer `-fsanitize=integer` now has a new sanitizer:
+`-fsanitize=unsigned-shift-base`. 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.
+
 External Open Source Projects Using LLVM 12
 ===
 
Index: compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp
===
--- /dev/null
+++ compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp
@@ -0,0 +1,54 @@
+// RUN: %clangxx -fsanitize=unsigned-shift-base %s -o %t1 && not %run %t1 2>&1 | FileCheck %s
+// RUN: %clangxx -fsanitize=unsigned-shift-base,shift-exponent %s -o %t1 && not %run %t1 2>&1 | FileCheck %s
+
+#define shift(val, amount) ({  \
+  volatile unsigned _v = (val);\
+  volatile unsigned _a = (amount); \
+  unsigned res = _v << _a; \
+  res; \
+})
+
+int main() {
+
+  shift(0b''', 31);
+  shift(0b'''0001, 31);
+  shift(0b'''0010, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 2 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0100, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 4 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''1000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 8 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0001, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 16 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0010, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 32 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0100, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 64 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''1000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 128 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0001', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 256 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0010', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 512 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0100', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 1024 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''1000', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 2048 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0001', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 4096 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0010', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 8192 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0100', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 16384 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''1000', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 32768 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'0001'', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 65536 by 31 places cannot be

[PATCH] D86000: Add an unsigned shift base sanitizer

2020-08-27 Thread Vedant Kumar via Phabricator via cfe-commits 
vsk accepted this revision.
vsk added a comment.
This revision is now accepted and ready to land.

Lgtm, thanks.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86000/new/

https://reviews.llvm.org/D86000

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86000: Add an unsigned shift base sanitizer

2020-08-27 Thread JF Bastien via Phabricator via cfe-commits 
jfb added inline comments.



Comment at: compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp:6
+  volatile unsigned _v = (val);\
+  volatile unsigned _a = (amount); \
+  unsigned res = _v << _a; \

vsk wrote:
> jfb wrote:
> > vsk wrote:
> > > Does the test not work without the volatiles?
> > It seems that LLVM sees too much without volatile, yes.
> The optimizer isn't running. Perhaps this is necessary because clang's 
> constant folder kicks in?
Probably, I haven't investigate. It's brittle is the main thing, and this 
forces it to not be.



Comment at: llvm/docs/ReleaseNotes.rst:151
+
+One can silence the check with masking, for example:
+

vsk wrote:
> This could use some more explanation, maybe s/with masking/by clearing bits 
> that would be shifted out/?
I just dropped it, I don't think release notes are the right place for this 
anyways.



Comment at: llvm/docs/ReleaseNotes.rst:153
+
+  `(lhs & ~(~1U << ((sizeof(lhs)*CHAR_BIT) - rhs))) << rhs`
+

lebedev.ri wrote:
> vsk wrote:
> > jfb wrote:
> > > lebedev.ri wrote:
> > > > Surely not `~1U`.
> > > Indeed, fixed.
> > I don't think this pattern works when rhs = 0 
> > (https://godbolt.org/z/rvEGqh).
> Surely not `1U` either :)
Right, I don't think it's something we want to explain here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86000/new/

https://reviews.llvm.org/D86000

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86000: Add an unsigned shift base sanitizer

2020-08-27 Thread JF Bastien via Phabricator via cfe-commits 
jfb updated this revision to Diff 288444.
jfb marked 4 inline comments as done.
jfb added a comment.

Remove the "suppress this" in release notes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86000/new/

https://reviews.llvm.org/D86000

Files:
  clang/docs/UndefinedBehaviorSanitizer.rst
  clang/include/clang/Basic/Sanitizers.def
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/Driver/ToolChain.cpp
  clang/test/CodeGen/unsigned-shift-base.c
  clang/test/Driver/fsanitize.c
  compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp
  llvm/docs/ReleaseNotes.rst

Index: llvm/docs/ReleaseNotes.rst
===
--- llvm/docs/ReleaseNotes.rst
+++ llvm/docs/ReleaseNotes.rst
@@ -140,6 +140,14 @@
 Changes to LLDB
 -
 
+Changes to Sanitizers
+-
+
+The integer sanitizer `-fsanitize=integer` now has a new sanitizer:
+`-fsanitize=unsigned-shift-base`. 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.
+
 External Open Source Projects Using LLVM 12
 ===
 
Index: compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp
===
--- /dev/null
+++ compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp
@@ -0,0 +1,54 @@
+// RUN: %clangxx -fsanitize=unsigned-shift-base %s -o %t1 && not %run %t1 2>&1 | FileCheck %s
+// RUN: %clangxx -fsanitize=unsigned-shift-base,shift-exponent %s -o %t1 && not %run %t1 2>&1 | FileCheck %s
+
+#define shift(val, amount) ({  \
+  volatile unsigned _v = (val);\
+  volatile unsigned _a = (amount); \
+  unsigned res = _v << _a; \
+  res; \
+})
+
+int main() {
+
+  shift(0b''', 31);
+  shift(0b'''0001, 31);
+  shift(0b'''0010, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 2 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0100, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 4 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''1000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 8 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0001, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 16 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0010, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 32 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0100, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 64 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''1000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 128 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0001', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 256 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0010', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 512 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0100', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 1024 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''1000', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 2048 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0001', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 4096 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0010', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 8192 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0100', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 16384 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''1000', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 32768 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'0001'', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 65536 by 31 places cannot be represented in type

[PATCH] D86000: Add an unsigned shift base sanitizer

2020-08-27 Thread Roman Lebedev via Phabricator via cfe-commits 
lebedev.ri added inline comments.



Comment at: llvm/docs/ReleaseNotes.rst:153
+
+  `(lhs & ~(~1U << ((sizeof(lhs)*CHAR_BIT) - rhs))) << rhs`
+

vsk wrote:
> jfb wrote:
> > lebedev.ri wrote:
> > > Surely not `~1U`.
> > Indeed, fixed.
> I don't think this pattern works when rhs = 0 (https://godbolt.org/z/rvEGqh).
Surely not `1U` either :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86000/new/

https://reviews.llvm.org/D86000

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86000: Add an unsigned shift base sanitizer

2020-08-27 Thread Vedant Kumar via Phabricator via cfe-commits 
vsk added inline comments.



Comment at: compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp:6
+  volatile unsigned _v = (val);\
+  volatile unsigned _a = (amount); \
+  unsigned res = _v << _a; \

jfb wrote:
> vsk wrote:
> > Does the test not work without the volatiles?
> It seems that LLVM sees too much without volatile, yes.
The optimizer isn't running. Perhaps this is necessary because clang's constant 
folder kicks in?



Comment at: llvm/docs/ReleaseNotes.rst:151
+
+One can silence the check with masking, for example:
+

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



Comment at: llvm/docs/ReleaseNotes.rst:153
+
+  `(lhs & ~(~1U << ((sizeof(lhs)*CHAR_BIT) - rhs))) << rhs`
+

jfb wrote:
> lebedev.ri wrote:
> > Surely not `~1U`.
> Indeed, fixed.
I don't think this pattern works when rhs = 0 (https://godbolt.org/z/rvEGqh).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86000/new/

https://reviews.llvm.org/D86000

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86000: Add an unsigned shift base sanitizer

2020-08-27 Thread JF Bastien via Phabricator via cfe-commits 
jfb added inline comments.



Comment at: llvm/docs/ReleaseNotes.rst:153
+
+  `(lhs & ~(~1U << ((sizeof(lhs)*CHAR_BIT) - rhs))) << rhs`
+

lebedev.ri wrote:
> Surely not `~1U`.
Indeed, fixed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86000/new/

https://reviews.llvm.org/D86000

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86000: Add an unsigned shift base sanitizer

2020-08-27 Thread JF Bastien via Phabricator via cfe-commits 
jfb updated this revision to Diff 288388.
jfb marked an inline comment as done.
jfb added a comment.

Fix notes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86000/new/

https://reviews.llvm.org/D86000

Files:
  clang/docs/UndefinedBehaviorSanitizer.rst
  clang/include/clang/Basic/Sanitizers.def
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/Driver/ToolChain.cpp
  clang/test/CodeGen/unsigned-shift-base.c
  clang/test/Driver/fsanitize.c
  compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp
  llvm/docs/ReleaseNotes.rst

Index: llvm/docs/ReleaseNotes.rst
===
--- llvm/docs/ReleaseNotes.rst
+++ llvm/docs/ReleaseNotes.rst
@@ -140,6 +140,18 @@
 Changes to LLDB
 -
 
+Changes to Sanitizers
+-
+
+The integer sanitizer `-fsanitize=integer` now has a new sanitizer:
+`-fsanitize=unsigned-shift-base`. 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.
+
+One can silence the check with masking, for example:
+
+  `(lhs & ~(1U << ((sizeof(lhs)*CHAR_BIT) - rhs))) << rhs`
+
 External Open Source Projects Using LLVM 12
 ===
 
Index: compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp
===
--- /dev/null
+++ compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp
@@ -0,0 +1,54 @@
+// RUN: %clangxx -fsanitize=unsigned-shift-base %s -o %t1 && not %run %t1 2>&1 | FileCheck %s
+// RUN: %clangxx -fsanitize=unsigned-shift-base,shift-exponent %s -o %t1 && not %run %t1 2>&1 | FileCheck %s
+
+#define shift(val, amount) ({  \
+  volatile unsigned _v = (val);\
+  volatile unsigned _a = (amount); \
+  unsigned res = _v << _a; \
+  res; \
+})
+
+int main() {
+
+  shift(0b''', 31);
+  shift(0b'''0001, 31);
+  shift(0b'''0010, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 2 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0100, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 4 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''1000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 8 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0001, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 16 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0010, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 32 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0100, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 64 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''1000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 128 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0001', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 256 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0010', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 512 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0100', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 1024 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''1000', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 2048 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0001', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 4096 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0010', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 8192 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0100', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 16384 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''1000', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 32768 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'0001'', 31); // CHECK:

[PATCH] D86000: Add an unsigned shift base sanitizer

2020-08-27 Thread Roman Lebedev via Phabricator via cfe-commits 
lebedev.ri added inline comments.



Comment at: llvm/docs/ReleaseNotes.rst:153
+
+  `(lhs & ~(~1U << ((sizeof(lhs)*CHAR_BIT) - rhs))) << rhs`
+

Surely not `~1U`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86000/new/

https://reviews.llvm.org/D86000

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86000: Add an unsigned shift base sanitizer

2020-08-27 Thread JF Bastien via Phabricator via cfe-commits 
jfb added inline comments.



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3872-3884
   llvm::Value *BitsShiftedOff = Builder.CreateLShr(
   Ops.LHS, Builder.CreateSub(PromotedWidthMinusOne, RHS, "shl.zeros",
  /*NUW*/ true, /*NSW*/ true),
   "shl.check");
-  if (CGF.getLangOpts().CPlusPlus) {
+  if (SanitizeUnsignedBase || CGF.getLangOpts().CPlusPlus) {
 // In C99, we are not permitted to shift a 1 bit into the sign bit.
 // Under C++11's rules, shifting a 1 bit into the sign bit is

lebedev.ri wrote:
> 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=10 --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.
Sure, but that's pre-existing and I'd rather not change it in this patch.



Comment at: compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp:2
+// RUN: %clangxx -fsanitize=unsigned-shift-base %s -o %t1 && not %run %t1 2>&1 
| FileCheck %s
+// RUN: %clangxx -fsanitize=unsigned-shift-base,shift-exponent %s -o %t1 && 
not %run %t1 2>&1 | FileCheck %s
+

vsk wrote:
> jfb wrote:
> > 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?
> Do you need to explicitly `return 1` or something to get a non-zero exit code?
That should be implicit, but I added it regardless to make sure. It's happy now 
路‍♂️



Comment at: compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp:6
+  volatile unsigned _v = (val);\
+  volatile unsigned _a = (amount); \
+  unsigned res = _v << _a; \

vsk wrote:
> Does the test not work without the volatiles?
It seems that LLVM sees too much without volatile, yes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86000/new/

https://reviews.llvm.org/D86000

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86000: Add an unsigned shift base sanitizer

2020-08-27 Thread JF Bastien via Phabricator via cfe-commits 
jfb updated this revision to Diff 288383.
jfb marked 6 inline comments as done.
jfb added a comment.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Address comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86000/new/

https://reviews.llvm.org/D86000

Files:
  clang/docs/UndefinedBehaviorSanitizer.rst
  clang/include/clang/Basic/Sanitizers.def
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/Driver/ToolChain.cpp
  clang/test/CodeGen/unsigned-shift-base.c
  clang/test/Driver/fsanitize.c
  compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp
  llvm/docs/ReleaseNotes.rst

Index: llvm/docs/ReleaseNotes.rst
===
--- llvm/docs/ReleaseNotes.rst
+++ llvm/docs/ReleaseNotes.rst
@@ -140,6 +140,18 @@
 Changes to LLDB
 -
 
+Changes to Sanitizers
+-
+
+The integer sanitizer `-fsanitize=integer` now has a new sanitizer:
+`-fsanitize=unsigned-shift-base`. 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.
+
+One can silence the check with masking, for example:
+
+  `(lhs & ~(~1U << ((sizeof(lhs)*CHAR_BIT) - rhs))) << rhs`
+
 External Open Source Projects Using LLVM 12
 ===
 
Index: compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp
===
--- /dev/null
+++ compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp
@@ -0,0 +1,54 @@
+// RUN: %clangxx -fsanitize=unsigned-shift-base %s -o %t1 && not %run %t1 2>&1 | FileCheck %s
+// RUN: %clangxx -fsanitize=unsigned-shift-base,shift-exponent %s -o %t1 && not %run %t1 2>&1 | FileCheck %s
+
+#define shift(val, amount) ({  \
+  volatile unsigned _v = (val);\
+  volatile unsigned _a = (amount); \
+  unsigned res = _v << _a; \
+  res; \
+})
+
+int main() {
+
+  shift(0b''', 31);
+  shift(0b'''0001, 31);
+  shift(0b'''0010, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 2 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0100, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 4 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''1000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 8 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0001, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 16 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0010, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 32 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0100, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 64 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''1000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 128 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0001', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 256 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0010', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 512 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0100', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 1024 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''1000', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 2048 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0001', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 4096 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0010', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 8192 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0100', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 16384 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''1000', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 32768 by 31 places cannot be represented in type 'unsigned int'
+

[PATCH] D86000: Add an unsigned shift base sanitizer

2020-08-27 Thread Roman Lebedev via Phabricator via cfe-commits 
lebedev.ri added a comment.

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




Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3872-3884
   llvm::Value *BitsShiftedOff = Builder.CreateLShr(
   Ops.LHS, Builder.CreateSub(PromotedWidthMinusOne, RHS, "shl.zeros",
  /*NUW*/ true, /*NSW*/ true),
   "shl.check");
-  if (CGF.getLangOpts().CPlusPlus) {
+  if (SanitizeUnsignedBase || CGF.getLangOpts().CPlusPlus) {
 // In C99, we are not permitted to shift a 1 bit into the sign bit.
 // Under C++11's rules, shifting a 1 bit into the sign bit is

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=10 --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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86000/new/

https://reviews.llvm.org/D86000

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86000: Add an unsigned shift base sanitizer

2020-08-26 Thread JF Bastien via Phabricator via cfe-commits 
jfb updated this revision to Diff 288133.
jfb added a comment.

Re-upload with full context.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86000/new/

https://reviews.llvm.org/D86000

Files:
  clang/docs/UndefinedBehaviorSanitizer.rst
  clang/include/clang/Basic/Sanitizers.def
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/Driver/ToolChain.cpp
  clang/test/CodeGen/unsigned-shift-base.c
  clang/test/Driver/fsanitize.c
  compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp

Index: compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp
===
--- /dev/null
+++ compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp
@@ -0,0 +1,52 @@
+// RUN: %clangxx -fsanitize=unsigned-shift-base %s -o %t1 && not %run %t1 2>&1 | FileCheck %s
+// RUN: %clangxx -fsanitize=unsigned-shift-base,shift-exponent %s -o %t1 && not %run %t1 2>&1 | FileCheck %s
+
+#define shift(val, amount) ({  \
+  volatile unsigned _v = (val);\
+  volatile unsigned _a = (amount); \
+  unsigned res = _v << _a; \
+  res; \
+})
+
+int main() {
+
+  shift(0b''', 31);
+  shift(0b'''0001, 31);
+  shift(0b'''0010, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 2 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0100, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 4 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''1000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 8 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0001, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 16 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0010, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 32 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0100, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 64 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''1000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 128 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0001', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 256 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0010', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 512 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0100', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 1024 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''1000', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 2048 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0001', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 4096 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0010', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 8192 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0100', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 16384 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''1000', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 32768 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'0001'', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 65536 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'0010'', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 131072 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'0100'', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 262144 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'1000'', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 524288 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'0001'', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 1048576 by 31 places cannot be

[PATCH] D86000: Add an unsigned shift base sanitizer

2020-08-26 Thread JF Bastien via Phabricator via cfe-commits 
jfb updated this revision to Diff 288132.
jfb added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86000/new/

https://reviews.llvm.org/D86000

Files:
  clang/include/clang/Basic/Sanitizers.def
  clang/lib/Driver/SanitizerArgs.cpp
  clang/test/Driver/fsanitize.c

Index: clang/test/Driver/fsanitize.c
===
--- clang/test/Driver/fsanitize.c
+++ clang/test/Driver/fsanitize.c
@@ -32,7 +32,7 @@
 // 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
@@ -899,18 +899,3 @@
 
 // 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"
Index: clang/lib/Driver/SanitizerArgs.cpp
===
--- clang/lib/Driver/SanitizerArgs.cpp
+++ clang/lib/Driver/SanitizerArgs.cpp
@@ -26,9 +26,9 @@
 
 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;
@@ -44,8 +44,7 @@
 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 |
@@ -53,9 +52,8 @@
 SanitizerKind::Thread | SanitizerKind::ObjCCast;
 static const SanitizerMask RecoverableByDefault =

[PATCH] D86000: Add an unsigned shift base sanitizer

2020-08-15 Thread Roman Lebedev via Phabricator via cfe-commits 
lebedev.ri added a comment.

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/1h33hP


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86000/new/

https://reviews.llvm.org/D86000

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86000: Add an unsigned shift base sanitizer

2020-08-14 Thread Evgenii Stepanov via Phabricator via cfe-commits 
eugenis added a comment.

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 :)




Comment at: clang/test/Driver/fsanitize.c:911
+// 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"

vsk wrote:
> jfb wrote:
> > vsk wrote:
> > > Not sure I follow this one. Why is 'NORECOVER' not expecting to see 
> > > "-fno-sanitize-recover=unsigned-shift-base"?
> > 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.
> 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"
> ```
I think that + the following line mean that the frontend depends on the default 
behavior (neither recover nor no-recover).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86000/new/

https://reviews.llvm.org/D86000

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86000: Add an unsigned shift base sanitizer

2020-08-14 Thread Vedant Kumar via Phabricator via cfe-commits 
vsk added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86000/new/

https://reviews.llvm.org/D86000

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86000: Add an unsigned shift base sanitizer

2020-08-14 Thread Vedant Kumar via Phabricator via cfe-commits 
vsk added inline comments.



Comment at: clang/test/Driver/fsanitize.c:911
+// 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"

jfb wrote:
> vsk wrote:
> > Not sure I follow this one. Why is 'NORECOVER' not expecting to see 
> > "-fno-sanitize-recover=unsigned-shift-base"?
> 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.
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"
```



Comment at: compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp:2
+// RUN: %clangxx -fsanitize=unsigned-shift-base %s -o %t1 && not %run %t1 2>&1 
| FileCheck %s
+// RUN: %clangxx -fsanitize=unsigned-shift-base,shift-exponent %s -o %t1 && 
not %run %t1 2>&1 | FileCheck %s
+

jfb wrote:
> 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?
Do you need to explicitly `return 1` or something to get a non-zero exit code?



Comment at: compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp:6
+  volatile unsigned _v = (val);\
+  volatile unsigned _a = (amount); \
+  unsigned res = _v << _a; \

Does the test not work without the volatiles?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86000/new/

https://reviews.llvm.org/D86000

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86000: Add an unsigned shift base sanitizer

2020-08-14 Thread JF Bastien via Phabricator via cfe-commits 
jfb added a comment.

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 




Comment at: clang/test/Driver/fsanitize.c:911
+// 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"

vsk wrote:
> Not sure I follow this one. Why is 'NORECOVER' not expecting to see 
> "-fno-sanitize-recover=unsigned-shift-base"?
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.



Comment at: compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp:2
+// RUN: %clangxx -fsanitize=unsigned-shift-base %s -o %t1 && not %run %t1 2>&1 
| FileCheck %s
+// RUN: %clangxx -fsanitize=unsigned-shift-base,shift-exponent %s -o %t1 && 
not %run %t1 2>&1 | FileCheck %s
+

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86000/new/

https://reviews.llvm.org/D86000

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86000: Add an unsigned shift base sanitizer

2020-08-14 Thread Vedant Kumar via Phabricator via cfe-commits 
vsk added a comment.

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.




Comment at: clang/test/Driver/fsanitize.c:911
+// 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"

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86000/new/

https://reviews.llvm.org/D86000

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86000: Add an unsigned shift base sanitizer

2020-08-14 Thread JF Bastien via Phabricator via cfe-commits 
jfb created this revision.
jfb added a reviewer: vsk.
Herald added subscribers: Sanitizers, cfe-commits, ributzka, dexonsmith, 
jkorous.
Herald added projects: clang, Sanitizers.
jfb requested review of this revision.

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 completely separately from other checkers since it's unlikely
that folks who have currently adopted other checkers will want this one.

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

rdar://problem/46129047


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D86000

Files:
  clang/docs/UndefinedBehaviorSanitizer.rst
  clang/include/clang/Basic/Sanitizers.def
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/Driver/SanitizerArgs.cpp
  clang/lib/Driver/ToolChain.cpp
  clang/test/CodeGen/unsigned-shift-base.c
  clang/test/Driver/fsanitize.c
  compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp

Index: compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp
===
--- /dev/null
+++ compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp
@@ -0,0 +1,52 @@
+// RUN: %clangxx -fsanitize=unsigned-shift-base %s -o %t1 && not %run %t1 2>&1 | FileCheck %s
+// RUN: %clangxx -fsanitize=unsigned-shift-base,shift-exponent %s -o %t1 && not %run %t1 2>&1 | FileCheck %s
+
+#define shift(val, amount) ({  \
+  volatile unsigned _v = (val);\
+  volatile unsigned _a = (amount); \
+  unsigned res = _v << _a; \
+  res; \
+})
+
+int main() {
+
+  shift(0b''', 31);
+  shift(0b'''0001, 31);
+  shift(0b'''0010, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 2 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0100, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 4 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''1000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 8 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0001, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 16 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0010, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 32 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''0100, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 64 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'''1000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 128 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0001', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 256 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0010', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 512 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0100', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 1024 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''1000', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 2048 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0001', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 4096 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0010', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 8192 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''0100', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 16384 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b''1000', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 32768 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'0001'', 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 65536 by 31 places cannot be represented in type 'unsigned int'
+  shift(0b'0010'',