Skip to content

add const_leak and reject interning non-leaked const_allocate ptrs #143595

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.

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions compiler/rustc_const_eval/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,11 @@ const_eval_const_context = {$kind ->
*[other] {""}
}

const_eval_const_heap_ptr_in_final = encountered `const_allocate` pointer in final value that was not made global
.note = use `const_make_global` to make allocated pointers immutable before returning

const_eval_const_make_global_invalid_pointer = invalid pointer passed to `const_make_global`

const_eval_copy_nonoverlapping_overlapping =
`copy_nonoverlapping` called on overlapping ranges

Expand Down Expand Up @@ -338,6 +343,7 @@ const_eval_realloc_or_alloc_with_offset =
{$kind ->
[dealloc] deallocating
[realloc] reallocating
[leak] leaking
*[other] {""}
} {$ptr} which does not point to the beginning of an object

Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_const_eval/src/const_eval/eval_queries.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ fn eval_body_using_ecx<'tcx, R: InterpretationResult<'tcx>>(
// Since evaluation had no errors, validate the resulting constant.
const_validate_mplace(ecx, &ret, cid)?;

// Only report this after validation, as validaiton produces much better diagnostics.
// Only report this after validation, as validation produces much better diagnostics.
// FIXME: ensure validation always reports this and stop making interning care about it.

match intern_result {
Expand Down
31 changes: 31 additions & 0 deletions compiler/rustc_const_eval/src/const_eval/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,37 @@ impl<'tcx> interpret::Machine<'tcx> for CompileTimeMachine<'tcx> {
)?;
}
}

sym::const_make_global => {
let ptr = ecx.read_pointer(&args[0])?;
let size = ecx.read_scalar(&args[1])?.to_target_usize(ecx)?;
let align = ecx.read_scalar(&args[2])?.to_target_usize(ecx)?;

let _size: Size = Size::from_bytes(size);
let _align = match Align::from_bytes(align) {
Ok(a) => a,
Err(err) => throw_ub_custom!(
fluent::const_eval_invalid_align_details,
name = "const_make_global",
err_kind = err.diag_ident(),
align = err.align()
),
};

let (alloc_id, _, _) = ecx.ptr_get_alloc_id(ptr, 0)?;
let is_allocated_in_another_const = matches!(
ecx.tcx.try_get_global_alloc(alloc_id),
Some(interpret::GlobalAlloc::Memory(_))
);

if !is_allocated_in_another_const {
Copy link
Contributor

Choose a reason for hiding this comment

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

actually I think we don't need this check at all, leak_const_heap_ptr will just fail with its error path because the allocation is not in the current alloc_map

ecx.leak_const_heap_ptr(ptr)?;
ecx.write_pointer(ptr, dest)?;
} else {
throw_ub_custom!(fluent::const_eval_const_make_global_invalid_pointer);
}
}

// The intrinsic represents whether the value is known to the optimizer (LLVM).
// We're not doing any optimizations here, so there is no optimizer that could know the value.
// (We know the value here in the machine of course, but this is the runtime of that code,
Expand Down
8 changes: 8 additions & 0 deletions compiler/rustc_const_eval/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,14 @@ pub(crate) struct MutablePtrInFinal {
pub kind: InternKind,
}

#[derive(Diagnostic)]
#[diag(const_eval_const_heap_ptr_in_final)]
#[note]
pub(crate) struct ConstHeapPtrInFinal {
#[primary_span]
pub span: Span,
}

#[derive(Diagnostic)]
#[diag(const_eval_unstable_in_stable_exposed)]
pub(crate) struct UnstableInStableExposed {
Expand Down
45 changes: 40 additions & 5 deletions compiler/rustc_const_eval/src/interpret/intern.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ use super::{
};
use crate::const_eval;
use crate::const_eval::DummyMachine;
use crate::errors::NestedStaticInThreadLocal;
use crate::errors::{ConstHeapPtrInFinal, NestedStaticInThreadLocal};

pub trait CompileTimeMachine<'tcx, T> = Machine<
'tcx,
Expand All @@ -55,14 +55,36 @@ impl HasStaticRootDefId for const_eval::CompileTimeMachine<'_> {
}
}

pub enum DisallowInternReason {
ConstHeap,
}

pub trait CanIntern {
fn disallows_intern(&self) -> Option<DisallowInternReason>;
}

impl CanIntern for const_eval::MemoryKind {
fn disallows_intern(&self) -> Option<DisallowInternReason> {
match self {
const_eval::MemoryKind::Heap => Some(DisallowInternReason::ConstHeap),
}
}
}

impl CanIntern for ! {
fn disallows_intern(&self) -> Option<DisallowInternReason> {
*self
}
}

/// Intern an allocation. Returns `Err` if the allocation does not exist in the local memory.
///
/// `mutability` can be used to force immutable interning: if it is `Mutability::Not`, the
/// allocation is interned immutably; if it is `Mutability::Mut`, then the allocation *must be*
/// already mutable (as a sanity check).
///
/// Returns an iterator over all relocations referred to by this allocation.
fn intern_shallow<'tcx, T, M: CompileTimeMachine<'tcx, T>>(
fn intern_shallow<'tcx, T: CanIntern, M: CompileTimeMachine<'tcx, T>>(
ecx: &mut InterpCx<'tcx, M>,
alloc_id: AllocId,
mutability: Mutability,
Expand All @@ -71,9 +93,22 @@ fn intern_shallow<'tcx, T, M: CompileTimeMachine<'tcx, T>>(
trace!("intern_shallow {:?}", alloc_id);
// remove allocation
// FIXME(#120456) - is `swap_remove` correct?
let Some((_kind, mut alloc)) = ecx.memory.alloc_map.swap_remove(&alloc_id) else {
let Some((kind, mut alloc)) = ecx.memory.alloc_map.swap_remove(&alloc_id) else {
return Err(());
};

match kind {
MemoryKind::Machine(x) if let Some(reason) = x.disallows_intern() => match reason {
// attempting to intern a `const_allocate`d pointer that was not made global via
// `const_make_global`. We emit an error here but don't return an `Err` as if we
// did the caller assumes we found a dangling pointer.
DisallowInternReason::ConstHeap => {
ecx.tcx.dcx().emit_err(ConstHeapPtrInFinal { span: ecx.tcx.span });
}
},
MemoryKind::Machine(_) | MemoryKind::Stack | MemoryKind::CallerLocation => {}
}

// Set allocation mutability as appropriate. This is used by LLVM to put things into
// read-only memory, and also by Miri when evaluating other globals that
// access this one.
Expand All @@ -99,7 +134,7 @@ fn intern_shallow<'tcx, T, M: CompileTimeMachine<'tcx, T>>(
} else {
ecx.tcx.set_alloc_id_memory(alloc_id, alloc);
}
Ok(alloc.0.0.provenance().ptrs().iter().map(|&(_, prov)| prov))
Ok(alloc.inner().provenance().ptrs().iter().map(|&(_, prov)| prov))
}

/// Creates a new `DefId` and feeds all the right queries to make this `DefId`
Expand Down Expand Up @@ -321,7 +356,7 @@ pub fn intern_const_alloc_recursive<'tcx, M: CompileTimeMachine<'tcx, const_eval

/// Intern `ret`. This function assumes that `ret` references no other allocation.
#[instrument(level = "debug", skip(ecx))]
pub fn intern_const_alloc_for_constprop<'tcx, T, M: CompileTimeMachine<'tcx, T>>(
pub fn intern_const_alloc_for_constprop<'tcx, T: CanIntern, M: CompileTimeMachine<'tcx, T>>(
ecx: &mut InterpCx<'tcx, M>,
alloc_id: AllocId,
) -> InterpResult<'tcx, ()> {
Expand Down
18 changes: 18 additions & 0 deletions compiler/rustc_const_eval/src/interpret/memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,24 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
interp_ok(new_ptr)
}

pub fn leak_const_heap_ptr(
Copy link
Contributor

Choose a reason for hiding this comment

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

Also needs the name adjusted to non-leak terminology

&mut self,
ptr: Pointer<Option<CtfeProvenance>>,
) -> InterpResult<'tcx>
where
M: Machine<'tcx, Provenance = CtfeProvenance, AllocExtra = (), Bytes = Box<[u8]>>,
{
let (alloc_id, _, _) = self.ptr_get_alloc_id(ptr, 0)?;
let Some((_kind, mut alloc)) = self.memory.alloc_map.remove(&alloc_id) else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should check the _kind for Heap

return Err(err_ub!(PointerUseAfterFree(alloc_id, CheckInAllocMsg::MemoryAccess)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return Err(err_ub!(PointerUseAfterFree(alloc_id, CheckInAllocMsg::MemoryAccess)))
throw_ub!(PointerUseAfterFree(alloc_id, CheckInAllocMsg::MemoryAccess))

Copy link
Contributor

Choose a reason for hiding this comment

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

Also test this code path (both by trying it on a dangling pointer and by trying it on an already interned allocation)

.into();
};
alloc.mutability = Mutability::Not;
let alloc = self.tcx.mk_const_alloc(alloc);
self.tcx.set_alloc_id_memory(alloc_id, alloc);
interp_ok(())
Comment on lines +326 to +329
Copy link
Member

Choose a reason for hiding this comment

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

This risks breaking key invariants like "tcx-managed allocations do not have any dangling pointers". So as nice as this is I think it won't work.

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'm pretty sure both ptr_get_alloc_id and self.memory.alloc_map.remove(&alloc_id) calls above ensure that this allocation is valid, unless you mean that the allocation itself could contain values that are dangling pointers to other stuff?

Copy link
Member

Choose a reason for hiding this comment

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

Yes that's what I mean.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bit easier in this situation than during interning, because we don't need to intern nested allocations, so you could just iterate over the relocation table and check if all of the relocations in it are already interned

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we want that, as already mentioned it would make it impossible to use this on cyclic structures.

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'm thinking of only marking it as immutable here and leaving it for the interning to actually make it into global.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please also add a test for a cyclic globalification

}

#[instrument(skip(self), level = "debug")]
pub fn deallocate_ptr(
&mut self,
Expand Down
6 changes: 6 additions & 0 deletions compiler/rustc_hir_analysis/src/check/intrinsic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -415,6 +415,12 @@ pub(crate) fn check_intrinsic_type(
vec![Ty::new_mut_ptr(tcx, tcx.types.u8), tcx.types.usize, tcx.types.usize],
tcx.types.unit,
),
sym::const_make_global => (
0,
0,
vec![Ty::new_mut_ptr(tcx, tcx.types.u8), tcx.types.usize, tcx.types.usize],
Ty::new_imm_ptr(tcx, tcx.types.u8),
),

sym::ptr_offset_from => (
1,
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_span/src/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -714,6 +714,7 @@ symbols! {
const_indexing,
const_let,
const_loop,
const_make_global,
const_mut_refs,
const_panic,
const_panic_fmt,
Expand Down
8 changes: 6 additions & 2 deletions library/alloc/src/boxed/thin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
use core::error::Error;
use core::fmt::{self, Debug, Display, Formatter};
#[cfg(not(no_global_oom_handling))]
use core::intrinsics::const_allocate;
use core::intrinsics::{const_allocate, const_make_global};
use core::marker::PhantomData;
#[cfg(not(no_global_oom_handling))]
use core::marker::Unsize;
Expand Down Expand Up @@ -342,7 +342,11 @@ impl<H> WithHeader<H> {
metadata_ptr.write(ptr::metadata::<Dyn>(ptr::dangling::<T>() as *const Dyn));

// SAFETY: we have just written the metadata.
&*(metadata_ptr)
const_make_global(alloc, alloc_size, alloc_align)
.add(metadata_offset)
.cast::<<Dyn as Pointee>::Metadata>()
.as_ref()
.unwrap()
}
};

Expand Down
9 changes: 9 additions & 0 deletions library/core/src/intrinsics/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2533,6 +2533,15 @@ pub const unsafe fn const_deallocate(_ptr: *mut u8, _size: usize, _align: usize)
// Runtime NOP
}

#[rustc_const_unstable(feature = "const_heap", issue = "79597")]
#[rustc_nounwind]
#[rustc_intrinsic]
#[miri::intrinsic_fallback_is_spec]
pub const unsafe fn const_make_global(ptr: *mut u8, _size: usize, _align: usize) -> *const u8 {
// const eval overrides this function.
ptr
}

/// Returns whether we should perform contract-checking at runtime.
///
/// This is meant to be similar to the ub_checks intrinsic, in terms
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,11 @@ const FOO_RAW: *const i32 = foo();

const fn foo() -> &'static i32 {
let t = unsafe {
let i = intrinsics::const_allocate(4, 4) as * mut i32;
let i = intrinsics::const_allocate(4, 4) as *mut i32;
*i = 20;
i
};
unsafe { &*t }
unsafe { &*(intrinsics::const_make_global(t as *mut u8, 4, 4) as *mut i32) }
}
fn main() {
assert_eq!(*FOO, 20);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
error[E0080]: constructing invalid value at .<deref>: encountered uninitialized memory, but expected an integer
--> $DIR/alloc_intrinsic_uninit.rs:7:1
|
LL | const BAR: &i32 = unsafe { &*(intrinsics::const_allocate(4, 4) as *mut i32) };
LL | const BAR: &i32 = unsafe {
| ^^^^^^^^^^^^^^^ it is undefined behavior to use this value
|
= note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior.
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
error[E0080]: constructing invalid value at .<deref>: encountered uninitialized memory, but expected an integer
--> $DIR/alloc_intrinsic_uninit.rs:7:1
|
LL | const BAR: &i32 = unsafe { &*(intrinsics::const_allocate(4, 4) as *mut i32) };
LL | const BAR: &i32 = unsafe {
| ^^^^^^^^^^^^^^^ it is undefined behavior to use this value
|
= note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior.
Expand Down
5 changes: 3 additions & 2 deletions tests/ui/consts/const-eval/heap/alloc_intrinsic_uninit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#![feature(const_heap)]
use std::intrinsics;

const BAR: &i32 = unsafe { &*(intrinsics::const_allocate(4, 4) as *mut i32) };
//~^ ERROR: uninitialized memory
const BAR: &i32 = unsafe { //~ ERROR: uninitialized memory
&*(intrinsics::const_make_global(intrinsics::const_allocate(4, 4), 4, 4) as *mut i32)
};
fn main() {}
1 change: 1 addition & 0 deletions tests/ui/consts/const-eval/heap/alloc_intrinsic_untyped.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,6 @@ use std::intrinsics;

const BAR: *mut i32 = unsafe { intrinsics::const_allocate(4, 4) as *mut i32 };
//~^ error: mutable pointer in final value of constant
//~| error: encountered `const_allocate` pointer in final value that was not made global

fn main() {}
10 changes: 9 additions & 1 deletion tests/ui/consts/const-eval/heap/alloc_intrinsic_untyped.stderr
Original file line number Diff line number Diff line change
@@ -1,8 +1,16 @@
error: encountered `const_allocate` pointer in final value that was not made global
--> $DIR/alloc_intrinsic_untyped.rs:8:1
|
LL | const BAR: *mut i32 = unsafe { intrinsics::const_allocate(4, 4) as *mut i32 };
| ^^^^^^^^^^^^^^^^^^^
|
= note: use `const_make_global` to make allocated pointers immutable before returning

error: encountered mutable pointer in final value of constant
--> $DIR/alloc_intrinsic_untyped.rs:8:1
|
LL | const BAR: *mut i32 = unsafe { intrinsics::const_allocate(4, 4) as *mut i32 };
| ^^^^^^^^^^^^^^^^^^^

error: aborting due to 1 previous error
error: aborting due to 2 previous errors

2 changes: 1 addition & 1 deletion tests/ui/consts/const-eval/heap/dealloc_intrinsic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ const _X: () = unsafe {
const Y: &u32 = unsafe {
let ptr = intrinsics::const_allocate(4, 4) as *mut u32;
*ptr = 42;
&*ptr
&*(intrinsics::const_make_global(ptr as *mut u8, 4, 4) as *const u32)
};

const Z: &u32 = &42;
Expand Down
14 changes: 14 additions & 0 deletions tests/ui/consts/const-eval/heap/ptr_made_global_mutated.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
#![feature(core_intrinsics)]
#![feature(const_heap)]
use std::intrinsics;

const A: &u8 = unsafe {
let ptr = intrinsics::const_allocate(1, 1);
*ptr = 1;
let ptr: *const u8 = intrinsics::const_make_global(ptr, 1, 1);
*(ptr as *mut u8) = 2;
//~^ error: writing to ALLOC0 which is read-only
&*ptr
};

fn main() {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
error[E0080]: writing to ALLOC0 which is read-only
--> $DIR/ptr_made_global_mutated.rs:9:5
|
LL | *(ptr as *mut u8) = 2;
| ^^^^^^^^^^^^^^^^^^^^^ evaluation of `A` failed here

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0080`.
19 changes: 19 additions & 0 deletions tests/ui/consts/const-eval/heap/ptr_not_made_global.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
#![feature(core_intrinsics)]
#![feature(const_heap)]
use std::intrinsics;

const FOO: &i32 = foo();
//~^ error: encountered `const_allocate` pointer in final value that was not made global
const FOO_RAW: *const i32 = foo();
//~^ error: encountered `const_allocate` pointer in final value that was not made global

const fn foo() -> &'static i32 {
let t = unsafe {
let i = intrinsics::const_allocate(4, 4) as *mut i32;
*i = 20;
i
};
unsafe { &*t }
}

fn main() {}
18 changes: 18 additions & 0 deletions tests/ui/consts/const-eval/heap/ptr_not_made_global.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
error: encountered `const_allocate` pointer in final value that was not made global
--> $DIR/ptr_not_made_global.rs:5:1
|
LL | const FOO: &i32 = foo();
| ^^^^^^^^^^^^^^^
|
= note: use `const_make_global` to make allocated pointers immutable before returning

error: encountered `const_allocate` pointer in final value that was not made global
--> $DIR/ptr_not_made_global.rs:7:1
|
LL | const FOO_RAW: *const i32 = foo();
| ^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: use `const_make_global` to make allocated pointers immutable before returning

error: aborting due to 2 previous errors

Loading