Is this unsound: extern "system" fn window_proc without catching unwinds?

[jbe]

Continuing the discussion from Passing callbacks to C: panic!:

@alice

Generally, if the Rust code can panic, you must use catch_unwind to stop the panic from unwinding into C code.

Does this also hold in this case:

pub(crate) unsafe extern "system" fn window_proc(
    hwnd: HWND,
    msg: u32,
    w_param: WPARAM,
    l_param: LPARAM,
) -> LRESULT {
    /* … */
}

/* … */

    let wnd = WNDCLASSW {
        /* … */
        lpfnWndProc: Some(window_proc),
        /* … */
    };
    if winuser::RegisterClassW(&wnd) == 0 {
        /* … */
    }

See source of tray-item crate. Am I right that the implementation of tray-item is unsound in that matter?

[CAD97]

Unwinding out of extern fn is an interesting edge case; it's currently unsound, but sometime in the future it will be changed to a guaranteed sound abort.

However, on a technicality, note that it's only really a "soundness" issue if you're calling downstream code that you don't control. If you're only calling upstream code which is "known" to not panic, it's "merely" a resiliency issue that a bug causing a panic will lead to potential UB.

In actuality, the current behavior is that Rust panics use the same mechanism as C++ exceptions, so the behavior will be the same as if C++ unwinds into the OS here. I think this is a consistent crash.

[jbe]

@CAD97

Unwinding out of extern fn is an interesting edge case; it's currently unsound, but sometime in the future it will be changed to a guaranteed sound abort.

That's also what I understood from the other thread, but I wasn't sure if this also applies to a function that's invoked by Windows somehow (and which doesn't have any point to "unwind to", I guess?). Or maybe it does ultimately get invoked by Rust. I'm not sure if I correctly understand the mechanism here.

Anyway, I understand you that panicking here is unsound but should be changed in future to be sound.

@CAD97

However, on a technicality, note that it's only really a "soundness" issue if you're calling downstream code that you don't control. If you're only calling upstream code which is "known" to not panic, it's "merely" a resiliency issue that a bug causing a panic will lead to potential UB.

Yes, I understand. In the given example, we have:

WININFO_STASH.with(|stash| {
    let stash = stash.borrow();
    let stash = stash.as_ref();
    if let Some(stash) = stash {
        let menu_id = winuser::GetMenuItemID(stash.info.hmenu, w_param as i32) as i32;
        if menu_id != -1 {
            stash.tx.send(WindowsTrayEvent(menu_id as u32)).ok();
        }
    }
});

If I take it right, then stash.tx is a std::sync::mspc::Sender, which does potentially allocate when using the send method and thus can panic. So do I understand right that tray-item is unsound here (with current Rust)?

@CAD97

In actuality, the current behavior is that Rust panics use the same mechanism as C++ exceptions, so the behavior will be the same as if C++ unwinds into the OS here. […]

From the other thread (which was related to extern "C" and unrolling back to Rust code):

@LegionMammal978

Right now, unwinding through extern "C" is nominally UB but in practice unwinds through C frames as if it were longjmp(), and C++ frames as if it were an exception. If you add #![feature(c_unwind)], then extern "C" will add an abort-on-unwind shim to the function. The plan is to make the latter behavior the default once c_unwind is stabilized, but that may take many months yet. There have been some soft promises made about the former behavior on GitHub and on Zulip, but I haven't been able to track down my old list of links.

What I don't really understand is who or what will call the window_proc function, i.e. will it be from some Rust context and unwind "through" C/C++ frames back to Rust? But maybe it doesn't matter as it seems to be UB anyway for now.

@CAD97

[…] I think this is a consistent crash.

What do you mean with "consistent crash"? A "clean" segmentation fault or abort?