Auto merge of rust-lang#130251 - saethlin:ptr-offset-preconditions, r…

…=Amanieu Add precondition checks to ptr::offset, ptr::add, ptr::sub All of `offset`, `add`, and `sub` (currently) have the trivial preconditions that the offset in bytes must be <= isize::MAX, and the computation of the new address must not wrap. This adds precondition checks for these, and like in slice indexing, we use intrinsics directly to implement unsafe APIs that have explicit checks, because we get a clearer error message that mentions the misused API not an implementation detail. Experimentation indicates these checks have 1-2% compile time overhead, due primarily to adding the checks for `add`. A crater run (rust-lang#130251 (comment)) indicates some people currently have buggy calls to `ptr::offset` that apply a negative offset to a null pointer, but the crater run does not hit the `ptr::add` or `ptr::sub` checks, which seems like an argument for cfg'ing out those checks on account of their overhead.
rust-lang-ci · Oct 7, 2024 · fb5d33d · fb5d33d
2 parents 7caad69 + ee9b057
commit fb5d33d
Show file tree

Hide file tree

Showing 13 changed files with 285 additions and 43 deletions.
diff --git a/library/core/src/ptr/const_ptr.rs b/library/core/src/ptr/const_ptr.rs
@@ -395,6 +395,36 @@ impl<T: ?Sized> *const T {
     where
         T: Sized,
     {
+        #[inline]
+        const fn runtime_offset_nowrap(this: *const (), count: isize, size: usize) -> bool {
+            #[inline]
+            fn runtime(this: *const (), count: isize, size: usize) -> bool {
+                // We know `size <= isize::MAX` so the `as` cast here is not lossy.
+                let Some(byte_offset) = count.checked_mul(size as isize) else {
+                    return false;
+                };
+                let (_, overflow) = this.addr().overflowing_add_signed(byte_offset);
+                !overflow
+            }
+
+            const fn comptime(_: *const (), _: isize, _: usize) -> bool {
+                true
+            }
+
+            // We can use const_eval_select here because this is only for UB checks.
+            intrinsics::const_eval_select((this, count, size), comptime, runtime)
+        }
+
+        ub_checks::assert_unsafe_precondition!(
+            check_language_ub,
+            "ptr::offset requires the address calculation to not overflow",
+            (
+                this: *const () = self as *const (),
+                count: isize = count,
+                size: usize = size_of::<T>(),
+            ) => runtime_offset_nowrap(this, count, size)
+        );
+
         // SAFETY: the caller must uphold the safety contract for `offset`.
         unsafe { intrinsics::offset(self, count) }
     }
@@ -726,7 +756,6 @@ impl<T: ?Sized> *const T {
                 true
             }
 
-            #[allow(unused_unsafe)]
             intrinsics::const_eval_select((this, origin), comptime, runtime)
         }
 
@@ -858,6 +887,36 @@ impl<T: ?Sized> *const T {
     where
         T: Sized,
     {
+        #[cfg(debug_assertions)]
+        #[inline]
+        const fn runtime_add_nowrap(this: *const (), count: usize, size: usize) -> bool {
+            #[inline]
+            fn runtime(this: *const (), count: usize, size: usize) -> bool {
+                let Some(byte_offset) = count.checked_mul(size) else {
+                    return false;
+                };
+                let (_, overflow) = this.addr().overflowing_add(byte_offset);
+                byte_offset <= (isize::MAX as usize) && !overflow
+            }
+
+            const fn comptime(_: *const (), _: usize, _: usize) -> bool {
+                true
+            }
+
+            intrinsics::const_eval_select((this, count, size), comptime, runtime)
+        }
+
+        #[cfg(debug_assertions)] // Expensive, and doesn't catch much in the wild.
+        ub_checks::assert_unsafe_precondition!(
+            check_language_ub,
+            "ptr::add requires that the address calculation does not overflow",
+            (
+                this: *const () = self as *const (),
+                count: usize = count,
+                size: usize = size_of::<T>(),
+            ) => runtime_add_nowrap(this, count, size)
+        );
+
         // SAFETY: the caller must uphold the safety contract for `offset`.
         unsafe { intrinsics::offset(self, count) }
     }
@@ -936,14 +995,43 @@ impl<T: ?Sized> *const T {
     where
         T: Sized,
     {
+        #[cfg(debug_assertions)]
+        #[inline]
+        const fn runtime_sub_nowrap(this: *const (), count: usize, size: usize) -> bool {
+            #[inline]
+            fn runtime(this: *const (), count: usize, size: usize) -> bool {
+                let Some(byte_offset) = count.checked_mul(size) else {
+                    return false;
+                };
+                byte_offset <= (isize::MAX as usize) && this.addr() >= byte_offset
+            }
+
+            const fn comptime(_: *const (), _: usize, _: usize) -> bool {
+                true
+            }
+
+            intrinsics::const_eval_select((this, count, size), comptime, runtime)
+        }
+
+        #[cfg(debug_assertions)] // Expensive, and doesn't catch much in the wild.
+        ub_checks::assert_unsafe_precondition!(
+            check_language_ub,
+            "ptr::sub requires that the address calculation does not overflow",
+            (
+                this: *const () = self as *const (),
+                count: usize = count,
+                size: usize = size_of::<T>(),
+            ) => runtime_sub_nowrap(this, count, size)
+        );
+
         if T::IS_ZST {
             // Pointer arithmetic does nothing when the pointee is a ZST.
             self
         } else {
             // SAFETY: the caller must uphold the safety contract for `offset`.
             // Because the pointee is *not* a ZST, that means that `count` is
             // at most `isize::MAX`, and thus the negation cannot overflow.
-            unsafe { self.offset((count as isize).unchecked_neg()) }
+            unsafe { intrinsics::offset(self, intrinsics::unchecked_sub(0, count as isize)) }
         }
     }
 

diff --git a/library/core/src/ptr/mut_ptr.rs b/library/core/src/ptr/mut_ptr.rs
@@ -393,6 +393,37 @@ impl<T: ?Sized> *mut T {
     where
         T: Sized,
     {
+        #[inline]
+        const fn runtime_offset_nowrap(this: *const (), count: isize, size: usize) -> bool {
+            #[inline]
+            fn runtime(this: *const (), count: isize, size: usize) -> bool {
+                // `size` is the size of a Rust type, so we know that
+                // `size <= isize::MAX` and thus `as` cast here is not lossy.
+                let Some(byte_offset) = count.checked_mul(size as isize) else {
+                    return false;
+                };
+                let (_, overflow) = this.addr().overflowing_add_signed(byte_offset);
+                !overflow
+            }
+
+            const fn comptime(_: *const (), _: isize, _: usize) -> bool {
+                true
+            }
+
+            // We can use const_eval_select here because this is only for UB checks.
+            intrinsics::const_eval_select((this, count, size), comptime, runtime)
+        }
+
+        ub_checks::assert_unsafe_precondition!(
+            check_language_ub,
+            "ptr::offset requires the address calculation to not overflow",
+            (
+                this: *const () = self as *const (),
+                count: isize = count,
+                size: usize = size_of::<T>(),
+            ) => runtime_offset_nowrap(this, count, size)
+        );
+
         // SAFETY: the caller must uphold the safety contract for `offset`.
         // The obtained pointer is valid for writes since the caller must
         // guarantee that it points to the same allocated object as `self`.
@@ -940,6 +971,36 @@ impl<T: ?Sized> *mut T {
     where
         T: Sized,
     {
+        #[cfg(debug_assertions)]
+        #[inline]
+        const fn runtime_add_nowrap(this: *const (), count: usize, size: usize) -> bool {
+            #[inline]
+            fn runtime(this: *const (), count: usize, size: usize) -> bool {
+                let Some(byte_offset) = count.checked_mul(size) else {
+                    return false;
+                };
+                let (_, overflow) = this.addr().overflowing_add(byte_offset);
+                byte_offset <= (isize::MAX as usize) && !overflow
+            }
+
+            const fn comptime(_: *const (), _: usize, _: usize) -> bool {
+                true
+            }
+
+            intrinsics::const_eval_select((this, count, size), comptime, runtime)
+        }
+
+        #[cfg(debug_assertions)] // Expensive, and doesn't catch much in the wild.
+        ub_checks::assert_unsafe_precondition!(
+            check_language_ub,
+            "ptr::add requires that the address calculation does not overflow",
+            (
+                this: *const () = self as *const (),
+                count: usize = count,
+                size: usize = size_of::<T>(),
+            ) => runtime_add_nowrap(this, count, size)
+        );
+
         // SAFETY: the caller must uphold the safety contract for `offset`.
         unsafe { intrinsics::offset(self, count) }
     }
@@ -1018,14 +1079,43 @@ impl<T: ?Sized> *mut T {
     where
         T: Sized,
     {
+        #[cfg(debug_assertions)]
+        #[inline]
+        const fn runtime_sub_nowrap(this: *const (), count: usize, size: usize) -> bool {
+            #[inline]
+            fn runtime(this: *const (), count: usize, size: usize) -> bool {
+                let Some(byte_offset) = count.checked_mul(size) else {
+                    return false;
+                };
+                byte_offset <= (isize::MAX as usize) && this.addr() >= byte_offset
+            }
+
+            const fn comptime(_: *const (), _: usize, _: usize) -> bool {
+                true
+            }
+
+            intrinsics::const_eval_select((this, count, size), comptime, runtime)
+        }
+
+        #[cfg(debug_assertions)] // Expensive, and doesn't catch much in the wild.
+        ub_checks::assert_unsafe_precondition!(
+            check_language_ub,
+            "ptr::sub requires that the address calculation does not overflow",
+            (
+                this: *const () = self as *const (),
+                count: usize = count,
+                size: usize = size_of::<T>(),
+            ) => runtime_sub_nowrap(this, count, size)
+        );
+
         if T::IS_ZST {
             // Pointer arithmetic does nothing when the pointee is a ZST.
             self
         } else {
             // SAFETY: the caller must uphold the safety contract for `offset`.
             // Because the pointee is *not* a ZST, that means that `count` is
             // at most `isize::MAX`, and thus the negation cannot overflow.
-            unsafe { self.offset((count as isize).unchecked_neg()) }
+            unsafe { intrinsics::offset(self, intrinsics::unchecked_sub(0, count as isize)) }
         }
     }
 

diff --git a/tests/codegen/option-as-slice.rs b/tests/codegen/option-as-slice.rs
@@ -14,7 +14,9 @@ pub fn u64_opt_as_slice(o: &Option<u64>) -> &[u64] {
     // CHECK-NOT: br
     // CHECK-NOT: switch
     // CHECK-NOT: icmp
-    // CHECK: %[[LEN:.+]] = load i64,{{.+}} !range ![[META_U64:.+]], !noundef
+    // CHECK: %[[LEN:.+]] = load i64
+    // CHECK-SAME: !range ![[META_U64:[0-9]+]],
+    // CHECK-SAME: !noundef
     // CHECK-NOT: select
     // CHECK-NOT: br
     // CHECK-NOT: switch
@@ -51,7 +53,9 @@ pub fn u8_opt_as_slice(o: &Option<u8>) -> &[u8] {
     // CHECK-NOT: br
     // CHECK-NOT: switch
     // CHECK-NOT: icmp
-    // CHECK: %[[TAG:.+]] = load i8,{{.+}} !range ![[META_U8:.+]], !noundef
+    // CHECK: %[[TAG:.+]] = load i8
+    // CHECK-SAME: !range ![[META_U8:[0-9]+]],
+    // CHECK-SAME: !noundef
     // CHECK: %[[LEN:.+]] = zext{{.*}} i8 %[[TAG]] to i64
     // CHECK-NOT: select
     // CHECK-NOT: br

diff --git a/tests/mir-opt/pre-codegen/ptr_offset.demo_byte_add_fat.PreCodegen.after.panic-abort.mir b/tests/mir-opt/pre-codegen/ptr_offset.demo_byte_add_fat.PreCodegen.after.panic-abort.mir
@@ -10,12 +10,18 @@ fn demo_byte_add_fat(_1: *const [u32], _2: usize) -> *const [u32] {
         scope 2 (inlined std::ptr::const_ptr::<impl *const [u32]>::cast::<u8>) {
         }
         scope 3 (inlined std::ptr::const_ptr::<impl *const u8>::add) {
+            scope 4 (inlined core::ub_checks::check_language_ub) {
+                scope 5 (inlined core::ub_checks::check_language_ub::runtime) {
+                }
+            }
+            scope 6 (inlined std::mem::size_of::<u8>) {
+            }
         }
-        scope 4 (inlined std::ptr::const_ptr::<impl *const u8>::with_metadata_of::<[u32]>) {
+        scope 7 (inlined std::ptr::const_ptr::<impl *const u8>::with_metadata_of::<[u32]>) {
             let mut _5: usize;
-            scope 5 (inlined std::ptr::metadata::<[u32]>) {
+            scope 8 (inlined std::ptr::metadata::<[u32]>) {
             }
-            scope 6 (inlined std::ptr::from_raw_parts::<[u32], ()>) {
+            scope 9 (inlined std::ptr::from_raw_parts::<[u32], ()>) {
             }
         }
     }

diff --git a/tests/mir-opt/pre-codegen/ptr_offset.demo_byte_add_fat.PreCodegen.after.panic-unwind.mir b/tests/mir-opt/pre-codegen/ptr_offset.demo_byte_add_fat.PreCodegen.after.panic-unwind.mir
@@ -10,12 +10,18 @@ fn demo_byte_add_fat(_1: *const [u32], _2: usize) -> *const [u32] {
         scope 2 (inlined std::ptr::const_ptr::<impl *const [u32]>::cast::<u8>) {
         }
         scope 3 (inlined std::ptr::const_ptr::<impl *const u8>::add) {
+            scope 4 (inlined core::ub_checks::check_language_ub) {
+                scope 5 (inlined core::ub_checks::check_language_ub::runtime) {
+                }
+            }
+            scope 6 (inlined std::mem::size_of::<u8>) {
+            }
         }
-        scope 4 (inlined std::ptr::const_ptr::<impl *const u8>::with_metadata_of::<[u32]>) {
+        scope 7 (inlined std::ptr::const_ptr::<impl *const u8>::with_metadata_of::<[u32]>) {
             let mut _5: usize;
-            scope 5 (inlined std::ptr::metadata::<[u32]>) {
+            scope 8 (inlined std::ptr::metadata::<[u32]>) {
             }
-            scope 6 (inlined std::ptr::from_raw_parts::<[u32], ()>) {
+            scope 9 (inlined std::ptr::from_raw_parts::<[u32], ()>) {
             }
         }
     }

diff --git a/tests/mir-opt/pre-codegen/ptr_offset.demo_byte_add_thin.PreCodegen.after.panic-abort.mir b/tests/mir-opt/pre-codegen/ptr_offset.demo_byte_add_thin.PreCodegen.after.panic-abort.mir
@@ -10,11 +10,17 @@ fn demo_byte_add_thin(_1: *const u32, _2: usize) -> *const u32 {
         scope 2 (inlined std::ptr::const_ptr::<impl *const u32>::cast::<u8>) {
         }
         scope 3 (inlined std::ptr::const_ptr::<impl *const u8>::add) {
+            scope 4 (inlined core::ub_checks::check_language_ub) {
+                scope 5 (inlined core::ub_checks::check_language_ub::runtime) {
+                }
+            }
+            scope 6 (inlined std::mem::size_of::<u8>) {
+            }
         }
-        scope 4 (inlined std::ptr::const_ptr::<impl *const u8>::with_metadata_of::<u32>) {
-            scope 5 (inlined std::ptr::metadata::<u32>) {
+        scope 7 (inlined std::ptr::const_ptr::<impl *const u8>::with_metadata_of::<u32>) {
+            scope 8 (inlined std::ptr::metadata::<u32>) {
             }
-            scope 6 (inlined std::ptr::from_raw_parts::<u32, ()>) {
+            scope 9 (inlined std::ptr::from_raw_parts::<u32, ()>) {
             }
         }
     }

diff --git a/tests/mir-opt/pre-codegen/ptr_offset.demo_byte_add_thin.PreCodegen.after.panic-unwind.mir b/tests/mir-opt/pre-codegen/ptr_offset.demo_byte_add_thin.PreCodegen.after.panic-unwind.mir
@@ -10,11 +10,17 @@ fn demo_byte_add_thin(_1: *const u32, _2: usize) -> *const u32 {
         scope 2 (inlined std::ptr::const_ptr::<impl *const u32>::cast::<u8>) {
         }
         scope 3 (inlined std::ptr::const_ptr::<impl *const u8>::add) {
+            scope 4 (inlined core::ub_checks::check_language_ub) {
+                scope 5 (inlined core::ub_checks::check_language_ub::runtime) {
+                }
+            }
+            scope 6 (inlined std::mem::size_of::<u8>) {
+            }
         }
-        scope 4 (inlined std::ptr::const_ptr::<impl *const u8>::with_metadata_of::<u32>) {
-            scope 5 (inlined std::ptr::metadata::<u32>) {
+        scope 7 (inlined std::ptr::const_ptr::<impl *const u8>::with_metadata_of::<u32>) {
+            scope 8 (inlined std::ptr::metadata::<u32>) {
             }
-            scope 6 (inlined std::ptr::from_raw_parts::<u32, ()>) {
+            scope 9 (inlined std::ptr::from_raw_parts::<u32, ()>) {
             }
         }
     }