search

gquintard / varnish-rs

Rust bindings for the Varnish Cache project
BSD 3-Clause "New" or "Revised" License
22 stars 4 forks source link

Fix the `workspape::alloc` API to be safe #107

Open nyurik opened 4 days ago

nyurik commented 4 days ago

The Workspace::alloc returns &'a mut [u8]. As a result, let p = ws.alloc(vsa_suckaddr_len)?.as_mut_ptr().cast::<suckaddr>(); generates a clippy::cast_ptr_alignment warning.

This is unsafe, and we should rethink the workspape::alloc API

gquintard commented 4 days ago

workspaces allocate with alignment, I think that's safe? https://github.com/varnishcache/varnish-cache/blob/master/include/vdef.h#L152 https://github.com/varnishcache/varnish-cache/blob/master/bin/varnishd/cache/cache_ws.c#L164

nyurik commented 3 days ago

You are correct - the alloc function is safe by itself - the issue is that the API from it returns a &[u8] - a slice pointing to individual bytes, which has to be unsafely cast to some other object - which makes the compiler think it is not safe because objects are bigger than u8, thus alignment questions. Moreover, we expose this API to the users as is. Instead, I think we should have a type-safe API, something like alloc<T>() -> OurObject<T> - so the proper alignment is never even a question, and moreover, the result is easy to use.