[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

There's definitely a lot of new findings this creates, but it's hard to say exactly how many root causes there are due to the way test failures are (not) grouped well in the way I'm testing. So far they all seem like true positives, so this would be good to submit. However a few are positive yet benign, like this interesting one (simplified):

  void ParseString(char *s) {
    char *next = s;
    for (char *end = s; end; next = end + 1) { // ubsan error computing (nil + 1), although it doesn't matter because the loop terminates when end == nil and next is not read after the loop
      // ...
      end = strchr(next, 'x'); // returns null if not found
      // ...
    }
  }

If I had to guesstimate, I'd say 20-100 bugs in a couple billion lines of code, so a lot, but shouldn't be too disruptive to anyone that has these checks enabled globally.

I haven't noticed any timeouts -- which is not to say this isn't a slowdown, but at least it's not egregious.

BTW, here's a minimal + complete repro of the original issue:

  $ cat ub.cc
  #include <cstdio>
  #include <cstdlib>
  
  static void Test(const char *x, int offset) {
    printf("%p + %d => %s\n", x, offset, x + offset ? "true" : "false");
  }
  
  int main(int argc, char **argv) {
    if (argc != 3) return 1;
  
    const char *x = reinterpret_cast<const char *>(atoi(argv[1]));
    int offset = atoi(argv[2]);
  
    Test(x, offset);
  
    return 0;
  }
  $ previous-clang++ -O3 ub.cc && ./a.out 0 1
  (nil) + 1 => true
  $ next-clang++ -O3 ub.cc && ./a.out 0 1
  (nil) + 1 => false
  $ patch-D67122-clang++ -O3 -fsanitize=undefined ub.cc && ./a.out 0 1
  ubsan: ub.cc:5:42: runtime error: applying non-zero offset 1 to null pointer                                                                                                    
  (nil) + 1 => false


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67122