google / sanitizers

AddressSanitizer, ThreadSanitizer, MemorySanitizer
Other
11.56k stars 1.04k forks source link

UBSAN does not catch overflow of short, chart #940

Open milianw opened 6 years ago

milianw commented 6 years ago

Overflows for short, char and signed char are not caught by UBSAN. It does work for long long, long and int though:

┌milian:/tmp
└$ cat test.c
#include <limits.h>

int main()
{
  long long ll = LLONG_MAX;
  ++ll;

  long l = LONG_MAX;
  ++l;

  int i = INT_MAX;
  ++i;

  short s = SHRT_MAX;
  ++s;

  char c = CHAR_MAX;
  ++c;

  signed char sc = SCHAR_MAX;
  ++sc;

  return 0;
}
┌milian:/tmp
└$ clang -fsanitize=undefined -g test.c && ./a.out 
test.c:6:3: runtime error: signed integer overflow: 9223372036854775807 + 1 cannot be represented in type 'long long'
    #0 0x55ca3af09f67 in main /tmp/test.c:6:3
    #1 0x7f6ad1feaf49 in __libc_start_main (/usr/lib/libc.so.6+0x20f49)
    #2 0x55ca3aee4249 in _start (/tmp/a.out+0x3249)

test.c:9:3: runtime error: signed integer overflow: 9223372036854775807 + 1 cannot be represented in type 'long'
    #0 0x55ca3af09fbb in main /tmp/test.c:9:3
    #1 0x7f6ad1feaf49 in __libc_start_main (/usr/lib/libc.so.6+0x20f49)
    #2 0x55ca3aee4249 in _start (/tmp/a.out+0x3249)

test.c:12:3: runtime error: signed integer overflow: 2147483647 + 1 cannot be represented in type 'int'
    #0 0x55ca3af0a004 in main /tmp/test.c:12:3
    #1 0x7f6ad1feaf49 in __libc_start_main (/usr/lib/libc.so.6+0x20f49)
    #2 0x55ca3aee4249 in _start (/tmp/a.out+0x3249)
dvyukov commented 6 years ago

Isn't it defined in C/C++ due to promotion rules? AFAIK if you add two shorts and assign the result to short, arguments are first promoted to int's, int's are added and then the result is truncated back to short.

milianw commented 6 years ago

ugh TIL... Considering that this is often not what the developer intended, can we maybe get a flag for this similar to -fsanitize=unsigned-integer-overflow to catch these?

yugr commented 6 years ago

I once filed LLVM bug #21530 about this and feedback was positive but it never got beyond prototype. Maybe try asking in Bugzilla?

LebedevRI commented 6 years ago

(Ah, i though i'we seen this issue here, but failed to find it the first time.) As i have written in LLVM bug #21530, i'm looking into this, and have a functional, working patch. By itself, it's quite simple. But there is at least one known prerequisite blocker that needs to be resolved on the clang's side first.

LebedevRI commented 6 years ago

https://reviews.llvm.org/D48958

LebedevRI commented 6 years ago

https://reviews.llvm.org/D50250

LebedevRI commented 6 years ago

And the second half - sign change - is finally here too. For integer scalars, i claim that the sanitizer is feature-complete. Only bitfield handling is missing.

LebedevRI commented 6 years ago

Rough overview of not-yet-sanitized constructs, mostly for myself:

LebedevRI commented 6 years ago

Compound assignment operators - done (yay) https://github.com/llvm-mirror/clang/commit/d8a18b8bb0023a295456958e267e84ba1d1f3d1f

LebedevRI commented 5 years ago

inc/dec: check. For plain integers i think only bitfields are left.

caarecengi commented 4 years ago

Dummy comment to subscribe to this issue

LebedevRI commented 4 years ago

You can just click on "Subscribe" button :)