jsha
commented
2 years ago Should I add unsafe to the function definition
No, and I don't think you can, since Debug is not an unsafe trait. Instead we should try to constraint the unsafe use inside this function as much as we can.
First, let's wrap the unsafe block around the one place that actually needs it: slice::from_raw_parts(...).
from_raw_parts is unsafe, so let's figure out what constraints we need to follow to use it soundly: https://doc.rust-lang.org/nightly/std/slice/fn.from_raw_parts.html#safety
Behavior is undefined if any of the following conditions are violated:
- data must be valid for reads for len * mem::size_of::
() many bytes, and it must be properly aligned. This means in particular:
We get "properly aligned" for free. This is a u8 and u8's have align(1).
The entire memory range of this slice must be contained within a single allocated object! Slices can never span across multiple allocated objects. See below for an example incorrectly not taking this into account.
This is satisfied for rustls_str's we produce internally, because we always get them from a slice. If we accept rustls_strs from C code*, we should document the fact that they must point to a single allocated object. It would be pretty bizarre to not do that, but we might as well pass along the requirement, in the name of precision.
*I checked briefly and AFAICT we don't currently do this, though I thought we did at one point.
data must be non-null and aligned even for zero-length slices. One reason for this is that enum layout optimizations may rely on references (including slices of any length) being aligned and non-null to distinguish them from other data. You can obtain a pointer that is usable as data for zero-length slices using NonNull::dangling().
We can check for NULL pretty easily; might as well do that. Though we could also document here "we only ever handle rustls_str that were generated internally, and those are always non-NULL."
data must point to len consecutive properly initialized values of type T.
This one is trickier than it sounds! Similar to the above, if we only ever handle rustls_str generated internally, this is true because the original &str must have been initialized. If we accept rustls_str from C, we would have to document "the pointed-to data must be initialized" (one of those things that gets assumed in a lot of places, but good to be explicit).
The memory referenced by the returned slice must not be mutated for the duration of lifetime 'a, except inside an UnsafeCell.
The total size len * mem::size_of::
() of the slice must be no larger than isize::MAX. See the safety documentation of pointer::offset.
Similar to the above.
So, long story short: When we're taking an unsafe function and wrapping it in a safe function, we should explain in a comment why we think it's sound to do so. Although, having said that, I realize that we use unsafe blocks throughout rustls-ffi without documenting the reasoning every single time, but instead relying on conventions.
kevinburke
This makes it easier to troubleshoot the contents of one of these strings.
Fixes #239.