[BUG/PATCH] crash in layout_free_cell(NULL) after invalid select-layout

[Jere Viikari]

Hello,

I found a crash in current tmux master when restoring a session with
tmux-resurrect. I think the crash itself is in tmux, not in tmux-resurrect,
although tmux-resurrect is the thing which triggers it in my setup.

Environment:
OS: Linux
tmux: next-3.7 from git master
tmux git commit tested: c67787536b49d13fb77fb0fb462f8714ce363a7c
tmux-resurrect git commit: cff343c

The crash happens when I start tmux and restore my saved session with
prefix + C-r from tmux-resurrect.

The saved resurrect file contains this window layout for main:0:

2321,188x55,0,0{94x55,0,0[94x22,0,0,1,94x17,0,23,2,94x14,0,41,3],93x55,95,0[93x22,95,0,4,93x18,95,23,5,93x13,95,42,6]}<94x17,0,23,2,94x22,0,0,1,94x14,0,41,3,93x22,95,0,4,93x18,95,23,5,93x13,95,42,6>

tmux-resurrect eventually runs this command:

select-layout -t main:0
"2321,188x55,0,0{94x55,0,0[94x22,0,0,1,94x17,0,23,2,94x14,0,41,3],93x55,95,0[93x22,95,0,4,93x18,95,23,5,93x13,95,42,6]}<94x17,0,23,2,94x22,0,0,1,94x14,0,41,3,93x22,95,0,4,93x18,95,23,5,93x13,95,42,6>"

The layout is rejected as invalid, which is OK. But then tmux crashes while
cleaning up the failed parse. I reproduced it also without my normal tmux
server by using a separate socket and running the restore script directly:

tmux -Ltest -f ~/.tmux.conf -vv new-session -d -s 0
tmux -Ltest run-shell ~/.tmux/plugins/tmux-resurrect/scripts/restore.sh

I also checked whether this is caused by my compiler flags. It does not seem
to be. The crash reproduced with a plain debug build:

CC=gcc CFLAGS="-O0 -g -fno-omit-frame-pointer" LDFLAGS="" ./configure

I got this backtrace from gdb:

Program received signal SIGSEGV, Segmentation fault.
layout_free_cell (lc=0x0) at layout.c:81
81 switch (lc->type) {

#0 layout_free_cell (lc=0x0) at layout.c:81
#1 layout_parse (w=0x55555575cb10,
layout=0x555555749116
"<94x17,0,23,2,94x22,0,0,1,94x14,0,41,3,93x22,95,0,4,93x18,95,23,5,93x13,95,42,6>",
cause=0x7fffffffcc48) at layout-custom.c:316
#2 cmd_select_layout_exec (self=0x55555572f5f0, item=0x555555752810)
at cmd-select-layout.c:127
#3 cmdq_fire_command (item=0x555555752810) at cmd-queue.c:649
#4 cmdq_next (c=0x555555789e00) at cmd-queue.c:774
#5 server_loop () at server.c:275
#6 proc_loop (tp=0x5555556e5e40, loopcb=0x555555614b2c <server_loop>)
at proc.c:214
#7 server_start (...) at server.c:254
#8 client_connect (...) at client.c:164
#9 client_main (...) at client.c:290
#10 main (...) at tmux.c:556

Looking at layout-custom.c, the fail path does this:

fail:
layout_free_cell(tiled_lc);
layout_free_cell(floating_lc);
return (-1);

In this case floating_lc is NULL, so layout_free_cell() dereferences NULL at
lc->type.

This small change fixes the crash for me:

diff --git a/layout.c b/layout.c
index 5ecad949..8ee80261 100644
--- a/layout.c
+++ b/layout.c
@@ -78,6 +78,9 @@ layout_free_cell(struct layout_cell *lc)
{
struct layout_cell *lcchild;

+ if (lc == NULL)
+ return;
+
switch (lc->type) {
case LAYOUT_LEFTRIGHT:
case LAYOUT_TOPBOTTOM:

After this change, the same tmux-resurrect restore no longer crashes. The
session is restored and the server stays alive:

main: 3 windows
main:0: bash (6 panes)
main:1: audit (1 pane)
main:2: bash (1 pane)

It does not necessarily make the old saved layout valid; it only makes the
invalid layout error path not crash the server. That seems right to me, because
invalid input to select-layout should return an error, not kill tmux.

I tried to find when this started. From git blame and git log -S, it _appears_
to be introduced by:

a8520ba59e32133414c710b26ec23bf3c19f8b23
Bring over some of layout and positioning code for floating panes.

Thanks,
Jere

[Nicholas Marriott]

Thanks, I have made your change now and also removed some now-unnecessary checks in the callers. Will be in GitHub later on.