Skip to content

Commit

Permalink
Rollup merge of rust-lang#128277 - RalfJung:offset_from_wildcard, r=o…
Browse files Browse the repository at this point in the history 
…li-obk

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 rust-lang/miri#3767
  • Loading branch information
@matthiaskrgr
matthiaskrgr authored Jul 29, 2024
2 parents 7e6943d + f8ebe8d commit eb8114b
Show file tree
Hide file tree
Showing 80 changed files with 301 additions and 239 deletions.
34 changes: 23 additions & 11 deletions compiler/rustc_const_eval/messages.ftl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,9 @@ const_eval_copy_nonoverlapping_overlapping =
`copy_nonoverlapping` called on overlapping ranges
const_eval_dangling_int_pointer =
{$bad_pointer_message}: {$pointer} is a dangling pointer (it has no provenance)
{$bad_pointer_message}: {const_eval_expected_inbounds_pointer}, but got {$pointer} which is a dangling pointer (it has no provenance)
const_eval_dangling_null_pointer =
{$bad_pointer_message}: null pointer is a dangling pointer (it has no provenance)
{$bad_pointer_message}: {const_eval_expected_inbounds_pointer}, but got a null pointer
const_eval_dangling_ptr_in_final = encountered dangling pointer in final value of {const_eval_intern_kind}
const_eval_dead_local =
Expand Down Expand Up @@ -87,6 +87,13 @@ const_eval_error = {$error_kind ->
const_eval_exact_div_has_remainder =
exact_div: {$a} cannot be divided by {$b} without remainder
const_eval_expected_inbounds_pointer =
expected {$inbounds_size ->
[0] a pointer to some allocation
[1] a pointer to 1 byte of memory
*[x] a pointer to {$inbounds_size} bytes of memory
}
const_eval_extern_static =
cannot access extern static ({$did})
const_eval_extern_type_field = `extern type` field does not have a known offset
Expand Down Expand Up @@ -233,16 +240,17 @@ const_eval_nullary_intrinsic_fail =
const_eval_offset_from_different_allocations =
`{$name}` called on pointers into different allocations
const_eval_offset_from_different_integers =
`{$name}` called on different pointers without provenance (i.e., without an associated allocation)
const_eval_offset_from_overflow =
`{$name}` called when first pointer is too far ahead of second
const_eval_offset_from_test =
out-of-bounds `offset_from`
const_eval_offset_from_underflow =
`{$name}` called when first pointer is too far before second
const_eval_offset_from_unsigned_overflow =
`ptr_offset_from_unsigned` called when first pointer has smaller offset than second: {$a_offset} < {$b_offset}
`ptr_offset_from_unsigned` called when first pointer has smaller {$is_addr ->
[true] address
*[false] offset
} than second: {$a_offset} < {$b_offset}
const_eval_operator_non_const =
cannot call non-const operator in {const_eval_const_context}s
Expand All @@ -264,10 +272,16 @@ const_eval_pointer_arithmetic_overflow =
overflowing in-bounds pointer arithmetic
const_eval_pointer_arithmetic_test = out-of-bounds pointer arithmetic
const_eval_pointer_out_of_bounds =
{$bad_pointer_message}: {$alloc_id} has size {$alloc_size}, so pointer to {$ptr_size} {$ptr_size ->
[1] byte
*[many] bytes
} starting at offset {$ptr_offset} is out-of-bounds
{$bad_pointer_message}: {const_eval_expected_inbounds_pointer}, but got {$pointer} {$ptr_offset_is_neg ->
[true] which points to before the beginning of the allocation
*[false] {$alloc_size_minus_ptr_offset ->
[0] which is at or beyond the end of the allocation of size {$alloc_size ->
[1] 1 byte
*[x] {$alloc_size} bytes
}
*[x] and there are only {$alloc_size_minus_ptr_offset} bytes starting at that pointer
}
}
const_eval_pointer_use_after_free =
{$bad_pointer_message}: {$alloc_id} has been freed, so this pointer is dangling
const_eval_ptr_as_bytes_1 =
Expand Down Expand Up @@ -465,5 +479,3 @@ const_eval_write_through_immutable_pointer =
const_eval_write_to_read_only =
writing to {$allocation} which is read-only
const_eval_zst_pointer_out_of_bounds =
{$bad_pointer_message}: {$alloc_id} has size {$alloc_size}, so pointer at offset {$ptr_offset} is out-of-bounds
42 changes: 28 additions & 14 deletions compiler/rustc_const_eval/src/errors.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@ use rustc_errors::{
use rustc_hir::ConstContext;
use rustc_macros::{Diagnostic, LintDiagnostic, Subdiagnostic};
use rustc_middle::mir::interpret::{
CheckInAllocMsg, ExpectedKind, InterpError, InvalidMetaKind, InvalidProgramInfo, Misalignment,
PointerKind, ResourceExhaustionInfo, UndefinedBehaviorInfo, UnsupportedOpInfo,
ValidationErrorInfo,
CheckInAllocMsg, CtfeProvenance, ExpectedKind, InterpError, InvalidMetaKind,
InvalidProgramInfo, Misalignment, Pointer, PointerKind, ResourceExhaustionInfo,
UndefinedBehaviorInfo, UnsupportedOpInfo, ValidationErrorInfo,
};
use rustc_middle::ty::{self, Mutability, Ty};
use rustc_span::Span;
Expand Down Expand Up @@ -490,10 +490,9 @@ impl<'a> ReportErrorExt for UndefinedBehaviorInfo<'a> {
InvalidMeta(InvalidMetaKind::TooBig) => const_eval_invalid_meta,
UnterminatedCString(_) => const_eval_unterminated_c_string,
PointerUseAfterFree(_, _) => const_eval_pointer_use_after_free,
PointerOutOfBounds { ptr_size: Size::ZERO, .. } => const_eval_zst_pointer_out_of_bounds,
PointerOutOfBounds { .. } => const_eval_pointer_out_of_bounds,
DanglingIntPointer(0, _) => const_eval_dangling_null_pointer,
DanglingIntPointer(_, _) => const_eval_dangling_int_pointer,
DanglingIntPointer { addr: 0, .. } => const_eval_dangling_null_pointer,
DanglingIntPointer { .. } => const_eval_dangling_int_pointer,
AlignmentCheckFailed { .. } => const_eval_alignment_check_failed,
WriteToReadOnly(_) => const_eval_write_to_read_only,
DerefFunctionPointer(_) => const_eval_deref_function_pointer,
Expand Down Expand Up @@ -575,18 +574,33 @@ impl<'a> ReportErrorExt for UndefinedBehaviorInfo<'a> {
diag.arg("alloc_id", alloc_id)
.arg("bad_pointer_message", bad_pointer_message(msg, dcx));
}
PointerOutOfBounds { alloc_id, alloc_size, ptr_offset, ptr_size, msg } => {
diag.arg("alloc_id", alloc_id)
.arg("alloc_size", alloc_size.bytes())
.arg("ptr_offset", ptr_offset)
.arg("ptr_size", ptr_size.bytes())
PointerOutOfBounds { alloc_id, alloc_size, ptr_offset, inbounds_size, msg } => {
diag.arg("alloc_size", alloc_size.bytes())
.arg("inbounds_size", inbounds_size.bytes())
.arg("bad_pointer_message", bad_pointer_message(msg, dcx));
diag.arg(
"pointer",
Pointer::new(
Some(CtfeProvenance::from(alloc_id)),
Size::from_bytes(ptr_offset as u64),
)
.to_string(),
);
diag.arg("ptr_offset_is_neg", ptr_offset < 0);
diag.arg(
"alloc_size_minus_ptr_offset",
alloc_size.bytes().saturating_sub(ptr_offset as u64),
);
}
DanglingIntPointer(ptr, msg) => {
if ptr != 0 {
diag.arg("pointer", format!("{ptr:#x}[noalloc]"));
DanglingIntPointer { addr, inbounds_size, msg } => {
if addr != 0 {
diag.arg(
"pointer",
Pointer::<Option<CtfeProvenance>>::from_addr_invalid(addr).to_string(),
);
}

diag.arg("inbounds_size", inbounds_size.bytes());
diag.arg("bad_pointer_message", bad_pointer_message(msg, dcx));
}
AlignmentCheckFailed(Misalignment { required, has }, msg) => {
Expand Down
83 changes: 36 additions & 47 deletions compiler/rustc_const_eval/src/interpret/intrinsics.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
_ 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)
// Neither pointer points to an allocation, so they are both absolute.
(a, b, /*is_addr*/ true)
}
// 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.
Expand All @@ -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.
Expand All @@ -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
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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`.
Expand Down
31 changes: 28 additions & 3 deletions compiler/rustc_const_eval/src/interpret/memory.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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)?;
Expand All @@ -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,
})
}
Expand Down Expand Up @@ -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 changes: 4 additions & 4 deletions compiler/rustc_const_eval/src/interpret/validity.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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
Expand Down
Loading

0 comments on commit eb8114b

Please sign in to comment.