From d2e67689ea0d5ec986b66936188e0557c1f04858 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Ekl=C3=B6f?= Date: Mon, 5 Sep 2022 19:23:40 +0200 Subject: [PATCH] =?UTF-8?q?terminal:=20don=E2=80=99t=20unref=20a=20not-yet?= =?UTF-8?q?-referenced=20key-binding=20set?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Key-binding sets are bound to a seat/configuration pair. The conf reference is done when a new terminal instance is created. When that same terminal instance is destroyed, the key binding set is unref:ed. If the terminal instance is destroyed *before* the key binding set has been referenced, we’ll still unref it. This creates an imbalance. In particular, when the there is exactly one other terminal instance referencing that same key binding set, that terminal instance will trigger a foot server crash as soon as it receives a key press/release event. This happens because the next-to-last terminal instance brought the reference count of the binding set down to 0, causing it to be free:d. Thus, we *must* reference the binding set *before* we can error out (when instantiating a new terminal instance). At this point, we don’t yet have a valid terminal instance. But, that’s ok, because all the key_binding_new_for_term() did with the terminal instance was get the "struct wayland" and "struct config" pointers. So, rename the function and simply pass these pointers explicitly. Similarly, change key_binding_for() to take a "struct config" pointer, rather than a "struct terminal" pointer. Also rename key_binding_unref_term() -> key_binding_unref(). --- CHANGELOG.md | 6 ++++++ input.c | 4 ++-- key-binding.c | 22 +++++++--------------- key-binding.h | 14 ++++++++------ pgo/pgo.c | 9 +++++---- terminal.c | 11 +++++++---- 6 files changed, 35 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 15b0e55c..b54016de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -60,6 +60,12 @@ ### Deprecated ### Removed ### Fixed + +* Crash in `foot --server` on key press, after another `footclient` + has terminated very early (for example, by trying to launch a + non-existing shell/client). + + ### Security ### Contributors diff --git a/input.c b/input.c index 50336679..3aa8b004 100644 --- a/input.c +++ b/input.c @@ -1406,7 +1406,7 @@ key_press_release(struct seat *seat, struct terminal *term, uint32_t serial, seat->kbd.xkb_keymap, key, layout_idx, 0, &raw_syms); const struct key_binding_set *bindings = key_binding_for( - seat->wayl->key_binding_manager, term, seat); + seat->wayl->key_binding_manager, term->conf, seat); xassert(bindings != NULL); if (pressed) { @@ -2335,7 +2335,7 @@ wl_pointer_button(void *data, struct wl_pointer *wl_pointer, /* Seat has keyboard - use mouse bindings *with* modifiers */ const struct key_binding_set *bindings = key_binding_for( - wayl->key_binding_manager, term, seat); + wayl->key_binding_manager, term->conf, seat); xassert(bindings != NULL); xkb_mod_mask_t mods; diff --git a/key-binding.c b/key-binding.c index 2135abbc..1876a885 100644 --- a/key-binding.c +++ b/key-binding.c @@ -80,17 +80,14 @@ key_binding_new_for_seat(struct key_binding_manager *mgr, } void -key_binding_new_for_term(struct key_binding_manager *mgr, - const struct terminal *term) +key_binding_new_for_conf(struct key_binding_manager *mgr, + const struct wayland *wayl, const struct config *conf) { - const struct config *conf = term->conf; - const struct wayland *wayl = term->wl; - tll_foreach(wayl->seats, it) { struct seat *seat = &it->item; struct key_set *existing = - (struct key_set *)key_binding_for(mgr, term, seat); + (struct key_set *)key_binding_for(mgr, conf, seat); if (existing != NULL) { existing->conf_ref_count++; @@ -116,21 +113,19 @@ key_binding_new_for_term(struct key_binding_manager *mgr, /* Chances are high this set will be requested next */ mgr->last_used_set = &tll_back(mgr->binding_sets); - LOG_DBG("new (term): set=%p, seat=%p, conf=%p, ref-count=1", + LOG_DBG("new (conf): set=%p, seat=%p, conf=%p, ref-count=1", (void *)&tll_back(mgr->binding_sets), (void *)set.seat, (void *)set.conf); } - LOG_DBG("new (term): total number of sets: %zu", + LOG_DBG("new (conf): total number of sets: %zu", tll_length(mgr->binding_sets)); } struct key_binding_set * NOINLINE -key_binding_for(struct key_binding_manager *mgr, const struct terminal *term, +key_binding_for(struct key_binding_manager *mgr, const struct config *conf, const struct seat *seat) { - const struct config *conf = term->conf; - struct key_set *last_used = mgr->last_used_set; if (last_used != NULL && last_used->conf == conf && @@ -192,11 +187,8 @@ key_binding_remove_seat(struct key_binding_manager *mgr, } void -key_binding_unref_term(struct key_binding_manager *mgr, - const struct terminal *term) +key_binding_unref(struct key_binding_manager *mgr, const struct config *conf) { - const struct config *conf = term->conf; - tll_foreach(mgr->binding_sets, it) { struct key_set *set = &it->item; diff --git a/key-binding.h b/key-binding.h index 448500c1..f607644f 100644 --- a/key-binding.h +++ b/key-binding.h @@ -110,6 +110,7 @@ typedef tll(struct key_binding) key_binding_list_t; struct terminal; struct seat; +struct wayland; struct key_binding_set { key_binding_list_t key; @@ -127,20 +128,21 @@ void key_binding_manager_destroy(struct key_binding_manager *mgr); void key_binding_new_for_seat( struct key_binding_manager *mgr, const struct seat *seat); -void key_binding_new_for_term( - struct key_binding_manager *mgr, const struct terminal *term); +void key_binding_new_for_conf( + struct key_binding_manager *mgr, const struct wayland *wayl, + const struct config *conf); -/* Returns the set of key bindings associated with this seat/term pair */ +/* Returns the set of key bindings associated with this seat/conf pair */ struct key_binding_set *key_binding_for( - struct key_binding_manager *mgr, const struct terminal *term, + struct key_binding_manager *mgr, const struct config *conf, const struct seat *seat); /* Remove all key bindings tied to the specified seat */ void key_binding_remove_seat( struct key_binding_manager *mgr, const struct seat *seat); -void key_binding_unref_term( - struct key_binding_manager *mgr, const struct terminal *term); +void key_binding_unref( + struct key_binding_manager *mgr, const struct config *conf); void key_binding_load_keymap( struct key_binding_manager *mgr, const struct seat *seat); diff --git a/pgo/pgo.c b/pgo/pgo.c index 3073c27c..7f6f758b 100644 --- a/pgo/pgo.c +++ b/pgo/pgo.c @@ -178,15 +178,16 @@ static bool kbd_initialized = false; struct key_binding_set * key_binding_for( - struct key_binding_manager *mgr, const struct terminal *term, + struct key_binding_manager *mgr, const struct config *conf, const struct seat *seat) { return &kbd; } void -key_binding_new_for_term( - struct key_binding_manager *mgr, const struct terminal *term) +key_binding_new_for_conf( + struct key_binding_manager *mgr, const struct wayland *wayl, + const struct config *conf) { if (!kbd_initialized) { kbd_initialized = true; @@ -201,7 +202,7 @@ key_binding_new_for_term( } void -key_binding_unref_term(struct key_binding_manager *mgr, const struct terminal *term) +key_binding_unref(struct key_binding_manager *mgr, const struct config *conf) { } diff --git a/terminal.c b/terminal.c index 0763fb52..3445b89f 100644 --- a/terminal.c +++ b/terminal.c @@ -1089,6 +1089,11 @@ term_init(const struct config *conf, struct fdm *fdm, struct reaper *reaper, goto close_fds; } + /* Need to register *very* early (before the first “goto err”), to + * ensure term_destroy() doesn’t unref a key-binding we haven’t + * yet ref:d */ + key_binding_new_for_conf(wayl->key_binding_manager, wayl, conf); + int ptmx_flags; if ((ptmx_flags = fcntl(ptmx, F_GETFL)) < 0 || fcntl(ptmx, F_SETFL, ptmx_flags | O_NONBLOCK) < 0) @@ -1266,8 +1271,6 @@ term_init(const struct config *conf, struct fdm *fdm, struct reaper *reaper, memcpy(term->colors.table, term->conf->colors.table, sizeof(term->colors.table)); - key_binding_new_for_term(wayl->key_binding_manager, term); - /* Initialize the Wayland window backend */ if ((term->window = wayl_win_init(term, token)) == NULL) goto err; @@ -1583,7 +1586,7 @@ term_destroy(struct terminal *term) if (term == NULL) return 0; - key_binding_unref_term(term->wl->key_binding_manager, term); + key_binding_unref(term->wl->key_binding_manager, term->conf); tll_foreach(term->wl->terms, it) { if (it->item == term) { @@ -2885,7 +2888,7 @@ term_mouse_grabbed(const struct terminal *term, const struct seat *seat) get_current_modifiers(seat, &mods, NULL, 0); const struct key_binding_set *bindings = - key_binding_for(term->wl->key_binding_manager, term, seat); + key_binding_for(term->wl->key_binding_manager, term->conf, seat); const xkb_mod_mask_t override_modmask = bindings->selection_overrides; bool override_mods_pressed = (mods & override_modmask) == override_modmask;