search

fastly / terrarium-rust-guest

The "http_guest" crate used by Fastly Labs Terrarium https://wasm.fastlylabs.com/
Apache License 2.0
29 stars 4 forks source link

`guest_allocator::free` should be unsafe #4

Open adamreichold opened 5 years ago

adamreichold commented 5 years ago

The guest_allocator::free public function has no way to validate its ptr argument and hence should be declared unsafe instead of claiming to be safe and using an unsafe block. Callers of the function, e.g. DNS::query_raw must then assure that the pointer passed is valid and was created by guest_allocator::default_malloc_impl (or at least Box::into_raw) using an unsafe block as they actually have that knowledge as the pointer was returned by a previous host call.

(The same seems to apply to guest_allocator::default_free_impl which should probably be unsafe extern "C" fn but since this is called only from the host runtime, there should be no visible ramifications of this except for the declaration of hostcall_init_mm.)

I am also curious why Vec::into_boxed_slice and Box::into_raw were used instead of std::alloc::alloc? The only difference in effect seems to be that the current approach makes the std::alloc::Layout used, implicitly that of Vec<u8>, i.e. using a alignment of std::mem::align_of::<u8>(), instead of enforcing a fixed minimal alignment suitable for all primitive types as e.g. a malloc implementation on the host would. If the host runtime assumes alignment of returned memory to have suitable alignment for all primitive types like a libc malloc would, then this might even introduce crashes on architectures where unaligned memory access is not allowed.

pchickey commented 5 years ago

Thanks for the excellent bug report! I agree, guest_allocator::free should be unsafe.

I'm not sure why we're not using the std::alloc::alloc calls - personally I was not aware of them until now.

We haven't been pushing new changes to Terrarium for a bit because we've been too busy working on other stuff, but we'll try to get this fixed soon.

adamreichold commented 5 years ago

I'm not sure why we're not using the std::alloc::alloc calls - personally I was not aware of them until now.

They are what is used in the implementation of Vec (or rather RawVec). In this particular case, it could also improve performance as vec![0u8; size] might have a capacity strictly larger than size (as it is optimizied for armortized cost extension) which needs to be shed by Vec::into_boxed_slice (re)allocating the buffer twice in effect.

We haven't been pushing new changes to Terrarium for a bit because we've been too busy working on other stuff, but we'll try to get this fixed soon.

I think issues like this do not invalidate the approach at all and are easily fixed (as the host calls will come with unsafe blocks in any case). Personally, I am looking forward to seeing where the highly insightful Terrarium project is going.

acfoltzer commented 5 years ago

Yeah, I think the reason is far more mundane: All the alloc stuff got stabilized very soon before we got rolling on Terrarium, so we probably just weren't aware it was available yet.

I really appreciate you reaching out with such insightful comments :heart: