Skip to content

Add precondition checks to ptr::offset, ptr::add, ptr::sub #130251

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Oct 8, 2024
Merged

Add precondition checks to ptr::offset, ptr::add, ptr::sub #130251

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6d246e4
Add precondition checks to ptr::offset, ptr::add, ptr::sub
saethlin Sep 11, 2024
9d5c961
cfg out checks in add and sub but not offset
saethlin Oct 7, 2024
128ccc3
Bless mir-opt tests
saethlin Oct 7, 2024
8d562f6
Disable slice_iter mir-opt test in debug builds
saethlin Oct 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 90 additions & 2 deletions library/core/src/ptr/const_ptr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) }
}
Expand Down Expand Up @@ -726,7 +756,6 @@ impl<T: ?Sized> *const T {
true
}

#[allow(unused_unsafe)]
intrinsics::const_eval_select((this, origin), comptime, runtime)
}

Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do this avoiding .addr()? That's a ptr2int in LLVM, which exposes, and thus can't be optimized out.

Perhaps have this: *const u8 and phrase it as this.wrapping_add(byte_offset) >= this instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a ptr2int in LLVM, which exposes, and thus can't be optimized out.

It's a transmute, not a ptr2int... but LLVM might turn one into the other.
LLVM does optimize out ptr2int (which is not correct, but that doesn't stop it).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, all the more reason to stay in pointer-land if we can then, to avoid those complications :D

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is worth thinking about this. This is a UB check and won't even be monomorphized in a build that cares about this level of optimization. And other UB checks are already riddled with calls to .addr(); if you want to try to remove them all you're welcome to try.

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) }
}
Expand Down Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since sub calls unchecked_neg and offset, does this need its own check for anything more than count * size <= isize::MAX?

Or if it's going to have one for a better error message, it might be good to change the implementation to use the intrinsics directly (offset(self, unchecked_sub(0, count as isize))) to avoid emitting three different ubchecks when calling sub.

#[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)) }
}
}

Expand Down
92 changes: 91 additions & 1 deletion library/core/src/ptr/mut_ptr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Expand Down Expand Up @@ -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) }
}
Expand Down Expand Up @@ -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)) }
}
}

Expand Down
8 changes: 6 additions & 2 deletions tests/codegen/option-as-slice.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions tests/mir-opt/pre-codegen/slice_iter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// skip-filecheck
//@ compile-flags: -O -C debuginfo=0 -Zmir-opt-level=2
//@ only-64bit (constants for `None::<&T>` show in the output)
//@ ignore-debug: precondition checks on ptr::add are under cfg(debug_assertions)
// EMIT_MIR_FOR_EACH_PANIC_STRATEGY

#![crate_type = "lib"]
Expand Down
Loading