rust-lang/rust
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

Rollup merge of #128277 - RalfJung:offset_from_wildcard, r=oli-obk

Browse Source 
miri: fix offset_from behavior on wildcard pointers

offset_from wouldn't behave correctly when the "end" pointer was a wildcard pointer (result of an int2ptr cast) just at the end of the allocation. Fix that by expressing the "same allocation" check in terms of two `check_ptr_access_signed` instead of something specific to offset_from, which is both more canonical and works better with wildcard pointers.

The second commit just improves diagnostics: I wanted the "pointer is dangling (has no provenance)" message to say how many bytes of memory it expected to see (since if it were 0 bytes, this would actually be legal, so it's good to tell the user that it's not 0 bytes). And then I was annoying that the error looks so different for when you deref a dangling pointer vs an out-of-bounds pointer so I made them more similar.

Fixes https://github.com/rust-lang/miri/issues/3767
This commit is contained in:
Matthias Krüger
2024-07-29 11:42:34 +02:00
committed by GitHub
parent 7e6943d67f f8ebe8d783
commit eb8114bad7
80 changed files with 301 additions and 239 deletions
Download Patch File Download Diff File Expand all files Collapse all files

83
compiler/rustc_const_eval/src/interpret/intrinsics.rs
View File

@@ -238,36 +238,22 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
let isize_layout = self.layout_of(self.tcx.types.isize)?;
// Get offsets for both that are at least relative to the same base.
let (a_offset, b_offset) =
// With `OFFSET_IS_ADDR` this is trivial; without it we need either
// two integers or two pointers into the same allocation.
let (a_offset, b_offset, is_addr) = if M::Provenance::OFFSET_IS_ADDR {
(a.addr().bytes(), b.addr().bytes(), /*is_addr*/ true)
} else {
match (self.ptr_try_get_alloc_id(a), self.ptr_try_get_alloc_id(b)) {
(Err(a), Err(b)) => {
// Neither pointer points to an allocation.
// This is okay only if they are the same.
if a != b {
// We'd catch this below in the "dereferenceable" check, but
// show a nicer error for this particular case.
throw_ub_custom!(
fluent::const_eval_offset_from_different_integers,
name = intrinsic_name,
);
}
// This will always return 0.
(a, b)
// Neither pointer points to an allocation, so they are both absolute.
(a, b, /*is_addr*/ true)
}
_ if M::Provenance::OFFSET_IS_ADDR && a.addr() == b.addr() => {
// At least one of the pointers has provenance, but they also point to
// the same address so it doesn't matter; this is fine. `(0, 0)` means
// we pass all the checks below and return 0.
(0, 0)
}
// From here onwards, the pointers are definitely for different addresses
// (or we can't determine their absolute address).
(Ok((a_alloc_id, a_offset, _)), Ok((b_alloc_id, b_offset, _)))
if a_alloc_id == b_alloc_id =>
{
// Found allocation for both, and it's the same.
// Use these offsets for distance calculation.
(a_offset.bytes(), b_offset.bytes())
(a_offset.bytes(), b_offset.bytes(), /*is_addr*/ false)
}
_ => {
// Not into the same allocation -- this is UB.
@@ -276,9 +262,10 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
name = intrinsic_name,
);
}
};
}
};
// Compute distance.
// Compute distance: a - b.
let dist = {
// Addresses are unsigned, so this is a `usize` computation. We have to do the
// overflow check separately anyway.
@@ -295,6 +282,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
fluent::const_eval_offset_from_unsigned_overflow,
a_offset = a_offset,
b_offset = b_offset,
is_addr = is_addr,
);
}
// The signed form of the intrinsic allows this. If we interpret the
@@ -323,14 +311,23 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
}
};
// Check that the range between them is dereferenceable ("in-bounds or one past the
// end of the same allocation"). This is like the check in ptr_offset_inbounds.
let min_ptr = if dist >= 0 { b } else { a };
self.check_ptr_access(
min_ptr,
Size::from_bytes(dist.unsigned_abs()),
// Check that the memory between them is dereferenceable at all, starting from the
// base pointer: `dist` is `a - b`, so it is based on `b`.
self.check_ptr_access_signed(b, dist, CheckInAllocMsg::OffsetFromTest)?;
// Then check that this is also dereferenceable from `a`. This ensures that they are
// derived from the same allocation.
self.check_ptr_access_signed(
a,
dist.checked_neg().unwrap(), // i64::MIN is impossible as no allocation can be that large
CheckInAllocMsg::OffsetFromTest,
)?;
)
.map_err(|_| {
// Make the error more specific.
err_ub_custom!(
fluent::const_eval_offset_from_different_allocations,
name = intrinsic_name,
)
})?;
// Perform division by size to compute return value.
let ret_layout = if intrinsic_name == sym::ptr_offset_from_unsigned {
@@ -577,27 +574,19 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
}
/// Offsets a pointer by some multiple of its type, returning an error if the pointer leaves its
/// allocation. For integer pointers, we consider each of them their own tiny allocation of size
/// 0, so offset-by-0 (and only 0) is okay -- except that null cannot be offset by _any_ value.
/// allocation.
pub fn ptr_offset_inbounds(
&self,
ptr: Pointer<Option<M::Provenance>>,
offset_bytes: i64,
) -> InterpResult<'tcx, Pointer<Option<M::Provenance>>> {
// The offset being in bounds cannot rely on "wrapping around" the address space.
// So, first rule out overflows in the pointer arithmetic.
let offset_ptr = ptr.signed_offset(offset_bytes, self)?;
// ptr and offset_ptr must be in bounds of the same allocated object. This means all of the
// memory between these pointers must be accessible. Note that we do not require the
// pointers to be properly aligned (unlike a read/write operation).
let min_ptr = if offset_bytes >= 0 { ptr } else { offset_ptr };
// This call handles checking for integer/null pointers.
self.check_ptr_access(
min_ptr,
Size::from_bytes(offset_bytes.unsigned_abs()),
CheckInAllocMsg::PointerArithmeticTest,
)?;
Ok(offset_ptr)
// We first compute the pointer with overflow checks, to get a specific error for when it
// overflows (though technically this is redundant with the following inbounds check).
let result = ptr.signed_offset(offset_bytes, self)?;
// The offset must be in bounds starting from `ptr`.
self.check_ptr_access_signed(ptr, offset_bytes, CheckInAllocMsg::PointerArithmeticTest)?;
// Done.
Ok(result)
}
/// Copy `count*size_of::<T>()` many bytes from `*src` to `*dst`.

31
compiler/rustc_const_eval/src/interpret/memory.rs
View File

@@ -411,6 +411,25 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
Ok(())
}
/// Check whether the given pointer points to live memory for a signed amount of bytes.
/// A negative amounts means that the given range of memory to the left of the pointer
/// needs to be dereferenceable.
pub fn check_ptr_access_signed(
&self,
ptr: Pointer<Option<M::Provenance>>,
size: i64,
msg: CheckInAllocMsg,
) -> InterpResult<'tcx> {
if let Ok(size) = u64::try_from(size) {
self.check_ptr_access(ptr, Size::from_bytes(size), msg)
} else {
// Compute the pointer at the beginning of the range, and do the standard
// dereferenceability check from there.
let begin_ptr = ptr.wrapping_signed_offset(size, self);
self.check_ptr_access(begin_ptr, Size::from_bytes(size.unsigned_abs()), msg)
}
}
/// Low-level helper function to check if a ptr is in-bounds and potentially return a reference
/// to the allocation it points to. Supports both shared and mutable references, as the actual
/// checking is offloaded to a helper closure.
@@ -437,7 +456,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
Ok(match self.ptr_try_get_alloc_id(ptr) {
Err(addr) => {
// We couldn't get a proper allocation.
throw_ub!(DanglingIntPointer(addr, msg));
throw_ub!(DanglingIntPointer { addr, inbounds_size: size, msg });
}
Ok((alloc_id, offset, prov)) => {
let (alloc_size, _alloc_align, ret_val) = alloc_size(alloc_id, offset, prov)?;
@@ -448,7 +467,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
alloc_id,
alloc_size,
ptr_offset: self.target_usize_to_isize(offset.bytes()),
ptr_size: size,
inbounds_size: size,
msg,
})
}
@@ -1421,7 +1440,13 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
ptr: Pointer<Option<M::Provenance>>,
) -> InterpResult<'tcx, (AllocId, Size, M::ProvenanceExtra)> {
self.ptr_try_get_alloc_id(ptr).map_err(|offset| {
err_ub!(DanglingIntPointer(offset, CheckInAllocMsg::InboundsTest)).into()
err_ub!(DanglingIntPointer {
addr: offset,
// We don't know the actually required size.
inbounds_size: Size::ZERO,
msg: CheckInAllocMsg::InboundsTest
})
.into()
})
}
}

8
compiler/rustc_const_eval/src/interpret/validity.rs
View File

@@ -348,7 +348,7 @@ impl<'rt, 'tcx, M: Machine<'tcx>> ValidityVisitor<'rt, 'tcx, M> {
try_validation!(
self.ecx.get_ptr_vtable_ty(vtable, Some(data)),
self.path,
Ub(DanglingIntPointer(..) | InvalidVTablePointer(..)) =>
Ub(DanglingIntPointer{ .. } | InvalidVTablePointer(..)) =>
InvalidVTablePtr { value: format!("{vtable}") },
Ub(InvalidVTableTrait { expected_trait, vtable_trait }) => {
InvalidMetaWrongTrait { expected_trait, vtable_trait: *vtable_trait }
@@ -405,8 +405,8 @@ impl<'rt, 'tcx, M: Machine<'tcx>> ValidityVisitor<'rt, 'tcx, M> {
CheckInAllocMsg::InboundsTest, // will anyway be replaced by validity message
),
self.path,
Ub(DanglingIntPointer(0, _)) => NullPtr { ptr_kind },
Ub(DanglingIntPointer(i, _)) => DanglingPtrNoProvenance {
Ub(DanglingIntPointer { addr: 0, .. }) => NullPtr { ptr_kind },
Ub(DanglingIntPointer { addr: i, .. }) => DanglingPtrNoProvenance {
ptr_kind,
// FIXME this says "null pointer" when null but we need translate
pointer: format!("{}", Pointer::<Option<AllocId>>::from_addr_invalid(*i))
@@ -605,7 +605,7 @@ impl<'rt, 'tcx, M: Machine<'tcx>> ValidityVisitor<'rt, 'tcx, M> {
let _fn = try_validation!(
self.ecx.get_ptr_fn(ptr),
self.path,
Ub(DanglingIntPointer(..) | InvalidFunctionPointer(..)) =>
Ub(DanglingIntPointer{ .. } | InvalidFunctionPointer(..)) =>
InvalidFnPtr { value: format!("{ptr}") },
);
// FIXME: Check if the signature matches