Safety problems and links with ctru-rs

[Meziu]

Citro3D is designed to be mostly used alone (an exception is the use of Citro2D, which is based on the aforementioned). This is a problem because the original implementation makes many assumptions which we cannot prove in a normal environment, but that we need to to ensure memory safety and correct execution.

For example, we cannot have the use of RawFramebuffers without taking the gfx::Screen‘s write lock. Simply passing immutable references and such is not safe, as the underlying C implementation could change the environment regardless.

We need to thoroughly examine Citro3D’s code and check for all of the functions that mutate the environment (like changing the framebuffer’s format, or even just displaying to the screen) and make appropriate changes to ensure a safe library.

Example for things to avoid: https://github.com/ian-h-chamberlain/citro3d-rs/blob/6ba85149d84aa3bcc879ec16ef79079dc411643f/citro3d/src/render.rs#L61

Because the choice of Screen target is permanent, but only a simple immutable reference is passed. Tracking internal changes would be impossible. The screen’s locked reference (RefMut) should be held by the struct.

[ian-h-chamberlain]

Hmm, nice catch, I think you're right that there is some potential for unsoundness here.

Maybe the API should change so that render_target_for_screen takes ownership of the RefMut instead (in the render::Target) and passes it through down to other APIs that need it?

I did have some trouble and revisited some of these APIs several times trying to get something that was reasonably easy to use without sacrificing safety, so I won't be surprised if they have to change several times more before we land on something decent. I'll see if I can spend some time this weekend to adjust this particular API at least.

Related question:

For example, we cannot have the use of RawFramebuffers without taking the gfx::Screen‘s write lock. Simply passing immutable references and such is not safe, as the underlying C implementation could change the environment regardless.

Does this indicate we're missing something in the RawFramebuffer (or the Screen) implementation? Maybe even screen.as_raw() should take an &mut self to be conservative so that e.g. the example you posted above would also require a mutable ref? From a quick search get_framebuffer_format seems like the only other API that doesn't already have a mutable reference to the screen when it's calling as_raw...

[Meziu]

Does this indicate we're missing something in the RawFramebuffer (or the Screen) implementation? Maybe even screen.as_raw() should take an &mut self to be conservative so that e.g. the example you posted above would also require a mutable ref? From a quick search get_framebuffer_format seems like the only other API that doesn't already have a mutable reference to the screen when it's calling as_raw...

get_framebuffer_format and screen.as_raw are just conversion methods to obtain the enum variants of those parameters. Those don't mean exclusivity or mutability, but only retrieve the current state of the screen in question.

The unsafe part is passing those paramenters to Citro3D (which are unsafe functions, so there is no intrinsic problem with it). The render::Target struct should hold the RefMut<&dyn Screen> like the ctru::Console does. This way the screen can't be managed by both Citro3D and Console, or by 2 Citro3D istances etc.

Small note: The point is that it's all up to us. Rust's borrow check has no idea that those functions call global states and services, so it lets us do whatever we want. To ensure memory safety within citro3d-rs we need to take into account all of the background work done by Citro3D and judge whether we need to hold exclusive references or not. In this case, since the render::Target changes stuff like the framebuffer format, the framebuffer data itself and other things, we need to hold inside of it a mutable reference to the linked Screen.