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

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a subscriber: dtzWill.
vsk added a comment.

Thanks, this is looking pretty good.



================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:4538
+                 llvm::Value * /*OffsetOverflows*/>
+EmitGEPOffsetInBytes(Value *BasePtr, Value *GEPVal,
+                     llvm::LLVMContext &VMContext, CodeGenModule &CGM,
----------------
Please document this helper. Also, consider having it return a struct, so that the field names don't need to be specified as comments. It'd be great to move the documentation for the TotalOffset and OffsetOverflows fields in EmitCheckedInBoundsGEP into the struct definition.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:4670
+          OffsetOverflows == Builder.getFalse()) &&
+         "If the offset got constant-folded, we expect that there was no "
+         "overflows.");
----------------
nit: don't expect


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:4692
+    // as a result, so the offset can not be -intptr_t(BasePtr).
+    // In other words, both bointers are either null, or both are non-null,
+    // or the behaviour is undefined.
----------------
nit: pointers


================
Comment at: clang/test/CodeGen/catch-nullptr-and-nonzero-offset-blacklist.c:1
+// RUN: %clang_cc1 -fsanitize=pointer-overflow -fsanitize-recover=pointer-overflow -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s -implicit-check-not="call void @__ubsan_handle_pointer_overflow" --check-prefixes=CHECK,CHECK-NULL-IS-INVALID-PTR
+// RUN: %clang_cc1 -fno-delete-null-pointer-checks -fsanitize=pointer-overflow -fsanitize-recover=pointer-overflow -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s -implicit-check-not="call void @__ubsan_handle_pointer_overflow" --check-prefixes=CHECK,CHECK-NULL-IS-VALID-PTR
----------------
Should the test filename be "ignore-nullptr-and-nonzero-..."?


================
Comment at: clang/test/CodeGen/catch-nullptr-and-nonzero-offset-blacklist.c:5
+// CHECK-LABEL: @baseline
+char *baseline(char *base, unsigned long offset) {
+  // CHECK: call void @__ubsan_handle_pointer_overflow(
----------------
The baseline case is tested elsewhere, why include it here?


================
Comment at: clang/test/CodeGen/catch-nullptr-and-nonzero-offset-in-offsetof-idiom.c:1
+// RUN: %clang_cc1 -x c   -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s
+// RUN: %clang_cc1 -x c   -fsanitize=pointer-overflow -fno-sanitize-recover=pointer-overflow -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s
----------------
Should the test filename be "ignore-nullptr-and-nonzero-..."?


================
Comment at: clang/test/CodeGen/catch-nullptr-and-nonzero-offset-in-offsetof-idiom.c:14
+uintptr_t get_offset_of_y() {
+  // CHECK:      define i64 @{{.*}}() {{.*}} {
+  // CHECK-NEXT: [[ENTRY:.*]]:
----------------
nit: CHECK-LABEL: define i64 @get_offset_of_y(


================
Comment at: clang/test/CodeGen/catch-nullptr-and-nonzero-offset.c:12
+// So in other words, the offset can not change "null status" of the pointer,
+// as in if the pointer was null, it can not become non-null, and vice verse,
+// if it was non-null, it can not become null.
----------------
nit: versa


================
Comment at: clang/test/CodeGen/catch-nullptr-and-nonzero-offset.c:28
+
+// We just don't know, can be bad, can be ok.
+char *var_var(char *base, unsigned long offset) {
----------------
I don't quite follow the comments. Perhaps it would be simpler to list the expected checks in the comment, e.g. "The base and offset are unknown. Check for pointer wrap and null status change."



================
Comment at: clang/test/CodeGen/catch-nullptr-and-nonzero-offset.c:66
+  // CHECK: define i8* @var_zero_OK(i8* %[[BASE:.*]])
+  // CHECK-NEXT: [[ENTRY:.*]]:
+  // CHECK-NEXT: %[[BASE_ADDR:.*]] = alloca i8*, align 8
----------------
Does this simplify to:

```
CHECK-LABEL: define i8* @var_zero_ok(
CHECK-NOT: !nosanitize
```


================
Comment at: clang/test/CodeGen/ubsan-pointer-overflow.m:48
 void pointer_array_unsigned_indices(int **arr, unsigned k) {
   // CHECK: icmp uge
   // CHECK-NOT: select
----------------
I'm curious about what happens here with the new null-status-change check. If array indices are unsigned, there is no need for a separate null-status-change check: if the result of the GEP is indeed null, that will be detected by the pointer wraparound check. We just need to check that the base pointer itself isn't null.

That generalizes to addition of unsigned offsets to base pointers, I think. So e.g. I wouldn't expect the 'preinc' function to contain a null-status-change check either.


================
Comment at: clang/test/CodeGenCXX/catch-nullptr-and-nonzero-offset-in-offsetof-idiom.cpp:1
+// RUN: %clang_cc1 -x c++ -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s
+// RUN: %clang_cc1 -x c++ -fsanitize=pointer-overflow -fno-sanitize-recover=pointer-overflow -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s
----------------
How is this different from the other offsetof test file?


================
Comment at: compiler-rt/test/ubsan/TestCases/Pointer/nullptr-and-nonzero-offset-constants.cpp:22
+  result = base - 1;
+  // CHECK: {{.*}}.cpp:[[@LINE-1]]:17: runtime error: applying non-zero offset to non-null pointer 0x{{.*}} producing null pointer
+
----------------
Should be 'produced null pointer' or 'gave null result'


================
Comment at: compiler-rt/test/ubsan_minimal/TestCases/nullptr-and-nonzero-offset.c:1
+// RUN: %clang -fsanitize=pointer-overflow %s -o %t && %run %t 2>&1 | FileCheck %s --check-prefixes=CHECK
+
----------------
implicit-check-not="pointer-overflow"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67122