Should functions that depend upon specific values be made unsafe?

user_16858052 0
Viewed 59

I have a function that takes a usize equivalent to a pointer, and aligns it up to the next alignment point.

It doesn't require any unsafe as it's side effect free, but the alignment must be a power of two with this implementation. This means that if you use the function with bad parameters, you might get undefined behaviour later down the line. I can't check for this inside the function itself with assert! as it's supposed to be very fast.

/// Align the given address `addr` upwards to alignment `align`.
///
/// Unsafe as `align` must be a power of two.
unsafe fn align_next_unsafe(addr: usize, align: usize) -> usize {
    (addr + align - 1) & !(align - 1)
}

Currently, I've made this unsafe for the above reasons, but I'm not sure if that's best practice. Should I only define a function as unsafe if it has side effects? Or is this a valid time to require an unsafe block?

rust requirements unsafe parameters
1 Answers

I'll preface this by saying this is a fairly opinion-heavy answer, and represents a point of view, rather than "the truth".

Consider this code taken from the Vec docs:

let x = vec![1, 2, 4];
let x_ptr = x.as_ptr();

unsafe {
    for i in 0..x.len() {
        assert_eq!(*x_ptr.add(i), 1 << i);
    }
}

The function you're describing seems to have a similar safety profile to Vec::as_ptr. Vec::as_ptr is not unsafe, and does nothing particularly bad on its own; having an invalid *const T isn't bad until you dereference it. That's why dereferencing the raw pointer requires unsafe.

Similarly, I'd argue that align_next doesn't do anything particularly bad unless that value is then passed into some unsafe context. As with any question of unsafe, it's a tradeoff between safety/risk and ergonomics.

In Vec::as_ptr's case, the risk is relatively low; the stdlib has lots of eyes on it, and is well "battle-tested". Moreover, it is a single function with a single implementation.

If your align_next was a function on a trait, I'd be much more tempted to make it unsafe, since someone in the future could implement it badly, and you might have other code whose safety relies on a correct implementation of align_next.

However, in your case, I'd say the pattern is similar to Vec::as_ptr, and you should make sure that any functions that consume this value are marked unsafe if they can cause UB.

I'd also second Martin Gallagher's point about creating a Result returning variant and benchmarking (you could also try an Option<usize>-returning API to make use of null-pointer optimizations).

user_91867830
Related