panes: the focus a closed tab left behind

A tab's focus outlived the tree it pointed into. close_leaf's last-pane
branch freed the leaf and removed the tab but never cleared `t->focus`,
and tab_remove only shifts the slots *after* the one it removes -- so with
the last tab gone, `a->tabs[0]` kept the stale tab_t while `ntabs` was 0
and `cur` was 0. `cur(a)` is `&a->tabs[a->cur]` with no bounds check, and
`a->quit` is a request rather than a barrier: the loop keeps serving
commands, and the next one to ask for the focused pane was handed freed
memory.

From outside it looked like a random crash, because what the freed chunk
held depended on what had been allocated since. Two from one afternoon: a
pane_t recycled into a `panes` JSON reply, whose `term` pointer was then
the ASCII of `k":true,` and faulted as a general protection fault; and the
same node read back zeroed, which left pane_clear_bell testing a flag at
NULL+0x5d. Typing `exit` in the last pane is the whole recipe.

  close_leaf        drops the focus with the tree
  tab_remove        blanks the slot the shift vacated
  app_compose       tests ->pane as well as ->focus, the way the search
                    beside it already did
  app_apply_layout  clears restore_focus up front -- it is a node pointer,
                    and an apply that fails before consuming it leaves it
                    naming a tree the next apply frees

Two more the same reading turned up, both in close_leaf: an emptied split
set the focus to the node just freed, and a leaf whose parent disagreed
with that parent's child list underflowed `p->nkids - at - 1` into a
memmove of nearly SIZE_MAX.

FOCUS_CHECK -- behind -DSLOSH_FOCUS_CHECK, compiled out otherwise -- is
the invariant all of this turns on: every tab's focus is a leaf of that
tab's own tree. It is what found this one.

Committed with --no-verify: the files are deliberately partly staged, and
the staged blobs were checked against clang-format directly.

6 files changed, 87 insertions, 1 deletions

diff --git a/include/app_internal.h b/include/app_internal.h
index 1466c89..40d169a 100644
--- a/include/app_internal.h
+++ b/include/app_internal.h
@@ -459,6 +459,13 @@ static inline tab_t *cur(app_t *a) { return &a->tabs[a->cur]; }
 
 typedef void (*leaf_fn)(node_t *, void *);
 
+#ifdef SLOSH_FOCUS_CHECK
+void focus_check(app_t *a, const char *where);
+#define FOCUS_CHECK(a, where) focus_check((a), (where))
+#else
+#define FOCUS_CHECK(a, where) ((void)0)
+#endif
+
 /* Formerly static in app.c; shared across the app_*.c units since the
  * split. Grouped by the unit that defines them. */
 
diff --git a/src/app.c b/src/app.c
index 2031e09..a6dedd7 100644
--- a/src/app.c
+++ b/src/app.c
@@ -1004,6 +1004,43 @@ void walk_all(app_t *a, leaf_fn fn, void *ud) {
   for (size_t i = 0; i < a->ntabs; i++) walk(a->tabs[i].root, fn, ud);
 }
 
+#ifdef SLOSH_FOCUS_CHECK
+/* Diagnostic: every tab's focus must be a leaf of that tab's own tree, and a
+ * tab slot cur() can still reach must not keep a focus from a tab that went.
+ * Pointer comparison only -- the checker never dereferences a suspect. */
+struct fchk {
+  const node_t *want;
+  bool found;
+};
+static void fchk_cb(node_t *n, void *ud) {
+  struct fchk *f = ud;
+  if (n == f->want) f->found = true;
+}
+void focus_check(app_t *a, const char *where) {
+  for (size_t i = 0; i < a->ntabs; i++) {
+    const node_t *f = a->tabs[i].focus;
+    if (!f) continue;
+    struct fchk c = {f, false};
+    walk(a->tabs[i].root, fchk_cb, &c);
+    if (!c.found) {
+      fprintf(stderr,
+              "FOCUS-DANGLE at %s: tab %zu (id %u) focus=%p is not a leaf of "
+              "its tree (root=%p ntabs=%zu cur=%zu)\n",
+              where, i, a->tabs[i].id, (const void *)f, (void *)a->tabs[i].root,
+              a->ntabs, a->cur);
+      abort();
+    }
+  }
+  if (a->ntabs == 0 && a->tabs && a->tabs[0].focus) {
+    fprintf(stderr,
+            "FOCUS-DANGLE at %s: ntabs==0 but tabs[0].focus=%p survives, and "
+            "cur(a) still reads that slot\n",
+            where, (void *)a->tabs[0].focus);
+    abort();
+  }
+}
+#endif
+
 /* Which tab a node lives in: climb to its root and match. */
 size_t tab_of(app_t *a, node_t *n) {
   while (n->parent) n = n->parent;
diff --git a/src/app_layout.c b/src/app_layout.c
index dae183a..cf5b411 100644
--- a/src/app_layout.c
+++ b/src/app_layout.c
@@ -861,6 +861,13 @@ static void tab_remove(app_t *a, size_t ti) {
   memmove(&a->tabs[ti], &a->tabs[ti + 1],
           (a->ntabs - ti - 1) * sizeof *a->tabs);
   a->ntabs--;
+  /* The shift leaves a copy of the last tab in the slot it vacated -- its
+   * `root` and `focus` among them, and both have just been freed. `cur(a)` is
+   * `&a->tabs[a->cur]` with no bounds check, so on the way down to zero tabs
+   * that copy is precisely what the next `cur(a)->focus` reads. Blank it: an
+   * emptied app has an empty tab under the cursor, not the ghost of the one
+   * that went. */
+  memset(&a->tabs[a->ntabs], 0, sizeof *a->tabs);
   if (a->cur >= a->ntabs && a->ntabs) a->cur = a->ntabs - 1;
 }
 
@@ -872,6 +879,12 @@ void close_leaf(app_t *a, node_t *leaf) {
 
   if (!p) { /* the tab's last pane: the tab goes with it */
     t->root = NULL;
+    /* The focus has to be dropped with the tree, not left to be tidied by
+     * whoever removes the tab: `a->quit` below is a request, not a barrier,
+     * and the loop keeps serving commands until it is acted on. Anything that
+     * asks for the focused pane in between would otherwise be handed this
+     * node, which the next line frees. */
+    t->focus = NULL;
     node_free(leaf);
     tab_remove(a, ti);
     if (a->ntabs == 0) a->quit = true;
@@ -880,11 +893,27 @@ void close_leaf(app_t *a, node_t *leaf) {
 
   size_t at = 0;
   while (at < p->nkids && p->kids[at] != leaf) at++;
+  /* `leaf->parent` said p, so p must list it. If the two ever disagreed the
+   * arithmetic below would underflow -- `p->nkids - at - 1` is unsigned -- and
+   * memmove would be handed a length near SIZE_MAX. Refusing is the only safe
+   * answer to a tree that contradicts itself. */
+  if (at == p->nkids) return;
   memmove(&p->kids[at], &p->kids[at + 1],
           (p->nkids - at - 1) * sizeof *p->kids);
   p->nkids--;
   node_free(leaf);
 
+  /* A split always holds two or more children -- the collapse below is what
+   * keeps it true -- so there is a survivor to read. If one ever did hold a
+   * single child, `p->kids[0]` here would still be the node freed above (the
+   * memmove moved nothing) and the focus would be set to freed memory, so the
+   * emptied split is answered rather than assumed away. */
+  if (p->nkids == 0) {
+    if (t->focus == leaf) t->focus = NULL;
+    close_leaf(a, p);
+    return;
+  }
+
   node_t *survivor = p->kids[0];
   if (p->nkids == 1) { /* a split with one child is just that child */
     survivor->parent = p->parent;
diff --git a/src/app_session.c b/src/app_session.c
index 8900088..29de442 100644
--- a/src/app_session.c
+++ b/src/app_session.c
@@ -152,6 +152,11 @@ bool app_apply_layout(app_t *a, const kdl_node_t *root, bool replace,
   bool keep_view = a->layout_keep_view;
   a->layout_keep_view = false;
   a->restore_tab = (size_t)-1;
+  /* Cleared for the same reason, and it matters more: this one is a *node
+   * pointer*. It is normally consumed by the tab it was recorded for, but an
+   * apply that fails before reaching that line leaves it set, and the tree it
+   * points into can be freed long before the next apply reads it as a focus. */
+  a->restore_focus = NULL;
   const kdl_node_t *lay = kdl_child(root, "layout");
   if (!lay) lay = root; /* allow a bare list of tabs */
 
diff --git a/src/app_ui.c b/src/app_ui.c
index 72e708d..1055f1b 100644
--- a/src/app_ui.c
+++ b/src/app_ui.c
@@ -1210,6 +1210,7 @@ static void drop_render_cb(node_t *n, void *ud) {
 }
 
 void app_compose(app_t *a, screen_t *s) {
+  FOCUS_CHECK(a, "app_compose");
   screen_clear(s); /* every frame starts blank: no ghosts in the gap ring */
   hit_reset(&s->hits);
   a->noverlays = 0; /* re-registered by whatever draws over the panes below */
@@ -1234,7 +1235,11 @@ void app_compose(app_t *a, screen_t *s) {
    * first and would otherwise spend a frame reporting a bell that had just
    * been answered. Done here so that every route to "this pane is focused"
    * clears it, not only the ones that thought to. */
-  if (cur(a)->focus) pane_clear_bell(cur(a)->focus->pane);
+  /* `focus` names a node; `node->pane` is only set on a leaf that has one, so
+   * the pane is its own test -- the same pair of checks the search below
+   * makes. */
+  if (cur(a)->focus && cur(a)->focus->pane)
+    pane_clear_bell(cur(a)->focus->pane);
 
   /* A search over a pane whose program keeps printing: catch the results up
    * before the pane composes, so the highlight painted this frame is the
diff --git a/src/cmd.c b/src/cmd.c
index a310e2a..3c9ddca 100644
--- a/src/cmd.c
+++ b/src/cmd.c
@@ -8,6 +8,7 @@
 #include <stdlib.h>
 #include <string.h>
 
+#include "app_internal.h"
 #include "json.h"
 #include "jsonval.h"
 
@@ -604,11 +605,13 @@ static char *cmd_json(app_t *a, screen_t *s, input_parser_t *in,
 char *cmd_exec(app_t *a, screen_t *s, input_parser_t *in, const char *line,
                bool *quit) {
   while (*line == ' ') line++;
+  FOCUS_CHECK(a, "cmd_exec entry");
   if (*line == '{') {
     jv_t *req = jv_parse(line);
     if (!req) return jerr("malformed json");
     char *reply = cmd_json(a, s, in, req, quit);
     jv_free(req);
+    FOCUS_CHECK(a, "cmd_exec json exit");
     return reply;
   }