apprt/gtk: fix double-free if quit action is used (#8068)

This fixes a double-free that Valgrind found when the quit action was
used (the keybinding to quit or the menu item). This fixes it in both
the gtk and gtk-ng apprts.

The issue stems from the fact that our quit action worked by traversing
the toplevels and destroying all windows. When all windows are
destroyed, GTK exits the main loop.

When fcitx is used as the input method editor (IME), it appears to hold
its own `gtk.Window` widget as a property (probably for the IME popup).
Unfortunately this does not react well to being destroyed externally and
triggers a double-free when the IME widget also tries to dispose itself.

I think this is probably a bug somewhere in the GTK IME widget because
it should be resilient to this kind of destruction. But, we can't
tolerate a double free in the mean time.

We can still quit by destroying only OUR windows (which cascades to
destroy everything else).
This commit is contained in:
Mitchell Hashimoto
2025-07-25 12:01:54 -07:00
committed by GitHub
2 changed files with 30 additions and 4 deletions

View File

@ -433,15 +433,26 @@ pub const Application = extern struct {
} }
fn quitNow(self: *Self) void { fn quitNow(self: *Self) void {
// Get all our windows and destroy them, forcing them to // Get all our windows and destroy them, forcing them to free.
// free their memory.
const list = gtk.Window.listToplevels(); const list = gtk.Window.listToplevels();
defer list.free(); defer list.free();
list.foreach(struct { list.foreach(struct {
fn callback(data: ?*anyopaque, _: ?*anyopaque) callconv(.c) void { fn callback(data: ?*anyopaque, _: ?*anyopaque) callconv(.c) void {
const ptr = data orelse return; const ptr = data orelse return;
const window: *gtk.Window = @ptrCast(@alignCast(ptr)); const window: *gtk.Window = @ptrCast(@alignCast(ptr));
window.destroy();
// We only want to destroy our windows. These windows own
// every other type of window that is possible so this will
// trigger a proper shutdown sequence.
//
// We previously just destroyed ALL windows but this leads to
// a double-free with the fcitx ime, because it has a nested
// gtk.Window as a property that we don't own and it later
// tries to free on its own. I think this is probably a bug in
// the fcitx ime widget but still, we don't want a double free!
if (gobject.ext.isA(window, Window)) {
window.destroy();
}
} }
}.callback, null); }.callback, null);

View File

@ -1556,7 +1556,22 @@ pub fn quitNow(self: *App) void {
fn callback(data: ?*anyopaque, _: ?*anyopaque) callconv(.c) void { fn callback(data: ?*anyopaque, _: ?*anyopaque) callconv(.c) void {
const ptr = data orelse return; const ptr = data orelse return;
const window: *gtk.Window = @ptrCast(@alignCast(ptr)); const window: *gtk.Window = @ptrCast(@alignCast(ptr));
window.destroy();
// We only want to destroy our windows. These windows own
// every other type of window that is possible so this will
// trigger a proper shutdown sequence.
//
// We previously just destroyed ALL windows but this leads to
// a double-free with the fcitx ime, because it has a nested
// gtk.Window as a property that we don't own and it later
// tries to free on its own. I think this is probably a bug in
// the fcitx ime widget but still, we don't want a double free!
//
// Since we don't use gobject directly we can't check class,
// so we use a heuristic based on CSS class.
if (window.as(gtk.Widget).hasCssClass("terminal-window") != 0) {
window.destroy();
}
} }
}.callback, null); }.callback, null);