NetBSD-Bugs archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: bin/50092: huge memory leaks in vi(1) when changing screen size
The following reply was made to PR bin/50092; it has been noted by GNATS.
From: Rin Okuyama <okuyama%flex.phys.tohoku.ac.jp@localhost>
To: Julian Coleman <jdc%coris.org.uk@localhost>, gnats-bugs%NetBSD.org@localhost
Cc:
Subject: Re: bin/50092: huge memory leaks in vi(1) when changing screen size
Date: Mon, 3 Aug 2015 10:56:19 +0900
This is a multi-part message in MIME format.
--------------070700000004050800030903
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Transfer-Encoding: 7bit
Hi,
> If we resize many times, we probably only care about the final size. So,
> we really could ignore any apart from the last resize. However, if we're
> already processing a resize, we should finish processing so that everthing
> is consistent (i.e. not interrupt the current resize).
>
> I wonder if we can just save the new size when we're already processing and
> check at the end of the current processing if a new (and different) size is
> saved? Then we would re-run the resize with the new saved size.
I examined the problem with the continuous screen size change in detail,
using icewm/xterm on a slow virtual machine.
(a) The original NetBSD version sometimes aborts with message
"ex/vi: Error: screen: Interrupted system call". This is because
newterm(3) is interrupted by SIGWINCH. This problem was reported for
nvi-1.79 as bin/25849:
http://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=25849
The fix is merged to NetBSD at that time, however it seems to be
overlooked when nvi-1.86 was imported.
(b) The patched version attached to my PR does not abort as it is based
on the fix to bin/25849. However, it sometimes issues error messages
like "Error: move: l(48 + 0) c(0 + 0)". This is because setterm(3) or
resizeterm(3) are interrupted by SIGWINCH and the screen initialization
fails. The fix to bin/25849 is not sufficient; the reason why it does
not abort is simply because it does not check return values of
setterm(3) and resizeterm(3).
To address the problem, I modified the patch as follows.
(1) During initialization of the screen, block signals to make sure that
curses/terminfo routines are not interrupted.
(2) In the signal handler for SIGWINCH, wait up to 1/10 seconds for a
succeeding SIGWINCH received. This approximately realizes your
suggestion.
In addition, I made following changes.
(3) Use delscreen(3) and newterm(3) for reinitialization of the screen,
instead of setterm(3) and resizeterm(3), that are BSD/ncurses extension
to the X/Open standards. This also resolves a memory leak when
switching to ex-mode from vi-mode.
(4) Delete the terminfo cur_term when we enter to vi-mode. We must
initialize it in main() for processing .exrc or EXINIT. However, in
vi-mode, it is overwritten by newterm(3), which results in a memory
leak.
(5) In ex-mode, use del_curterm(3) and setupterm(3) to update the
terminfo database when the terminal type is changed. In the original
version, we forget this before retrieving the terminfo entries.
For memory leaks in libcurses/terminfo, I will send another PRs if
necessary.
Thanks,
Rin
--------------070700000004050800030903
Content-Type: text/plain; charset=UTF-8; x-mac-type="0"; x-mac-creator="0";
name="nvi.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename="nvi.patch"
Index: src/external/bsd/nvi/Makefile.inc
diff -u src/external/bsd/nvi/Makefile.inc:1.1.1.1 src/external/bsd/nvi/Makefile.inc:1.4
--- src/external/bsd/nvi/Makefile.inc:1.1.1.1 Fri Jul 31 20:44:59 2015
+++ src/external/bsd/nvi/Makefile.inc Mon Aug 3 00:21:50 2015
@@ -7,4 +7,4 @@
BINDIR=/usr/bin
CWARNFLAGS.clang+= -Wno-error=unused-const-variable
-VERSION=1.81.6-2013-11-20
+VERSION=1.81.6-2013-11-20nb1
Index: src/external/bsd/nvi/dist/cl/cl.h
diff -u src/external/bsd/nvi/dist/cl/cl.h:1.1.1.1 src/external/bsd/nvi/dist/cl/cl.h:1.2
--- src/external/bsd/nvi/dist/cl/cl.h:1.1.1.1 Fri Jul 31 20:44:59 2015
+++ src/external/bsd/nvi/dist/cl/cl.h Sat Aug 1 18:24:49 2015
@@ -42,6 +42,8 @@
struct termios ex_enter;/* Terminal values to enter ex. */
struct termios vi_enter;/* Terminal values to enter vi. */
+ SCREEN *screen; /* Curses screen. */
+
char *el; /* Clear to EOL terminal string. */
char *cup; /* Cursor movement terminal string. */
char *cuu1; /* Cursor up terminal string. */
@@ -77,6 +79,8 @@
#define CL_SIGTERM 0x0100 /* SIGTERM arrived. */
#define CL_SIGWINCH 0x0200 /* SIGWINCH arrived. */
#define CL_STDIN_TTY 0x0400 /* Talking to a terminal. */
+#define CL_SETUPTERM 0x0800 /* Terminal initialized. */
+#define CL_CHANGE_TERM 0x1000 /* Terminal changed. */
u_int32_t flags;
} CL_PRIVATE;
Index: src/external/bsd/nvi/dist/cl/cl_main.c
diff -u src/external/bsd/nvi/dist/cl/cl_main.c:1.1.1.1 src/external/bsd/nvi/dist/cl/cl_main.c:1.4
--- src/external/bsd/nvi/dist/cl/cl_main.c:1.1.1.1 Fri Jul 31 20:44:59 2015
+++ src/external/bsd/nvi/dist/cl/cl_main.c Mon Aug 3 00:21:14 2015
@@ -109,6 +109,7 @@
ttype = "unknown";
}
term_init(gp->progname, ttype);
+ F_SET(clp, CL_SETUPTERM);
/* Add the terminal type to the global structure. */
if ((OG_D_STR(gp, GO_TERM) =
@@ -292,6 +293,22 @@
h_winch(int signo)
{
GLOBAL_CLP;
+ sigset_t sigset;
+ struct timespec timeout;
+
+ /*
+ * Some window managers continuously change the screen size of terminal
+ * emulators, by which a lot of SIGWINCH signals are to be received. In
+ * such a case, we only need to respond the final signal; the remaining
+ * signals are meaningless. Thus, we wait here up to 1/10 of a second
+ * for a succeeding signal received.
+ */
+ (void)sigemptyset(&sigset);
+ (void)sigaddset(&sigset, SIGWINCH);
+ timeout.tv_sec = 0;
+ timeout.tv_nsec = 100 * 1000 * 1000;
+ while (sigtimedwait(&sigset, NULL, &timeout) != -1)
+ continue;
F_SET(clp, CL_SIGWINCH);
}
Index: src/external/bsd/nvi/dist/cl/cl_screen.c
diff -u src/external/bsd/nvi/dist/cl/cl_screen.c:1.1.1.1 src/external/bsd/nvi/dist/cl/cl_screen.c:1.5
--- src/external/bsd/nvi/dist/cl/cl_screen.c:1.1.1.1 Fri Jul 31 20:44:59 2015
+++ src/external/bsd/nvi/dist/cl/cl_screen.c Mon Aug 3 00:29:51 2015
@@ -34,6 +34,10 @@
#include "../common/common.h"
#include "cl.h"
+#ifndef BLOCK_SIGNALS
+extern sigset_t __sigblockset;
+#endif
+
static int cl_ex_end __P((GS *));
static int cl_ex_init __P((SCR *));
static void cl_freecap __P((CL_PRIVATE *));
@@ -53,26 +57,37 @@
CL_PRIVATE *clp;
WINDOW *win;
GS *gp;
+ int ret;
gp = sp->gp;
clp = CLP(sp);
win = CLSP(sp) ? CLSP(sp) : stdscr;
+ ret = 0;
+
+ /*
+ * During initialization of the screen, block signals to make sure that
+ * curses/terminfo routines are not interrupted.
+ */
+ (void)sigprocmask(SIG_BLOCK, &__sigblockset, NULL);
+
/* See if the current information is incorrect. */
if (F_ISSET(gp, G_SRESTART)) {
if (CLSP(sp)) {
delwin(CLSP(sp));
sp->cl_private = NULL;
}
- if (cl_quit(gp))
- return (1);
+ if (cl_quit(gp)) {
+ ret = 1;
+ goto end;
+ }
F_CLR(gp, G_SRESTART);
}
/* See if we're already in the right mode. */
if ((LF_ISSET(SC_EX) && F_ISSET(sp, SC_SCR_EX)) ||
(LF_ISSET(SC_VI) && F_ISSET(sp, SC_SCR_VI)))
- return (0);
+ goto end;
/*
* Fake leaving ex mode.
@@ -109,8 +124,10 @@
/* Enter the requested mode. */
if (LF_ISSET(SC_EX)) {
- if (cl_ex_init(sp))
- return (1);
+ if (cl_ex_init(sp)) {
+ ret = 1;
+ goto end;
+ }
F_SET(clp, CL_IN_EX | CL_SCR_EX_INIT);
/*
@@ -121,12 +138,17 @@
tputs(tgoto(clp->cup,
0, O_VAL(sp, O_LINES) - 1), 1, cl_putchar);
} else {
- if (cl_vi_init(sp))
- return (1);
+ if (cl_vi_init(sp)) {
+ ret = 1;
+ goto end;
+ }
F_CLR(clp, CL_IN_EX);
F_SET(clp, CL_SCR_VI_INIT);
}
- return (0);
+end:
+ /* Unblock signals. */
+ (void)sigprocmask(SIG_UNBLOCK, &__sigblockset, NULL);
+ return ret;
}
/*
@@ -234,10 +256,14 @@
o_cols = getenv("COLUMNS");
cl_putenv(sp, "COLUMNS", NULL, (u_long)O_VAL(sp, O_COLUMNS));
+ /* Delete cur_term if exists. */
+ if (F_ISSET(clp, CL_SETUPTERM)) {
+ if (del_curterm(cur_term))
+ return (1);
+ F_CLR(clp, CL_SETUPTERM);
+ }
+
/*
- * We don't care about the SCREEN reference returned by newterm, we
- * never have more than one SCREEN at a time.
- *
* XXX
* The SunOS initscr() can't be called twice. Don't even think about
* using it. It fails in subtle ways (e.g. select(2) on fileno(stdin)
@@ -249,7 +275,7 @@
* have to specify the terminal type.
*/
errno = 0;
- if (newterm(__UNCONST(ttype), stdout, stdin) == NULL) {
+ if ((clp->screen = newterm(__UNCONST(ttype), stdout, stdin)) == NULL) {
if (errno)
msgq(sp, M_SYSERR, "%s", ttype);
else
@@ -374,8 +400,6 @@
fast: /* Set the terminal modes. */
if (tcsetattr(STDIN_FILENO, TCSASOFT | TCSADRAIN, &clp->vi_enter)) {
- if (errno == EINTR)
- goto fast;
msgq(sp, M_SYSERR, "tcsetattr");
err: (void)cl_vi_end(sp->gp);
return (1);
@@ -416,6 +440,9 @@
/* End curses window. */
(void)endwin();
+ /* Delete curses screen. */
+ delscreen(clp->screen);
+
/*
* XXX
* The screen TE sequence just got sent. See the comment in
@@ -434,6 +461,8 @@
cl_ex_init(SCR *sp)
{
CL_PRIVATE *clp;
+ int error;
+ const char *ttype;
clp = CLP(sp);
@@ -445,6 +474,22 @@
if (!F_ISSET(clp, CL_STDIN_TTY))
return (0);
+ if (F_ISSET(clp, CL_CHANGE_TERM)) {
+ if (F_ISSET(clp, CL_SETUPTERM) && del_curterm(cur_term))
+ return (1);
+ F_CLR(clp, CL_SETUPTERM | CL_CHANGE_TERM);
+ }
+
+ if (!F_ISSET(clp, CL_SETUPTERM)) {
+ /* We'll need a terminal type. */
+ if (opts_empty(sp, O_TERM, 0))
+ return (1);
+ ttype = O_STR(sp, O_TERM);
+ (void)setupterm(ttype, STDOUT_FILENO, &error);
+ if (error == 0 || error == -1)
+ return (1);
+ }
+
/* Get the ex termcap/terminfo strings. */
(void)cl_getcap(sp, "cup", &clp->cup);
(void)cl_getcap(sp, "smso", &clp->smso);
Index: src/external/bsd/nvi/dist/cl/cl_term.c
diff -u src/external/bsd/nvi/dist/cl/cl_term.c:1.1.1.1 src/external/bsd/nvi/dist/cl/cl_term.c:1.3
--- src/external/bsd/nvi/dist/cl/cl_term.c:1.1.1.1 Fri Jul 31 20:44:59 2015
+++ src/external/bsd/nvi/dist/cl/cl_term.c Sat Aug 1 18:24:49 2015
@@ -268,9 +268,12 @@
clp = CLP(sp);
switch (opt) {
+ case O_TERM:
+ if (F_ISSET(sp, SC_SCR_EX))
+ F_SET(clp, CL_CHANGE_TERM);
+ /* FALLTHROUGH */
case O_COLUMNS:
case O_LINES:
- case O_TERM:
/*
* Changing the columns, lines or terminal require that
* we restart the screen.
@@ -417,7 +420,6 @@
*rowp = row;
if (colp != NULL)
*colp = col;
- resizeterm(row, col);
return (0);
}
--------------070700000004050800030903--
Home |
Main Index |
Thread Index |
Old Index