crashes when setting COLUMNS=0 or 1

crashes when setting COLUMNS=0 or 1

[Mikael Magnusson]

To reproduce, run COLUMNS=0 and type two characters. This code in
zle_refresh is where it crashes, it looks quite suspicious too.

ZR_memset(p1 + nllen, zr_sp, winw - nllen);
p1[winw] = zr_zr;
if (nllen < winw)
    p1[winw + 1] = zr_zr;
else
    p1[winw + 1] = nl[winw + 1];

The code seems to expect nllen to possibly be larger than winw, yet it
passes winw - nllen to memset as the length? (which is where it
crashes). I can actually get a crash with a 1-char wide window too, by
just resizing the terminal and typing some characters (This doesn't
happen right away, needs to fiddle around for a bit, possibly ctrl-a
and type some more). At first I thought it had to be doublewidth
characters, but regular ones trigger it too. Hm, but this crashes
somewhere else:

(gdb) bt
#0  0x00007ffff74198d9 in free () from /lib64/libc.so.6
#1  0x000000000047d630 in free_colour_buffer () at prompt.c:1863
#2  0x00007ffff6aab222 in zle_free_highlight () at zle_refresh.c:374
#3  0x00007ffff6aaeb08 in zrefresh () at zle_refresh.c:1715
#4  0x00007ffff6aa1d1e in zlecore () at zle_main.c:1100
#5  0x00007ffff6aa22b5 in zleread (lp=0x6c3a98, rp=0x0, flags=3, context=0)
    at zle_main.c:1219
#6  0x00007ffff6aa45b5 in zle_main_entry (cmd=1, ap=0x7fffffffd290) at
zle_main.c:1874
#7  0x00000000004456dd in zleentry (cmd=1) at init.c:1363
#8  0x000000000044616f in inputline () at input.c:281
#9  0x0000000000445fe6 in ingetc () at input.c:217
#10 0x000000000043b8b6 in ihgetc () at hist.c:279
#11 0x000000000044e156 in gettok () at lex.c:717
#12 0x000000000044d907 in zshlex () at lex.c:395
#13 0x000000000046aefb in parse_event () at parse.c:451
#14 0x0000000000442a9d in loop (toplevel=1, justonce=0) at init.c:132
#15 0x0000000000445be6 in zsh_main (argc=2, argv=0x7fffffffd678) at init.c:1528
#16 0x0000000000410784 in main (argc=2, argv=0x7fffffffd678) at ./main.c:93

Not sure what's going on in this case.

-- 
Mikael Magnusson

Re: crashes when setting COLUMNS=0 or 1

[Geoff Wing]

On Thursday 2011-06-09 12:36 +0200, Mikael Magnusson output:
:To reproduce, run COLUMNS=0 and type two characters. This code in
:zle_refresh is where it crashes, it looks quite suspicious too.
:
:ZR_memset(p1 + nllen, zr_sp, winw - nllen);
:p1[winw] = zr_zr;
:if (nllen < winw)
:    p1[winw + 1] = zr_zr;
:else
:    p1[winw + 1] = nl[winw + 1];
:
:The code seems to expect nllen to possibly be larger than winw, yet it
:passes winw - nllen to memset as the length? (which is where it
:crashes).

I believe that it's checking if nllen equals winw (which is quite valid),
not that it is larger.  It's been a long time since I looked at this code.

:I can actually get a crash with a 1-char wide window too, by
:just resizing the terminal and typing some characters (This doesn't
:happen right away, needs to fiddle around for a bit, possibly ctrl-a
:and type some more). At first I thought it had to be doublewidth
:characters, but regular ones trigger it too.

Sounds like a different bug.

Old versions of Zsh never let COLUMNS be set to 0 or negative numbers.
It would retain its old value if you tried since it just doesn't make
sense.  This behaviour should be recovered.  Try 

% COLUMNS=-1; echo $COLUMNS
-1
%  (core dumped)

Regards,
Geoff

Re: crashes when setting COLUMNS=0 or 1

[Bart Schaefer]

On Jun 26,  1:38pm, Geoff Wing wrote:
}
} Old versions of Zsh never let COLUMNS be set to 0 or negative numbers.

Not precisely true.  Here's zsh 2.4:

torch<501> COLUMNS=-1
torch<502> print $COLUMNS
-1
*** glibc detected *** malloc(): memory corruption (fast): 0x0809d838 ***
zsh: abort (core dumped)  src/zsh

} It would retain its old value if you tried since it just doesn't make
} sense.  This behaviour should be recovered.

In 2.4, putprompt() would force columns = 80 when columns == 0, but there
isn't any test I can find for columns < 0.

In recent version adjustcolumns() has this:

#ifdef TIOCGWINSZ
    if (signalled || zterm_columns <= 0)
        zterm_columns = shttyinfo.winsize.ws_col;
    else
        shttyinfo.winsize.ws_col = zterm_columns;
#endif /* TIOCGWINSZ */
    if (zterm_columns <= 0) {
        DPUTS(signalled, "BUG: Impossible TIOCGWINSZ cols");
        zterm_columns = tccolumns > 0 ? tccolumns : 80;
    }

This is called from adjustwinsize(), and from various bits of params.c:

IPDEF5("COLUMNS", &zterm_columns, zlevar_gsu)

static const struct gsu_integer zlevar_gsu =
{ intvargetfn, zlevarsetfn, stdunsetfn };

void
zlevarsetfn(Param pm, zlong x)
{
    zlong *p = pm->u.valptr;

    *p = x;
    if (p == &zterm_lines || p == &zterm_columns)
        adjustwinsize(2 + (p == &zterm_columns));
}

But for some reason zlvarsetfn is never called when assigning COLUMNS,
so adjustwinsize() never happens.

Tracing through, COLUMNS=0 is handled by addvars() via execsimple(),
which does call v->pm->gsu.i->setfn at params.c:2343 ... but that's
the generic intvarsetfn, zlevarsetfn has been lost somewhere along
the way.

This may indicate there's a more generalized problem with the handling
of parameters for which IPDEF* have assigned a custom gsu.

Re: crashes when setting COLUMNS=0 or 1

[Bart Schaefer]

On Jun 26, 12:03am, Bart Schaefer wrote:
}
} IPDEF5("COLUMNS", &zterm_columns, zlevar_gsu)

Oh, for pity's sake.  The definition of IPDEF5 never references the F
argument.

Index: Src/params.c
===================================================================
RCS file: /extra/cvsroot/zsh/zsh-4.0/Src/params.c,v
retrieving revision 1.48
diff -u -r1.48 params.c
--- Src/params.c        20 Dec 2011 17:13:38 -0000      1.48
+++ Src/params.c        26 Jun 2012 07:18:22 -0000
@@ -315,7 +315,7 @@
 IPDEF4("PPID", &ppid),
 IPDEF4("ZSH_SUBSHELL", &zsh_subshell),
 
-#define IPDEF5(A,B,F) {{NULL,A,PM_INTEGER|PM_SPECIAL},BR((void *)B),GSU(varinteger_gsu),10,0,NULL,NULL,NULL,0}
+#define IPDEF5(A,B,F) {{NULL,A,PM_INTEGER|PM_SPECIAL},BR((void *)B),GSU(F),10,0,NULL,NULL,NULL,0}
 IPDEF5("COLUMNS", &zterm_columns, zlevar_gsu),
 IPDEF5("LINES", &zterm_lines, zlevar_gsu),
 IPDEF5("OPTIND", &zoptind, varinteger_gsu),

Re: crashes when setting COLUMNS=0 or 1

[Bart Schaefer]

On Jun 26, 12:03am, Bart Schaefer wrote:
}
} In recent version adjustcolumns() has this:
} 
} #ifdef TIOCGWINSZ
}     if (signalled || zterm_columns <= 0)
}         zterm_columns = shttyinfo.winsize.ws_col;
}     else
}         shttyinfo.winsize.ws_col = zterm_columns;
} #endif /* TIOCGWINSZ */
}     if (zterm_columns <= 0) {
}         DPUTS(signalled, "BUG: Impossible TIOCGWINSZ cols");
}         zterm_columns = tccolumns > 0 ? tccolumns : 80;
}     }
} 
} This is called from adjustwinsize()

So, interesting side-effect:  If my patch for IPDEF5 is applied, then
settings of COLUMNS and LINES get propagated through to the tty driver
and can end up affecting the behavior of the parent shell (or anything
else that's using the terminal where those values were changed), even
after the shell where the assignments were made has exited.

E.g. (COLUMNS=20) in a subshell will change the tty driver which will
signal the parent shell which will pick up the change and set its own
column width to 20.  This is the only case I can think of where a
parameter assigned in a subshell can behave this way.

So we might want to consider carefully whether this is something we want
to inject only days before a 5.0.0 release ... but we should still fix
the crash somehow, so maybe there's a corresponding change needed in
some other place.

Re: crashes when setting COLUMNS=0 or 1

[Peter Stephenson]

On Tue, 26 Jun 2012 00:30:42 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> So, interesting side-effect:  If my patch for IPDEF5 is applied, then
> settings of COLUMNS and LINES get propagated through to the tty driver
> and can end up affecting the behavior of the parent shell (or anything
> else that's using the terminal where those values were changed), even
> after the shell where the assignments were made has exited.
> 
> E.g. (COLUMNS=20) in a subshell will change the tty driver which will
> signal the parent shell which will pick up the change and set its own
> column width to 20.  This is the only case I can think of where a
> parameter assigned in a subshell can behave this way.

That's not necessarily always a problem.  The parent is in case subject
to such changes asynchronously via SIGWINCH.  The fact that it's getting
the value a different way shouldn't make matters worse in general.

Where it's unhelpful is if you're setting COLUMNS not because anything
has changed but as a temporary trick for some utility that tests it and
truncates.  Then you wouldn't want the change to propagate back.

This is going to be niggling if it means these two have different
effects:

COLUMNS=20 ls
(export COLUMNS=20; ls)

since they look as if they ought to be similar.

Howeve, I'm not sure what can reasonably be done.  Restoring COLUMNS on
exit from a subshell doesn't seem a particularly clean thing to do, but
might work in most cases.

-- 
Peter Stephenson <pws@csr.com>            Software Engineer
Tel: +44 (0)1223 692070                   Cambridge Silicon Radio Limited
Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, UK


Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom
More information can be found at www.csr.com. Follow CSR on Twitter at http://twitter.com/CSR_PLC and read our blog at www.csr.com/blog

Re: crashes when setting COLUMNS=0 or 1

[Bart Schaefer]

On Jun 26, 10:33am, Peter Stephenson wrote:
} Subject: Re: crashes when setting COLUMNS=0 or 1
}
} On Tue, 26 Jun 2012 00:30:42 -0700
} Bart Schaefer <schaefer@brasslantern.com> wrote:
} > So, interesting side-effect:  If my patch for IPDEF5 is applied, then
} > settings of COLUMNS and LINES get propagated through to the tty driver
} > and can end up affecting the behavior of the parent shell (or anything
} > else that's using the terminal where those values were changed), even
} > after the shell where the assignments were made has exited.
} 
} That's not necessarily always a problem.  The parent is in case subject
} to such changes asynchronously via SIGWINCH.  The fact that it's getting
} the value a different way shouldn't make matters worse in general.

Yeah, but programs (including older versions of zsh) that don't expect
the tty driver to lie about the terminal size can be very confused.

(Aside:  ZLE can behave incredibly badly if the actual terminal width is
greater than what zterm_columns asserts, because of assumptions about what
happens with auto-wrap at the margins.)
 
} Where it's unhelpful is if you're setting COLUMNS not because anything
} has changed but as a temporary trick for some utility that tests it and
} truncates.  Then you wouldn't want the change to propagate back.

Indeed.

} This is going to be niggling if it means these two have different
} effects:
} 
} COLUMNS=20 ls
} (export COLUMNS=20; ls)

Actually those behave exactly the same; in both cases the new COLUMNS
persists after the command exits, because it has been enshrined in the
terminal driver.  Undoing of the "temporary" assignment in the first
example does not cause an explicit re-assignment of the old value, so
the driver is not reloaded.
 
} Howeve, I'm not sure what can reasonably be done.

Here's one possibility:   Never poke the new COLUMNS into the tty driver.
Actually we could just remove the entire #ifdef block -- it's been dead
code for years because (from >= 2) has never been true, because the only
place adjustwinsize(2) is referenced is via zlevarsetfn, which has never
been called because of the IPDEF5 typo.

On the other hand it might in the longer term (no pun intended) be
useful to figure out if there are circumstances where we do want this to
happen -- e.g., some assignments possibly should do it, others not, and
maybe we can pass a different "from" value in those cases.

Index: Src/utils.c
--- Src/utils.c 2012-05-07 20:54:49.000000000 -0700
+++ Src/utils.c 2012-06-26 07:07:15.000000000 -0700
@@ -1684,11 +1684,11 @@
 #ifdef TIOCGWINSZ
     if (interact && from >= 2 &&
        (shttyinfo.winsize.ws_row != ttyrows ||
         shttyinfo.winsize.ws_col != ttycols)) {
        /* shttyinfo.winsize is already set up correctly */
-       ioctl(SHTTY, TIOCSWINSZ, (char *)&shttyinfo.winsize);
+       /* ioctl(SHTTY, TIOCSWINSZ, (char *)&shttyinfo.winsize); */
     }
 #endif /* TIOCGWINSZ */
 
     if (zleactive && resetzle) {
 #ifdef TIOCGWINSZ


Without objection, I'll commit these two patches.