Immutable global state, but initialized by both Rust and FFI

code review
1

Hello!

This topic has a bit of exposition because I'm inter-oping with as somewhat strange internal C library that has the following API:

struct Context {...};
void start_workers(struct Context* global_ctx, void (*worker)());

As a user of the library, you're supposed to

  1. Allocate the context globally
  2. Fill some initialization data in the context
  3. Give it to start_workers(), which will put more data in the context
  4. And then you need to access the context from the worker functions (which you do via global variable).

Usage example:

static struct Context g_ctx;
void worker() {
  int x = g_ctx.things;
  // ...
}
int main() {
  g_ctx.stuff = get_stuff();
  start_workers(&g_ctx, worker);
  // ...
}

I want to use this from Rust and it turned out tricky because:

  1. This struct needs to be available to workers functions, so it needs to be global.
  2. This struct needs to be filled by main() before it's passed to start_workers() (so it needs some mutability).
  3. This struct is also mutated by start_workers() in C code.
  4. start_workers() creates threads which may call worker() even before start_workers() returns.

Note that:

  1. The context is updated by start_workers() before threads are created (so threads have a happens-before relationship with the latest context, no data race there)
  2. The workers don't modify the context.

I first reached out to static mut, but then I saw that it's very deprecated.

Therefore I came up with the following contraption based on UnsafeCell:

// Wrap UnsafeCell so that we can declare it Sync. I know
// SyncUnsafeCell exists but it's not in stable.
struct ContextWrapper(pub UnsafeCell<ffi::Context>);
// We mutate context only before threads are created. Afterwards it's
// read-only. ffi::Context is Sync. Therefore the wrapper is Sync too.
unsafe impl Sync for ContextWrapper {}

static CTX: ContextWrapper = ContextWrapper(UnsafeCell::new(ffi::Context {
    inited_by_caller: 0,
    inited_by_start: 0,
}));

fn main() {
    unsafe {
        {
            // Initialize the context. It's safe to take a mutable reference because
            // no threads are running at this point and no other references exist.
            let ctx = &mut *CTX.0.get();
            ctx.inited_by_caller = 123 /* some dynamic computation */;
        }

        // This will mutate the context, but only before worker threads
        // creation.
        ffi::start_workers(CTX.0.get(), worker_wrapper as _);

        unsafe extern "C" fn worker_wrapper() {
            // Since threads are running, context is read-only from now on. Access it
            // concurrently from multiple threads:
            let ctx = &*CTX.0.get();
            worker(ctx);
        }
    }
}

fn worker(ctx: &ffi::Context) {
    println!("yay {}", ctx.inited_by_start + ctx.inited_by_caller);
}

(Full code in Playground)

I would appreciate your review :slight_smile:

2

Isn't this just LazyLock (or OnceCell if you are using older Rust versions)?

1 Like
3

I tried modifying the Playground to use LazyLock. The LazyLock::force call is what initialises the context and starts workers.

1 Like
4

Thanks for your help!
I wasn't aware of LazyLock, good to add it to my toolbox!

Do I understand correctly that LazyLock will prevent worker threads from accessing CTX until start_workers() returns?

Because ffi::start_worker() takes a while to return (it does a bunch other work after spawning threads) I wouldn't like to block the workers from progressing till then. (In the most extreme case, we can image that we have ffi::start_workers_and_join(), which doesn't return till the workers are done).
Are there any ways manage such context handover with LazyLock or maybe other tools?

5

If you want the initialization to still mutate the state while the workers are already accessing it: don't. That's the very definition of a data race, and it's UB.

6

The implementation of ffi::start_workers() is something like:

void start_workers(struct Context* global_ctx, void (*worker)()) {
  // 1. Mutate global_ctx
  // 2. (global_ctx is not mutated anymore) Create threads.
  // 3. Do other IO stuff that's doesn't mutate global_ctx.
}

So when the workers are running and using the context it's not going to be mutated anymore (even though ffi::start_workers() didn't return yet). The reason I'm trying to allow the worker to access global_ctx before start_workers returns is because I don't want them to wait for step 3.

Therefore I don't think there's an inherent data race here, the question is just how to best express this pretty wierd ctx handoff in Rust and avoid UB.

(BTW I might also end up patching the C library to expose a saner FFI, but I'm still curious whether the code I wrote above is sound given this implementation of start_worker().)

7

It's not sound if main is called again (and there isn't an analysis to ensure that doesn't happen), so that at least would be a program-wide unsafe invariant you'd have to uphold.[1]

I think splitting it into complete_context and start_workers would be a better approach.

  1. Incidentally, the boundary outside of which you're considering soundness is unclear. Body of fn worker maybe? ↩︎

2 Likes
8

EDIT: Originally I talked about marking main unsafe, but even though it compiled it didn't actually invoke main. So I edited to post to use Once.

Right, I haven't considered this. Then I guess I need Once, like:

static CREATE_WORKERS: Once = Once::new();

fn main() {
    CREATE_WORKERS.call_once(|| unsafe {create_workers();});
}

unsafe fn create_workers() {
   /// The init from before
}

(Playground)

  • I originally considered the boundary to be "everything not unsafe", but you showed why it's not true (main can be called again)
  • In the latest playground I'm still considering the boundary to be "everything not unsafe" :slight_smile: But if I'm not using the right terms here or saying something that doesn't make sense please let me know

Agree, after this discussion I think I'll split it so that it's usable with LazyLock.
But if you or anyone else see any other safety issues with the latest playground, please point them out too so that I can learn

9

Well, the apparent safety condition on create_workers is "don't call me if I have been called before", so why not just make its body the code protected by Once instead? And then, since you call it in main, calling it again/from anywhere else will just be a no-op...[1] so why expose it outside of main at all?

Elaborating a bit on your version...

// # Safety
// This function must only be called once
unsafe fn create_workers() {

That still requires the consumer to do a global analysis to make sure it's only called once. In this case the analysis isn't too hard at least -- main calls it, so no one else should call it.

Unsatisfiable requirements on an unsafe fn are technically sound,[2] but not very useful.

  1. or recursive invocation of Once ↩︎

  2. safe code can't call it ↩︎

1 Like
10

Thanks, it makes sense to encapsulate this entire flow (including the global CTX) in main().

11

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.