Potential UB returning mutable bytes as MemoryBlock

[HeroicKatora]

alloc-compose/src/region.rs

Lines 86 to 96 in 375d762

	fn alloc(&mut self, layout: Layout, init: AllocInit) -> Result<MemoryBlock, AllocErr> {
	let new_ptr = self.ptr.checked_sub(layout.size()).ok_or(AllocErr)?;
	let aligned_ptr = new_ptr & !(layout.align() - 1);
	if unlikely(aligned_ptr < self.start()) {
	return Err(AllocErr);
	}
	let memory = self.create_block(aligned_ptr)?;
	unsafe { init.init(memory) };
	Ok(memory)

These lines return part of the data as a MemoryBlock. This unsafe operation permits the caller to write uninitialized bytes to that region. It's not yet clear if it is UB to do that. In any case it is a safety invariant so you mustn't leak this uninitialized state after your borrow of data has ended but there is no Drop implementation that would take care of it.

[TimDiekmann]

As &mut [u8] is passed as data to Region I don't see any problems here, as MemoryBlock just returns a NonNull<u8> pointing into the data. Additionally, the unsafe block in the mentioned code will be removed as soon as rust-lang/rust#74850 has landed (It's removing InPlace-allocation and splits alloc and alloc_zeroed - thus also removing AllocInit). Even now it's only zeroing the content if specified by AllocInit and a zeroed slice of u8 is well defined as 0_u8 is well defined. This method is (besides initializing the slice) written without any unsafe code and writing to a pointer is unsafe. Am I missing something here?

[HeroicKatora]

The NonNull<u8> must be valid for writing any data with the request layout, as required by the unsafe trait AllocRef. Such data can contain uninitialized padding bytes (edit: I don't actually see that requirement in the doc but not having it would make it useless for allocating boxes of structs with padding so I must assume it is an omission and intended to be there).

[TimDiekmann]

From the current AllocRef documentation:

Safety

• Memory blocks returned from an allocator must point to valid memory and retain their validity until the instance and all of its clones are dropped,
• cloning or moving the allocator must not invalidate memory blocks returned from this allocator. A cloned allocator must behave like the same allocator, and
• any pointer to a memory block which is currently allocated may be passed to any other method of the allocator.

It's not mentioned there directly. Even if it would be mentioned, I think this still holds as there is no bit pattern, which is undefined for u8, it's only up to you how you interpret those bits. The typical representation (a number) is defined for any 256 bit patterns. Regarding uninitialized memory: As you pass &mut [u8] to Region, the initial data is well defined, you already owned the memory before.

[TimDiekmann]

If this is UB in any way, I'm fine with changing the signature of Region::new to take two pointers or one pointer and a size, but currently I don't see a reason as passing a mutable slice is pretty convenient:

let mut data = [0; 64];
let mut alloc = Region::new(&mut data);

vs

let mut data = [0; 64];
let mut alloc = Region::new(data.as_mut_ptr(), data.len());
// make sure you don't use `data`, as `Region` don't have a lifetime bound anymore

[HeroicKatora]

Even if it would be mentioned, I think this still holds as there is no bit pattern, which is undefined for u8, it's only up to you how you interpret those bits.

Not yet clear, as quoted above (rust-lang/unsafe-code-guidelines#71)

As you pass &mut [u8] to Region, the initial data is well defined

And in particular must not contain any uninitialized bytes (nit: if non-empty). It is safe to dereference a slice reference and do something like if data[0] which is instant UB if data[0] is an uninitialized byte. Thus not containing uninitialized bytes is surely a safety invariant you have to uphold and maybe (under debate of ucg) also a validity invariant for reference.

[TimDiekmann]

Okay, I now understand you point I guess, sry for taking so long 😄

I skimmed your linked issue. I think a good solution in any case (allowed or not) would be using Region::new(data: &mut [MaybeUninit<u8>])?