Impact of Static Variables on API Soundness

language design Unsafe Code Guidelines
1

I have a question regarding the policy for declaring API safety when it depends on the value of static variables. In the example below, the module FooSys contains a static variable PTR with internal mutability and two public APIs. Can the API do_critical_task be declared safe?

The function do_critical_task requires that PTR is either null or points to CONFIG. In the current module, this is guaranteed because PTR can only have these two states. However, future modifications to the module could introduce additional states or allow PTR to be changed, which could break this guarantee.

My specific questions are as follows:

  1. In the current implementation, I believe do_critical_task can be declared safe. Is this correct?
  2. If PTR remains private, but additional invalid states become possible, then do_critical_task cannot be declared safe, correct?
  3. If PTR is made public (but not to other crates), meaning that additional invalid states could be introduced from outside the module, can do_critical_task still be declared safe?
mod FooSys {
    use std::{ptr, sync::atomic::{AtomicPtr, Ordering}};

    /// Static atomic pointer pointing to system configuration
    static PTR: AtomicPtr<u32> = AtomicPtr::new(ptr::null_mut());

    /// Initialize the system and set PTR to point to a valid static resource
    pub fn initialize_system() {
        static CONFIG: u32 = 42;
        PTR.store(&CONFIG as *const u32 as *mut u32, Ordering::SeqCst);
    }

    pub fn do_critical_task() {
        let ptr = PTR.load(Ordering::SeqCst);
        if !ptr.is_null() {
            unsafe { read_config(ptr); }
        }
    }

    // Safety: `ptr` must point to a valid configuration.
    unsafe fn read_config(ptr: *mut u32) {
       ...
    }
}
1 Like
2

Yes.

If do_critical_task is crate-public and those additional invalid states are reachable from crate-public safe functions, then yes, do_critical_task cannot be safe.

If the same conditions as above hold, then no, do_critical_task cannot be safe. However, if one of those conditions do not hold, then it can be declared safe. It's just a matter of style (crates with multiple contributors like the standard library may prefer to declare it unsafe to avoid misunderstandings, but simple crates with a single author might not bother in order to keep call sites readable).

2 Likes
3

For what it’s worth I think the “what about the future” part is a distraction for this question. Normally, yes, I’d be all for looking beyond today to make a better, more stable library for the future. But unsafe functions are those where the caller must guarantee some condition the compiler can’t check for them. Today, your function has no preconditions from the perspective of an outside client. So what are they supposed to check?

Now consider that you add a new API that puts things in a bad state. (I’m intentionally being abstract because I don’t think involving a static is special; the pointer could just as easily be a member variable and you’d still be in this situation.) Now your original function has an uncheckable precondition, and it’s pretty easy to say it should be marked unsafe. That’d be a breaking change if it wasn’t before, because old callers won’t compile.

However, even that’s a red herring. Suppose you documented the precondition as “you must not have called foo() before calling critical()”. And then in the next release you add bar() that can also put things in a bad state. Now your precondition has changed, but (at least with the way unsafe works today) there’s no source break. I’d argue that this is still a semantically breaking change, because the client does not know they have to uphold the new precondition.

You could maybe argue that existing clients can’t be calling bar(), and thus no existing code is broken. That’s probably true for the time being. But without marking it as breaking, they won’t have a reason to see that the preconditions on critical() have changed. I mean, sure, you can read the release notes for non-breaking updates, but it’s not the same—I think a lot of times people would expect cargo update && cargo test to be sufficient.

So even if it’s not technically build-breaking, I think every change to a function’s unchecked preconditions should at least default to being considered breaking, and therefore the idea of designing an unsafe function to be futureproof is not worth it.

6 Likes
4

Also, instead of marking your current API functions as unsafe, what you can do is mark whatever new functions you add that could break the invariants as unsafe, e.g.:

static ORIGINAL: u32 = 0;
static PTR: AtomicPtr<u32> = AtomicPtr::new(&raw const ORIGINAL as *mut u32);

pub fn existing_function() -> u32 {
    // Safety: PTR is known to contain a valid pointer and we used Acquire/Release correctly
    unsafe {
        *PTR.load(Ordering::Acquire)
    }
}

/// Safety: you must not pass in an invalid `v`
pub unsafe fn new_function(v: *const u32) {
    PTR.store(v as *mut u32, Ordering::Release);
}
6 Likes