Unsoundness in ApplicationContext upgrade_ref

[SafariMonkey]

The following may come off as strong. I want to mention up front that I appreciate the work done on libremarkable greatly, and that this is absolutely not intended as a criticism of the project. I have merely spotted an issue which I believe would need addressing in order for the memory safety guarantees of Rust to continue to be useful to us.

In libremarkable, there is a function on the ApplicationContext called upgrade_ref. This function notes that it's probably bad practice, but asserts that its operation is safe because ApplicationContext will live forever. This is kinda valid reasoning for upgrading a reference to an immutable value that will live forever, but in the ways it's used, it's absolutely not safe. Specifically, it upgrades a &'a mut to a &'static mut, effectively annihilating any borrow checking as it asserts that the mutable borrow is valid forever, even in the face of other borrows. ApplicationContext is not an immutable value like a &'static str, it's more like a global state - and this state can currently have as many simultaneous mutable references to it as one likes. In my opinion, this is a problem.

I have attempted to resolve this by changing the signature from fn upgrade_ref(&mut self) -> &'static mut ApplicationContext<'static> to fn upgrade_ref(&mut self) -> &mut ApplicationContext<'static>, that is to say, changing it to return a reference with the elided lifetime rather than a static one. Unfortunately, while this is fine for all uses within libremarkable, uses outside of the core library, including examples, go ahead and pass mutable references to the ApplicationContext to as many threads as they please, while each of them assumes it has a unique reference. This is clearly undefined behaviour.

Now, whether this issue is of concern to library users is another question. I am not aware of any instances of memory corruption arising from the current implementation, so maybe the current implementation has less unsoundness than it seems, or maybe we're lucky. It may be that keeping the existing applications running is more important than addressing this undefined behaviour. However, there may be a way to resolve this without an architecture-breaking change: if all parts of the ApplicationContext (including Framebuffer) can be made interior mutable (e.g. atomics and mutexes rather than &mut), upgrade_ref could be largely superceded by a version that returns a shared reference, and all the methods can take it by shared reference. I have given this idea a poke, but I haven't spent much time attempting an implementation.

The other possibility I can think of is a major architecture-breaking change based on granular resource delegation. (I have been playing with this idea since 2019, but it happens to also maybe solve this issue - I only noticed the soundness issues this month. As such, the relationship to the problem is incidental.) My idea is essentially traits for receiving input events and drawing on screen regions, which are implemented by structs that hold mutable access to said screen regions. The idea would be that if you want to delegate control of, say, the top bar of the screen to a thread, then you split your canvas into two accessor structs (above region and below region) that both implement the drawing traits but have mutually exclusive safety checks in place to prevent overdraw.

This idea is actually one I've explored a little a couple of times, first in 2019 with my f-canvas-traits branch. On that branch, only pixel writing is implemented by Framebuffer, and the rest is on an extension trait. This means that one can have a wrapper struct man-in-the-middle pixel draw calls to e.g. mask the output. This allowed me to easily implement patterned drawing (pattern, masking) in the demo, for example. A more directly applicable change was tried on my (poorly named) split-out-draw-text branch, which makes FramebufferIO itself a trait. I believe the intent was to make generic framebuffer and event IO traits that would allow the implementation of PC emulation layers for dev and/or automated tests. I was blocked at the time by two major factors: I couldn't find a library for pen input, and I realised that ApplicationContext was a behemoth of state that would require significant work to convert to a clean interface. Attempts at the latter were what led me to finding this issue.

Anyway, I'd like to know what everyone thinks. Is this a non-issue? Do you think something should be done? If so, what?

tl;dr: there are some soundness issues I think it's worth being aware of and maybe fixing. There's a boring fix that might work and a pet idea of mine might also be a solution. I want to know what people think.

[fenollp]

The idea would be that if you want to delegate control of, say, the top bar of the screen to a thread, then you split your canvas into two accessor structs (above region and below region) that both implement the drawing traits but have mutually exclusive safety checks in place to prevent overdraw.

I like this a lot (btw, how would buttons be handled? An API for registration that errors on conflicts?) but believe this should be worked on in a separate crate.

I don't have much to add here. I also noticed the unsoundness and I'm wondering if a global Mutex<ApplicationContext> would be preferable, but I guess this doesn't change anything wrt lifetimes.

[LinusCDE]

Do we want to do this for 0.6.0?

Not sure whether to consider this a bug or enhancement. Just overlooked it rn and will probably dig deeper in the evening.

[SafariMonkey]

Personally, I'm not particularly involved in this project currently. I just noticed the problem while I was attempting again to implement the above-mentioned panes concept and wanted to draw attention to it, since mutable aliasing is nasal demons levels of UB in Rust. I hope other contributors here can settle on a workable solution!