gtk-rs-core: [BUG] AppInfoExt::executable can return unexpected NULL

Bug description

On my system (Fedora 37, gio 0.17.4), the following demonstrates a panic from AppInfoExt::executable:

use gio::{traits::AppInfoExt, AppInfo};

fn main() {
    let app_info_all = AppInfo::all();

    for info in &app_info_all {
        let result = std::panic::catch_unwind(|| info.executable());
        if result.is_err() {
            println!("{}\t{}", info.id().unwrap_or("NO_ID".into()), info.name());
        }
    }
}

This also identifies the offending app as qemu.desktop QEMU.

The backtrace points to this debug assert as a result of failing to null-check here.

Clearly (although the docs don’t make it obvious), upstream gio does not guarantee a non-null response. It seems likely that other strings returned from this trait might have similar corner cases, so to guarantee soundness I would recommend null-checking them all. The choice is then, do we make the breaking change of the return types to Option<_>, or hotfix to some sentinel value (the empty string, an empty path, etc.)?

About this issue

Original URL
State: open
Created a year ago
Comments: 16 (11 by maintainers)

Most upvoted comments

More importantly, the rust compiler will make assumptions on NonNull values not being zero, so the potential effects are much more than dereferencing null.

As does any C compiler, for that matter. A pointer that is dereferenced anywhere can’t be NULL.

Sorry if what I said felt rude to you, but I don’t see how patching around this without going to the root of the problem is any useful. If you don’t want / can’t look at the C functions then that’s fine, but then this just has to wait until someone has the time to do so.

If we would just randomly patch things here we would have a lot of churn with fixes getting reverted all the time because they were actually not completely correct. You seem to have a different approach and that’s fine.

I don’t really have time to look into that in detail right now and this discussion already took more time than necessary so let’s keep it at that.

sdroege on Apr 8, 2023