NetBSD Problem Report #54329
From www@netbsd.org Thu Jun 27 05:01:06 2019
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
by mollari.NetBSD.org (Postfix) with ESMTPS id ABFAC7A15D
for <gnats-bugs@gnats.NetBSD.org>; Thu, 27 Jun 2019 05:01:06 +0000 (UTC)
Message-Id: <20190627050105.C22097A1DB@mollari.NetBSD.org>
Date: Thu, 27 Jun 2019 05:01:05 +0000 (UTC)
From: knz@thaumogen.net
Reply-To: knz@thaumogen.net
To: gnats-bugs@NetBSD.org
Cc: jordan@cockroachlabs.com, nikhil.benesch@gmail.com
Subject: libedit: undefined terminal behavior in terminal_move_to_line (fix included)
X-Send-Pr-Version: www-1.0
>Number: 54329
>Category: lib
>Synopsis: libedit: undefined terminal behavior in terminal_move_to_line (fix included)
>Confidential: no
>Severity: non-critical
>Priority: medium
>Responsible: lib-bug-people
>State: open
>Class: change-request
>Submitter-Id: net
>Arrival-Date: Thu Jun 27 05:05:00 +0000 2019
>Last-Modified: Tue Jul 02 18:15:00 +0000 2019
>Originator: Raphael Poss
>Release: all
>Organization:
>Environment:
all
>Description:
The algorithm for terminal_move_to_line()
uses the DO termcap (aka cud or parm_down_cursor), which produces
undefined behavior when the cursor is on the last line. See the manual at https://www.gnu.org/software/termutils/manual/termcap-1.3/html_chapter/termcap_4.html#SEC23, which says:
DO', UP', LE', RI'
Strings of commands to move the cursor n lines down vertically, up
vertically, or n columns left or right. Do not attempt to move past any
edge of the screen with these commands; the effect of trying that is
undefined. Only a few terminal descriptions provide these commands, and
most programs do not use them.
A patch is included below.
(Credits to Jordan Lewis of Cockroach Labs and Nikhil Benesch for investigating this)
>How-To-Repeat:
For reviewers trying to understand why DO is not appropriate on the last line, I encourage you to try running "tput cud 5" while on the last line of your terminal versus somewhere higher up. That command outputs a DO cap with an argument of 5, which should move the cursor 5 lines down. This will work as long as the cursor doesn't hit the last line.
>Fix:
The following patch seems to help:
diff --git a/src/terminal.c b/src/terminal.c
index d8db3d1..fb865b6 100644
--- a/src/terminal.c
+++ b/src/terminal.c
@@ -509,15 +509,10 @@ terminal_move_to_line(EditLine *el, int where)
return;
}
if ((del = where - el->el_cursor.v) > 0) {
- if ((del > 1) && GoodStr(T_DO)) {
- terminal_tputs(el, tgoto(Str(T_DO), del, del), del);
- del = 0;
- } else {
- for (; del > 0; del--)
- terminal__putc(el, '\n');
- /* because the \n will become \r\n */
- el->el_cursor.h = 0;
- }
+ for (; del > 0; del--)
+ terminal__putc(el, '\n');
+ /* because the \n will become \r\n */
+ el->el_cursor.h = 0;
} else { /* del < 0 */
if (GoodStr(T_UP) && (-del > 1 || !GoodStr(T_up)))
terminal_tputs(el, tgoto(Str(T_UP), -del, -del), -del);
>Release-Note:
>Audit-Trail:
From: Valery Ushakov <uwe@stderr.spb.ru>
To: gnats-bugs@netbsd.org
Cc:
Subject: Re: lib/54329: libedit: undefined terminal behavior in
terminal_move_to_line (fix included)
Date: Fri, 28 Jun 2019 02:09:18 +0300
On Thu, Jun 27, 2019 at 05:05:01 +0000, knz@thaumogen.net wrote:
> The algorithm for terminal_move_to_line() uses the DO termcap (aka
> cud or parm_down_cursor), which produces undefined behavior when the
> cursor is on the last line.
Out of curiosity, is this a real problem, though? Doesn't the code
just above verify that "where" is within screen bounds?
-uwe
From: Jordan Lewis <jordan@cockroachlabs.com>
To: "Raphael 'kena' Poss" <knz@thaumogen.net>, gnats-bugs@netbsd.org
Cc: Nikhil Benesch <nikhil.benesch@gmail.com>
Subject: Re: lib/54329: libedit: undefined terminal behavior in
terminal_move_to_line (fix included)
Date: Fri, 28 Jun 2019 09:55:38 -0400
--000000000000c4630f058c62a204
Content-Type: text/plain; charset="UTF-8"
> Out of curiosity, is this a real problem, though? Doesn't the code
> just above verify that "where" is within screen bounds?
> -uwe
The reason we started messing with this is that if you try to restore a
history entry that's taller than 2 lines when the cursor is at the bottom
of the screen, the program tries to use T_DO to move the cursor, which has
no effect, so it ends up overwriting the bottom line with the contents of
all of the lines of the multi-line history entry.
So there is a real practical problem here. Perhaps that code is supposed to
verify the condition you say, but it doesn't work.
On Fri, Jun 28, 2019 at 4:14 AM Raphael 'kena' Poss <knz@thaumogen.net>
wrote:
>
> please respond
>
> -------- Forwarded Message --------
> Subject: Re: lib/54329: libedit: undefined terminal behavior in
> terminal_move_to_line (fix included)
> Date: Thu, 27 Jun 2019 23:10:01 +0000 (UTC)
> From: Valery Ushakov <uwe@stderr.spb.ru>
> Reply-To: gnats-bugs@netbsd.org
> To: lib-bug-people@netbsd.org, gnats-admin@netbsd.org,
> netbsd-bugs@netbsd.org, knz@thaumogen.net
>
> The following reply was made to PR lib/54329; it has been noted by GNATS.
>
> From: Valery Ushakov <uwe@stderr.spb.ru>
> To: gnats-bugs@netbsd.org
> Cc: Subject: Re: lib/54329: libedit: undefined terminal behavior in
> terminal_move_to_line (fix included)
> Date: Fri, 28 Jun 2019 02:09:18 +0300
>
> On Thu, Jun 27, 2019 at 05:05:01 +0000, knz@thaumogen.net wrote:
> > The algorithm for terminal_move_to_line() uses the DO termcap (aka
> > cud or parm_down_cursor), which produces undefined behavior when the
> > cursor is on the last line.
> Out of curiosity, is this a real problem, though? Doesn't the code
> just above verify that "where" is within screen bounds?
> -uwe
>
>
--000000000000c4630f058c62a204
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
<div dir=3D"ltr"><div dir=3D"ltr"><div>=C2=A0> Out of curiosity, is this=
a real problem, though?=C2=A0 Doesn't the code<br>=C2=A0> just abov=
e verify that "where" is within screen bounds?<br>=C2=A0> -uwe=
<div class=3D"gmail-yj6qo"></div><br class=3D"gmail-Apple-interchange-newli=
ne"></div><div>The reason we started messing with this is that if you try t=
o restore a history entry that's taller than 2 lines when the cursor is=
at the bottom of the screen, the program tries to use T_DO to move the cur=
sor, which has no effect, so it ends up overwriting the bottom line with th=
e contents of all of the lines of the multi-line history entry.</div><div><=
br></div><div>So there is a real practical problem here. Perhaps that code =
is supposed to verify the condition you say, but it doesn't work.</div>=
</div><br><div class=3D"gmail_quote"><div dir=3D"ltr" class=3D"gmail_attr">=
On Fri, Jun 28, 2019 at 4:14 AM Raphael 'kena' Poss <<a href=3D"=
mailto:knz@thaumogen.net">knz@thaumogen.net</a>> wrote:<br></div><blockq=
uote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1p=
x solid rgb(204,204,204);padding-left:1ex"><br>
please respond<br>
<br>
-------- Forwarded Message --------<br>
Subject: Re: lib/54329: libedit: undefined terminal behavior in<br>
terminal_move_to_line (fix included)<br>
Date: Thu, 27 Jun 2019 23:10:01 +0000 (UTC)<br>
From: Valery Ushakov <<a href=3D"mailto:uwe@stderr.spb.ru" target=3D"_bl=
ank">uwe@stderr.spb.ru</a>><br>
Reply-To: <a href=3D"mailto:gnats-bugs@netbsd.org" target=3D"_blank">gnats-=
bugs@netbsd.org</a><br>
To: <a href=3D"mailto:lib-bug-people@netbsd.org" target=3D"_blank">lib-bug-=
people@netbsd.org</a>, <a href=3D"mailto:gnats-admin@netbsd.org" target=3D"=
_blank">gnats-admin@netbsd.org</a>,<br>
<a href=3D"mailto:netbsd-bugs@netbsd.org" target=3D"_blank">netbsd-bugs@net=
bsd.org</a>, <a href=3D"mailto:knz@thaumogen.net" target=3D"_blank">knz@tha=
umogen.net</a><br>
<br>
The following reply was made to PR lib/54329; it has been noted by GNATS.<b=
r>
<br>
From: Valery Ushakov <<a href=3D"mailto:uwe@stderr.spb.ru" target=3D"_bl=
ank">uwe@stderr.spb.ru</a>><br>
To: <a href=3D"mailto:gnats-bugs@netbsd.org" target=3D"_blank">gnats-bugs@n=
etbsd.org</a><br>
Cc: Subject: Re: lib/54329: libedit: undefined terminal behavior in<br>
=C2=A0terminal_move_to_line (fix included)<br>
Date: Fri, 28 Jun 2019 02:09:18 +0300<br>
<br>
=C2=A0On Thu, Jun 27, 2019 at 05:05:01 +0000, <a href=3D"mailto:knz@thaumog=
en.net" target=3D"_blank">knz@thaumogen.net</a> wrote:<br>
=C2=A0 > The algorithm for terminal_move_to_line() uses the DO termcap (=
aka<br>
=C2=A0> cud or parm_down_cursor), which produces undefined behavior when=
the<br>
=C2=A0> cursor is on the last line.<br>
=C2=A0 Out of curiosity, is this a real problem, though?=C2=A0 Doesn't =
the code<br>
=C2=A0just above verify that "where" is within screen bounds?<br>
=C2=A0 -uwe<br>
<br>
</blockquote></div></div>
--000000000000c4630f058c62a204--
From: Christos Zoulas <christos@zoulas.com>
To: gnats-bugs@netbsd.org
Cc: lib-bug-people@netbsd.org,
gnats-admin@netbsd.org,
netbsd-bugs@netbsd.org,
knz@thaumogen.net
Subject: Re: lib/54329: libedit: undefined terminal behavior in
terminal_move_to_line (fix included)
Date: Fri, 28 Jun 2019 20:56:20 -0400
What terminal is that? I am asking because xterm works....
christos
From: Raphael 'kena' Poss <knz@thaumogen.net>
To: Jordan Lewis <jordan@cockroachlabs.com>, gnats-bugs@netbsd.org
Cc: Nikhil Benesch <nikhil.benesch@gmail.com>
Subject: Re: lib/54329: libedit: undefined terminal behavior in
terminal_move_to_line (fix included)
Date: Sat, 29 Jun 2019 09:05:46 +0200
On 28-06-19 15:55, Jordan Lewis wrote:
> The reason we started messing with this is that if you try to restore a
> history entry that's taller than 2 lines when the cursor is at the
> bottom of the screen, the program tries to use T_DO to move the cursor,
> which has no effect, so it ends up overwriting the bottom line with the
> contents of all of the lines of the multi-line history entry.
For reference I have tried this (recalling 2 lines of history at the
bottom of the terminal, with libedit) on the following terminals:
- st
- lxterm
- xfce4-terminal
- xterm
and I observe no issue whatsoever. also used `tput cud 5` on the last
line and observed no badness.
Like the other commenter, I suspect you have a different problem.
(Or perhaps you need to record your screen to show more precisely what
is happening)
I would recommend checking the following:
- that the TERM env var is set properly for your terminal. Possibly you
need to change it.
- if you are using `screen` or `tmux`, bring it up to date.
- if you are using `screen` or `tmux`, ensure BOTH that:
- the TERM env var *outside* of the session manager matches your
terminal emulator.
- the TERM env var *inside* of the session manager is properly set to,
respectively, "screen" or "tmux".
--
Raphael 'kena' Poss
From: Jordan Lewis <jordan@cockroachlabs.com>
To: "Raphael 'kena' Poss" <knz@thaumogen.net>
Cc: gnats-bugs@netbsd.org, Nikhil Benesch <nikhil.benesch@gmail.com>
Subject: Re: lib/54329: libedit: undefined terminal behavior in
terminal_move_to_line (fix included)
Date: Sat, 29 Jun 2019 10:05:02 -0400
--0000000000005042eb058c76e25f
Content-Type: text/plain; charset="UTF-8"
On Sat, Jun 29, 2019, 03:05 Raphael 'kena' Poss <knz@thaumogen.net> wrote:
>
> For reference I have tried this (recalling 2 lines of history at the
> bottom of the terminal, with libedit) on the following terminals:
>
> - st
> - lxterm
> - xfce4-terminal
> - xterm
>
> and I observe no issue whatsoever. also used `tput cud 5` on the last
> line and observed no badness.
It needs to be at least 3 lines of history. With 2 lines of history, the
program uses newlines, not T_DO.
Regardless of whether some terminals handle this ok, the old behavior works
only by accident. The manual is very clear that T_DO at the bottom of the
screen results in undefined behavior.
--0000000000005042eb058c76e25f
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
<div dir=3D"auto"><div><div class=3D"gmail_quote"><div dir=3D"ltr" class=3D=
"gmail_attr">On Sat, Jun 29, 2019, 03:05 Raphael 'kena' Poss <<a=
href=3D"mailto:knz@thaumogen.net">knz@thaumogen.net</a>> wrote:</div><b=
lockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1px =
#ccc solid;padding-left:1ex">
<br>
For reference I have tried this (recalling 2 lines of history at the<br>
bottom of the terminal, with libedit) on the following terminals:<br>
<br>
- st<br>
- lxterm<br>
- xfce4-terminal<br>
- xterm<br>
<br>
and I observe no issue whatsoever. also used `tput cud 5` on the last<br>
line and observed no badness.</blockquote></div></div><div dir=3D"auto"><br=
></div><div dir=3D"auto">It needs to be at least 3 lines of history. With 2=
lines of history, the program uses newlines, not T_DO.</div><div dir=3D"au=
to"><br></div><div dir=3D"auto">Regardless of whether some terminals handle=
this ok, the old behavior works only by accident. The manual is very clear=
that T_DO at the bottom of the screen results in undefined behavior.</div>=
<div dir=3D"auto"></div></div>
--0000000000005042eb058c76e25f--
From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/54329 CVS commit: src/lib/libedit
Date: Sat, 29 Jun 2019 17:35:09 -0400
Module Name: src
Committed By: christos
Date: Sat Jun 29 21:35:09 UTC 2019
Modified Files:
src/lib/libedit: terminal.c
Log Message:
PR/54329: Raphael Ross: According to https://www.gnu.org/software/termutils/\
manual/termcap-1.3/html_chapter/termcap_4.html#SEC23 the cursor move multiple
escapes have undefined results when moving out of the screen. Stop using DO
to move down multiple lines and use a loop of newlines instead.
To generate a diff of this commit:
cvs rdiff -u -r1.36 -r1.37 src/lib/libedit/terminal.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
From: Valery Ushakov <uwe@stderr.spb.ru>
To: gnats-bugs@netbsd.org
Cc: Christos Zoulas <christos@zoulas.com>
Subject: Re: lib/54329: libedit: undefined terminal behavior in
terminal_move_to_line (fix included)
Date: Sun, 30 Jun 2019 14:34:50 +0300
On Sat, Jun 29, 2019 at 14:10:01 +0000, Jordan Lewis wrote:
> Regardless of whether some terminals handle this ok, the old behavior works
> only by accident. The manual is very clear that T_DO at the bottom of the
> screen results in undefined behavior.
But the code just above is also very clearly checking for that, isn't
it? It may be getting confused about the window size and end up using
DO when it shouldn't, but then it's the size tracking that should be
fixed (as it likely has other bad consequences), not the innocent
bystander.
Also, as far as I can tell you still haven't provided complete
reproduction scenario. The emulator you use, window size, the value
of TERM, etc.
Christos, were you able to reproduce this? Yyou said you didn't and
then suddenly applied the diff. This is confusing.
-uwe
From: Christos Zoulas <christos@zoulas.com>
To: Valery Ushakov <uwe@stderr.spb.ru>
Cc: gnats-bugs@netbsd.org
Subject: Re: lib/54329: libedit: undefined terminal behavior in
terminal_move_to_line (fix included)
Date: Sun, 30 Jun 2019 09:23:24 -0400
I could not reproduce it, but I've seen it before. In the end the =
optimization is probably not worth the complexity.
This code was written 30 years ago where the cost of each character was =
high at 2400 or 9600 baud. It is also
unclear that the cost/benefit warrants the complexity in the frequently =
used cases. For example on an xterm,
the DO sequence is "\e[%p1%dB" which would take quite a few lines (> 5 =
if I counted right with 0 padding), to
exceed the cost of just printing line feeds. So unless am I missing =
something, we don't lose much by skipping
the optimization.
christos=
From: Nikhil Benesch <nikhil.benesch@gmail.com>
To: gnats-bugs@netbsd.org
Cc: uwe@stderr.spb.ru,
jordan@cockroachlabs.com,
knz@thaumogen.net,
christos@zoulas.com
Subject: Re: lib/54329
Date: Mon, 1 Jul 2019 17:32:45 -0400
I made a video reproducing this bug using xterm on NetBSD 8.0:
https://www.youtube.com/watch?v=3D_11p93_ylyk
Note that the tc1 program shown in the video was compiled with Jess =
Thrysoee's
autotoolized libedit port, v20190324-3.1. (I don't have the NetBSD-foo =
to
upgrade the bundled libedit, but I can learn and try to upgrade my VM to
the latest trunk, if someone thinks that's important.)
It's not shown in the video, but $TERM is set to "xterm".
Responses to a few specific points in line below:
> ...also used `tput cud 5` on the last line and observed no badness.
It's not that badness happens if you run `tput cud 5` with the cursor =
positioned
on the last line; it's that nothing happens. This is demonstrated in the =
video.
The result is that libedit's cursor position tracking gets out of sync =
with the
cursor's actual vertical position; libedit thinks that the cursor has =
been moved
down five lines when, in fact, it has not.
> But the code just above is also very clearly checking for that, isn't
> it? It may be getting confused about the window size and end up using
> DO when it shouldn't, but then it's the size tracking that should be
> fixed (as it likely has other bad consequences), not the innocent
> bystander.
Based on my debugging, libedit is not tracking the absolute vertical =
position of
the cursor. It is tracking only its relative vertical position, relative =
to
wherever the cursor is when the editline instance is initialized. So if =
the
editing session is started when the cursor is already on the last line, =
the
editline.el_cursor.v will be set to "0", which tells us nothing about =
whether it
is safe to send T_DO or not.
It's possible that the absolute vertical position is tracked somewhere, =
but
nowhere that I've found. Perhaps it would be easy enough to add, but I
concur with Christos that the optimization is unlikely to be worthwhile =
on
modern terminal emulators.
From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@netbsd.org
Cc:
Subject: Re: lib/54329
Date: Tue, 2 Jul 2019 18:14:05 +0000
On Mon, Jul 01, 2019 at 09:35:02PM +0000, Nikhil Benesch wrote:
> Based on my debugging, libedit is not tracking the absolute vertical =
> position of
> the cursor. It is tracking only its relative vertical position, relative =
> to
> wherever the cursor is when the editline instance is initialized. So if =
> the
> editing session is started when the cursor is already on the last line, =
> the
> editline.el_cursor.v will be set to "0", which tells us nothing about =
> whether it
> is safe to send T_DO or not.
This appears to be the case, inasmuch as nothing ever assigns
el_cursor.v, only adjusts it, but I still can't reproduce the problem.
If you start this as your terminal session, e.g. with xterm -e:
#!/bin/sh
for x in 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20; do echo $x; done
exec sh -o emacs
the libedit in sh should initialize a long way from the top of the
screen, but I don't see the reported behavior in either xterm or tmux,
even though both do nothing when you run tput DO 5 on the bottom line.
(With a libedit binary from November.)
> It's possible that the absolute vertical position is tracked somewhere, =
> but
> nowhere that I've found. Perhaps it would be easy enough to add, but I
> concur with Christos that the optimization is unlikely to be worthwhile =
> on
> modern terminal emulators.
I'd expect other things to mess up if it's off. Even if it never
issues an absolute cursor move, there are still things like special
treatment of the last character on the last line to go wrong. So it
should really be reading the cursor position from the terminal at
initialization.
I think I've discovered one, which is that when started this way, if
you make an input line that's nearly as large as the whole screen,
move to the middle of it and hold down 'a' to start inserting
characters, when the beginning of the line scrolls off the top of the
screen the cursor doesn't move with it. But it doesn't seem to happen
if started normally. I think. This is presumably from the overflow
code in re_fastputc in refresh.c, which tests el_cursor.v against
t_size.v...
Also, very broken things happen if you cause libedit to initialize
when it's not at column 1.
(hi Nikhil!)
--
David A. Holland
dholland@netbsd.org
>Unformatted:
(Contact us)
$NetBSD: query-full-pr,v 1.43 2018/01/16 07:36:43 maya Exp $
$NetBSD: gnats_config.sh,v 1.9 2014/08/02 14:16:04 spz Exp $
Copyright © 1994-2017
The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.
Home