gio::Application::get_default() is unsound

[sdroege]

It allows retrieving the application from any thread, not just the main thread (or more general the thread where the application was created). gio::Application is not thread-safe, so this is a problem.

[alatiera]

Would it be possible to add a check that make it so it only returns if called from the thread gtk was initialized from?

I mean, would this be proper workaround/fix?

[sdroege]

We don't have any gtk in gio :) a fix, half at least, would be to check if the default main context is owned by this thread. There's a function for that

[alatiera]

We don't have any gtk in gio

ah, of course, meant to say GMainContext

[sdroege]

MainContext::is_owner() is what I was thinking of. This will return false while application.run() is not running though.

[sdroege]

We can probably add into qdata of the Application the current thread id whenever we see it first in Rust, and later always check it and if it differs we panic or return None or similar.

[jf2048]

It seems this can never be completely safe because g_application_set_default is not thread safe, and that gets called internally by GApplication implementations. I guess what that means is, a GApplication can only be referenced in the main thread, but we don't have any way of knowing what the main thread really is, getting the current thread id is just a guess.

It could be made a little more robust by checking if we can acquire the main context before storing the qdata, but that still isn't completely safe in all cases because it only checks for a running application... I wonder if set_default and get_default should just be unsafe functions, but that also won't completely solve it because another GApplication could be created from another thread which is also unsafe

[sdroege]

Yeah the problem with making get_default() unsafe is that it's a very useful API to have in GTK applications and requiring those to use unsafe code is not ideal (set_default() is fine IMHO, who needs that anyway...)

[felinira]

I think I have a way forward:

• Mark it as unsafe in gio
• Add a safe wrapper to gtk::Application that checks for main thread and panics otherwise

[jf2048]

That will not fully make it sound but will probably correct most of the issues people have. The wrapper macro will need some modifications here to do the thread checking so we may need to see https://github.com/gtk-rs/gtk-rs-core/issues/916 for that

[sdroege]

I think the easiest would be to implement default() manually for now and implement the same assertion that is also used in e.g. glib::MainContext::spawn_local(). That should cover the easiest mistakes at least.

More correct would be to forbid construction of a gio::Application from another thread but that requires glib::wrapper! macro changes as @jf2048 says, and not very easy ones.

Does someone want to do the easy part for now?

[jf2048]

I think the easiest would be to implement default() manually for now and implement the same assertion that is also used in e.g. glib::MainContext::spawn_local(). That should cover the easiest mistakes at least.

How can we do that, since Application does not reference a MainContext? I think any solution will need to modify the wrapper and do the qdata solution described above.

[sdroege]

Can you actually use an Application on a non-default main context?