Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Impl traits don't have enough trait bounds #1515

Open
felinira opened this issue Sep 23, 2024 · 1 comment
Open

[BUG] Impl traits don't have enough trait bounds #1515

felinira opened this issue Sep 23, 2024 · 1 comment
Labels
bug Something isn't working

Comments

@felinira
Copy link
Contributor

Bug description

XyzImpl traits don't actually check whether they are implemented on classes that derive / implement the specific class. The Impl traits themselves only provide implementations, so usually they would be fine to implement wherever. However, almost all bindings also offer to call parent implementations, or other ffi methods as part of some Ext traits.

In practise this means that you can call unrelated methods on any object, thus being able to cause UB.

Examples:

  • Implement gtk::WidgetImpl on any gobject subclass and get access to things like set_css_name without the object actually having to be derived from GtkWidget. This causes a runtime critical with UB afterwards.
  • Implement gio::InitableImpl on a gobject subclass that doesn't implement gio::Initable and call parent_init. This panics here:
    None => unreachable!("No parent interfaces"),

There are probably more ways to make this lead to more serious and harder to detect behaviour.

Solution

Adding trait bounds like this should probably resolve all these issues:

pub trait WidgetImpl: ObjectSubclass where <Self as ObjectSubclass>::Type: IsA<Widget>

@felinira felinira added the bug Something isn't working label Sep 23, 2024
@sdroege
Copy link
Member

sdroege commented Sep 24, 2024

As discussed on Matrix yesterday, adding such trait bounds will require them to be added recursively in more places. E.g. ButtonImpl will need both IsA<Widget> and IsA<Button>.

Also it means that the glib::wrapper! usage must list all super-classes and can't just skip one. That part will probably break a bit of code.

This seems like the right thing to do though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants