Subscriptions and shared memory produce UB when moved

[Woyten]

From my understanding, the following code should turn on an LED but it doesn't:

let count_cell = Cell::new(0);
tock::syscalls::yieldk_for(|| {
    let count = count_cell.get();
    count_cell.set(count + 1);
    count > 0
});
unsafe { tock::syscalls::command(2, 3, 0, 0) }; // Toggle first LED

There are no kernel panics and the yieldk loop seems to be inactive...

Any idea what could be the problem? Did I make a mistake?

[torfmaster]

I'm not 100% sure whether I understand the example correctly. For me 'count>0' evaluates to 'false' (in the first run). Hence, at first sight the led never turning on makes sense to me, as there is no callback registered.

[brghena]

Moreover, you need an event to wake you from a yield. I don't know if the body of yieldk_for even executes the first time if there hasn't been an event.

[Woyten]

Hmm... I am pretty sure there is some sort of problem but maybe not in the yield function.

@brghena Maybe you could have a look at https://github.com/torfmaster/libtock-rs/blob/feature/unsafe-cleanup/src/console.rs? When I write to the console, the program gets stuck in line 90. First, I suspected that the callback in line 54 is not called but I verified that this is the case. Besides that, I tried the following things:

• Replace Cell<bool> by bool with volatile reads/writes
• Query is_written() inside of yieldk_for
• Query is_written() after replacing the yieldk_for by a 500ms wait

The value for is_written() always returned false although line 54 gets executed... Any ideas?

By the way, yieldk_for evaluates the closure first and if the closure returns false it yields and loops.

[Woyten]

@brghena Before you waste your time: I am pretty sure that the subscriptions are broken. Let me check that first...

[Woyten]

Okay, I found the bug. It's not in yieldk_for but in the types CallbackSubscription and SharedMemory. They lead to UB when a value is moved on the stack.

I am trying to solve that problem by making syscalls::subscribe consume &mut CB instead of CB (syscalls::allow_new accordingly) but this turns out to be quite challenging. If you know of some bleeding edge Rust feature that can solve that problem, please let me know. I have heard that there is some stack pinning feature under consideration but I am not aware of the current state.

[alevy]

I believe the problem is that in subscribe, the callback value is being semantically moved, then a raw pointer to it is unsafely being converted to a temporary reference. So at the end of the subscribe function, the value will be dropped.

Instead, the callback value needs to be (logically) stored away somewhere, e.g., by calling mem::forget, or actually storing it somewhere... Conceptually, we want to be moving the callback into the kernel...

[Woyten]

The problem is not that a CallbackSubscription<CB> can be dropped: The Drop implementation will unregister the contained callback CB, so there are no leaks.

Instead, the problem is that the current implementation allows CallbackSubscription<CB> to be moved on the stack. This must not be possible as the contained callback is borrowed by the kernel. Rust just doesn't know that.

The correct way would be to consume &'a mut CB instead of CB in subscribe and return a CallbackSubscription<'a> (with a phantom lifetime). In this way, Rust knows that the &'a mut CB must not be touched as long as the CallbackSubscription<'a> is alive.

Of course, this has an impact on the libtock API (which I consider far from stable anyway). The new API could look like this:

let callback = ButtonsCallback::new(|a, b| {
	/* Callback code here */
});
let buttons = Buttons::with_callback(&mut callback);

Being a bit more verbose, unfortunately, a change of this kind is unavoidable IMO.

My feeling is that we are very close to having a safe subscription/allow API for libtock-rs. I consider this as an important milestone as this enables us to remove almost all unsafe code in libtock-rs.