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.

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

Correctly construct Region to uphold deallocation safety invariant #2062

Merged
merged 6 commits into from
Mar 21, 2024
Merged
Show file tree
Hide file tree
Changes from 3 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
2 changes: 1 addition & 1 deletion packages/std/src/imports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ impl Api for ExternalApi {
}

fn addr_humanize(&self, canonical: &CanonicalAddr) -> StdResult<Addr> {
let send = build_region(&canonical);
let send = build_region(canonical.as_slice());
let send_ptr = &*send as *const Region as u32;
let human = alloc(HUMAN_ADDRESS_BUFFER_LENGTH);

Expand Down
79 changes: 74 additions & 5 deletions packages/std/src/memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ pub fn alloc(size: usize) -> *mut Region {
/// Similar to alloc, but instead of creating a new vector it consumes an existing one and returns
/// a pointer to the Region (preventing the memory from being freed until explicitly called later).
///
/// The resulting Region has capacity = length, i.e. the buffer's capacity is ignored.
/// The resulting Region has capacity = length, the buffer capacity is shrunk down to its length.
aumetra marked this conversation as resolved.
Show resolved Hide resolved
pub fn release_buffer(buffer: Vec<u8>) -> *mut Region {
let region = build_region(&buffer);
mem::forget(buffer);
Expand Down Expand Up @@ -69,15 +69,84 @@ pub unsafe fn consume_region(ptr: *mut Region) -> Vec<u8> {
)
}

/// Element that can be used to construct a new `Box<Region>`
///
/// # Safety
///
/// The following invariant must be upheld:
///
/// - full allocated capacity == value returned by capacity
///
/// This is important to uphold the safety invariant of the `dealloc` method, which requires us to pass the same Layout
/// into it as was used to allocate a memory region.
///
/// And since `size` is one of the parameters, it is important to pass in the exact same capacity.
aumetra marked this conversation as resolved.
Show resolved Hide resolved
///
/// See: <https://doc.rust-lang.org/stable/alloc/alloc/trait.GlobalAlloc.html#safety-2>
pub unsafe trait RegionSource {
fn ptr(&self) -> *const u8;
fn len(&self) -> usize;
fn capacity(&self) -> usize;
}

unsafe impl RegionSource for &[u8] {
fn ptr(&self) -> *const u8 {
self.as_ptr()
}

fn len(&self) -> usize {
(*self).len()
}

fn capacity(&self) -> usize {
self.len()
}
}

unsafe impl RegionSource for Vec<u8> {
fn ptr(&self) -> *const u8 {
self.as_ptr()
}

fn len(&self) -> usize {
self.len()
}

fn capacity(&self) -> usize {
self.capacity()
}
}

unsafe impl<T: ?Sized> RegionSource for &T
where
T: RegionSource,
{
fn ptr(&self) -> *const u8 {
(**self).ptr()
}

fn len(&self) -> usize {
(**self).len()
}

fn capacity(&self) -> usize {
(**self).capacity()
}
}

/// Returns a box of a Region, which can be sent over a call to extern
/// note that this DOES NOT take ownership of the data, and we MUST NOT consume_region
/// the resulting data.
/// The Box must be dropped (with scope), but not the data
pub fn build_region(data: &[u8]) -> Box<Region> {
let data_ptr = data.as_ptr() as usize;
pub fn build_region<S>(data: S) -> Box<Region>
where
S: RegionSource,
{
// Well, this technically violates pointer provenance rules.
// But there isn't a stable API for it, so that's the best we can do, I guess.
Copy link
Member

Choose a reason for hiding this comment

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

Which provenance rules are you referring to?

The whole memory module is wasm32 only, such that we know

  • The memory is a Wasm linear memory where memory can be addressed by offsets
  • usize is 32 bit long

Copy link
Member Author

@aumetra aumetra Mar 21, 2024

Choose a reason for hiding this comment

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

Yeah, makes sense, but it's just internally a bunch of hints for the compiler and miri.
It contains stuff like the address space, address, and some other metadata which a usize can technically not accurately represent since it's just a pointer somewhere in memory.

It's technically just an experiment but they also wanna explore if it makes sense for the compiler to enforce provenance at some point AFAIK.

https://doc.rust-lang.org/nightly/std/ptr/index.html#strict-provenance

But as the docs say

The following text is non-normative, insufficiently formal, and is an extremely strict interpretation of provenance. It’s ok if your code doesn’t strictly conform to it.

Just something that was on my mind and I commented in ¯\_(ツ)_/¯
We can remove the comment, too.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, all good, no worries. I was just not sure if there is anything practically relevant we need to be careful of.

build_region_from_components(
u32::try_from(data_ptr).expect("pointer doesn't fit in u32"),
u32::try_from(data.len()).expect("length doesn't fit in u32"),
u32::try_from(data.ptr() as usize).expect("pointer doesn't fit in u32"),
u32::try_from(data.capacity()).expect("capacity doesn't fit in u32"),
u32::try_from(data.len()).expect("length doesn't fit in u32"),
)
}
Expand Down
Loading