Skip to content
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

Jump to bottom

miri: fix offset_from behavior on wildcard pointers #128277

Merged
merged 2 commits into from
Jul 29, 2024
Merged

miri: fix offset_from behavior on wildcard pointers #128277

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5b38b14
miri: fix offset_from behavior on wildcard pointers
RalfJung Jul 27, 2024
f8ebe8d
improve dangling/oob errors and make them more uniform
RalfJung Jul 27, 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
34 changes: 23 additions & 11 deletions compiler/rustc_const_eval/messages.ftl
View file
Open in desktop
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,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 @@ -488,10 +488,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 @@ -573,18 +572,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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,36 +243,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 @@ -281,9 +267,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 @@ -300,6 +287,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 @@ -328,14 +316,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 @@ -582,27 +579,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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,6 +414,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 @@ -440,7 +459,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 @@ -451,7 +470,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 @@ -1424,7 +1443,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
Open in desktop
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
Loading