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

[Richard Smith - zygoloid via Phabricator via llvm-commits]

rsmith added a comment.

Looks fine to me with some doc improvements.



================
Comment at: clang/docs/ReleaseNotes.rst:64-66
+  non-zero offset to ``nullptr`` (or making non-``nullptr`` a ``nullptr``,
+  by subtracting pointer's integral value from the pointer itself; in C, also,
+  applying *any* (even zero) offset to ``nullptr``) is undefined behaviour.
----------------
The parenthetical here makes this awkward to read. Also, I don't think we need to say which LLVM revision this happened in. How about:

"In both C and C++ (C17 6.5.6p8, C++ [expr.add]), pointer arithmetic is only permitted within arrays. In particular, the behavior of a program is not defined if it adds a non-zero offset (or in C, any offset) to a null pointer, or that forms a null pointer by subtracting an integer from a non-null pointer, and the LLVM optimizer now uses those guarantees for transformations. This may lead to unintended behavior in code that performs these operations. The Undefined Behavior Sanitizer `-fsanitize=pointer-overflow` check has been extended to detect these cases, so that code relying on them can be detected and fixed."


================
Comment at: clang/docs/ReleaseNotes.rst:242
 
-- ...
+- * ``pointer-overflow`` check was extended added to catch the cases where
+    a non-zero offset being applied, either to a ``nullptr``, or the result
----------------
Add "The" to the start of this bullet.


================
Comment at: clang/docs/ReleaseNotes.rst:243-244
+- * ``pointer-overflow`` check was extended added to catch the cases where
+    a non-zero offset being applied, either to a ``nullptr``, or the result
+    of applying of the offset is a ``nullptr``.
+    As per C++ Standard ``[expr.add]`` that is undefined behaviour.
----------------
"[...] where a non-zero offset is applied to a null pointer, or the result of applying the offset is a null pointer."


================
Comment at: clang/docs/ReleaseNotes.rst:245
+    of applying of the offset is a ``nullptr``.
+    As per C++ Standard ``[expr.add]`` that is undefined behaviour.
+
----------------
I don't think we really need to say this is undefined behavior here.


================
Comment at: clang/docs/UndefinedBehaviorSanitizer.rst:133
   -  ``-fsanitize=pointer-overflow``: Performing pointer arithmetic which
-     overflows.
+     overflows; applying a non-zero (in C++; in C - any) offset to either a
+     non-``nullptr``, or pointer becoming ``nullptr`` after applying the offset.
----------------
Simplify this a bit: "Performing pointer arithmetic which overflows, or where either the old or new pointer value is a null pointer (or in C, when they both are)."


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:4657
+      Builder.GetInsertBlock()->getParent(), PtrTy->getPointerAddressSpace());
+  // Check for overflows unless the GEP got constant-folded,
+  // and only in the default address space
----------------
If we want to split out the "constant folded" case to avoid issuing too many sanitizer traps on bogus but common patterns, we should have another sanitizer group to re-enable those diagnostics for the constant-folded cases. (I'm fine with not doing that in this patch, though.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67122