rust3ds / ctru-rs

Rust wrapper for libctru
https://rust3ds.github.io/ctru-rs/
Other
122 stars 18 forks source link

Fixed duplicate services and constistency in naming #50

Closed Meziu closed 2 years ago

Meziu commented 2 years ago

Weirdly, we missed one of the biggest memory safety issues in this whole library: there may be more than one handle to a service at once, yet services would break if more than an instance existed at a time.

For example:

let gfx = Gfx::default();
let gfx2 = Gfx::default();

drop(gfx2);

// Now `gfx` is completely invalid and the program will crash when dropping it 
// since it'll try to double-free memory

Some services (like Gfx) completely crash the program, others simply stop working, but all are affected in an unexpected behaviour.

I wanted to keep the "own the service" behaviour of having a struct to hold them, because it fits nicely with Rust's drop mechanism, so the solution was to have an AtomicBool signaling the presence of the struct at runtime. This also comes with a new ctru::Error field to signal multiple instancing (we can add as many of those as we want since the Os field is separate).

This also comes with a different Gfx default impl, since using the trait wouldn't work anymore with the possible error handling (changed ::default() with ::init_default() which now returns a ctru::Result object). At last, I've added tests in all service modules to test for these new changes. I believe we should start adding them from now on, since testing is an absolute breeze.

Overall it's a gigantic PR with many breaking changes, but it is absolutely needed something like this system to keep the whole library memory safe. Hope crashes won't be a thing anymore in the future.

AzureMarker commented 2 years ago

Where is the double init problem documented? I haven't run across that before.

Meziu commented 2 years ago

Where is the double init problem documented? I haven't run across that before.

The issue isn’t with double initialisation, it’s with double ownership. As I showed in my snippet, one of the two Gfx instances could get dropped before the other, but the drop code closes the service anyways, making the other struct invalid (functions would either do nothing or crash, depending on the service). We also can’t solve it with an Rc since that would undo the control of mutability of the service’s state (Eg. keeping track of the current setup for the Gfx screens or RomFS mount).

ian-h-chamberlain commented 2 years ago

As an alternative design, I wonder if we could consider something like the singleton pattern laid out in the embedded book.

I think it would look basically like

#[derive(Default)]
struct Services {
    gfx: Option<Gfx>,
    hid: Option<Hid>,
    //...
}

static mut SERVICES: Services = Services::default();

Then a user could either request a given service (Gfx::try_init or something maybe?), which could initialize and either create or replace the global Option<Gfx>. This would perhaps require APIs like Gfx::force_init or something if the user wanted to replace the existing service, but it seems like that might be necessary to support such a use case anyway.

There would probably still need to be some kind of lock on the singleton, for thread safety, but since it's static it should be safe to hand out references to the underlying service handles (&'static Gfx - I think &mut would be okay here too?). We might want to avoid the take_serial() example they provide, since we wouldn't want to allow a user to call take_gfx() followed by gfx_init or something and end up with two "live" Gfx... but I think in principle this could work to ensure services are only initialized one at a time.

AzureMarker commented 2 years ago

That singleton pattern is interesting (I haven't seen it before), but since libctru already reference counts most of the services, I think we should keep that pattern.

Meziu commented 2 years ago

Fixed the nits and such. I believe the ServiceCloser idea would be better, but I won't be able to thoroughly test it right now.

Meziu commented 2 years ago

Oh well, trying to test things out made me realize an issue with the ServiceCloser idea: Mutexes aren't const. The idea of keeping the lock is good, but it's a problem to implement...

AzureMarker commented 2 years ago

We can use the Mutex from the spin crate like we do with RwLock in pthread-3ds (yielding instead of busy-looping).

AzureMarker commented 2 years ago

We may also be able to avoid any reference counting on our end for most services based on this: https://github.com/Meziu/ctru-rs/pull/50#discussion_r817347456

Meziu commented 2 years ago

We can use the Mutex from the spin crate like we do with RwLock in pthread-3ds (yielding instead of busy-looping).

I went with SyncLazy. I believe it's better because it only initializes when needed, and not in all programs.

Meziu commented 2 years ago

Ok, I changed most of everything. Now it uses Mutex, and all services have an abstract implementation of ServiceHandler (did it this way because the init functions had to be in the lock's lifetime too, and also because it was easy to expand). This is nowhere near final.

@AzureMarker there are many things you'll have to review, though one of the smallest is the presence of Error::ServiceAlreadyActive. With this implementation, it's used only by Gfx, so I removed the &str fields altogether (they were also really ugly).

Meziu commented 2 years ago

I reverted all services not needing the refcount (Soc doesn't have ref-count by itself, nor RomFS).

Meziu commented 2 years ago

Changed Soc's allow_multiple to false, because the init is handled with a parameter and we can't return an Ok without telling the user the change won't be made.