bin/50092: huge memory leaks in vi(1) when changing screen size

[oku...@flex.phys.tohoku.ac.jp]

>Number: 50092
>Category: bin
>Synopsis: huge memory leaks in vi(1) when changing screen size
>Confidential: no
>Severity: serious
>Priority: high
>Responsible: bin-bug-people
>State: open
>Class: sw-bug
>Submitter-Id: net
>Arrival-Date: Sun Jul 26 10:00:00 +0000 2015
>Originator: Rin Okuyama
>Release: 7.99.20
>Organization:
Department of Physics, Tohoku University
>Environment:
NetBSD XXX 7.99.20 NetBSD 7.99.20 (XXX) #0: Sun Jul 19 18:42:21 JST 2015 root@XXX:XXX amd64
>Description:
The memory consumption of vi(1) grows order of MB by changing the
screen size.
>How-To-Repeat:
Execute vi(1), and change the screen size several times. You will
observe massive increase in the memory consumption.
>Fix:
This is caused by (I) vi(1) itself and underlying (II) curses and
(III) terminfo libraries.

(I) When the screen size is changed, vi(1) calls newterm(3) without
removing the established screen. For this problem, nvi2 [1,2] and
OpenBSD [3,4] provide patches, but unfortunately, we cannot apply
their patches for two reasons. First, they assume that
set_term(NULL) returns the current screen without any side effects.
This is *not* guaranteed by X/Open standards [5], and causes null
pointer dereferences for our implementation [6]. Second, their
patches break vertically splitted windows [2]. I've prepared an
alternative patch derived from nvi-1.79nb16 [7]: use setterm(3) and
resizeterm(3) instead of newterm(3) for reinitializing the screen.

[1] https://github.com/lichray/nvi2/commit/a8c38480adb030a05bbb2aafec6067dd65d8c2eb
[2] https://github.com/lichray/nvi2/commit/879d2ad6dd4a4343eb0a588ebfe637e1c9845bc4
[3] http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/vi/cl/cl_screen.c#rev1.23
[4] http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/vi/cl/cl_term.c#rev1.20
[5] http://pubs.opengroup.org/onlinepubs/7908799/xcurses/set_term.html
[6] http://nxr.netbsd.org/source/xref/src/lib/libcurses/screen.c#46
[7] http://cvsweb.netbsd.org/bsdweb.cgi/src/usr.bin/vi/cl/Attic/cl_screen.c#rev1.7

(II) _cursesi_setterm() does not delete the established terminal
before calling ti_setupterm(3). I've also found other small memory
leaks.

(III) _ti_readterm() uses [cm]alloc(3) without checking whether
buffers have been allocated or not. I've also addressed other small
memory leaks.

After applying this patch, the memory consumption of vi(1) grows
order of 10KB for the first time you change the screen size; there
are possibly other memory leaks. However, it saturates by a few
times of resizing and does not increases infinitely.

I shall add one more comment. If you change the screen size rapidly,
sometimes you get error messages and additional memory consumption.
I guess that this is because a succeeding SIGWINCH interrupts the
reinitialization of screen due to a preceding SIGWINCH. It would be
better to neglect SIGWINCH during the (re)initialization of curses
screen. I'd like to hear your opinion.

--- src/external/bsd/nvi/Makefile.inc.orig 2015-07-26 01:29:59.000000000 +0900
+++ src/external/bsd/nvi/Makefile.inc 2015-07-26 01:30:05.000000000 +0900
@@ -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
--- src/external/bsd/nvi/dist/cl/cl_screen.c.orig 2015-07-25 08:26:48.000000000 +0900
+++ src/external/bsd/nvi/dist/cl/cl_screen.c 2015-07-25 11:50:37.000000000 +0900
@@ -189,6 +189,7 @@
cl_vi_init(SCR *sp)
{
CL_PRIVATE *clp;
+ static int newterm_done = 0;
char *o_cols, *o_lines, *o_term;
const char *ttype;

@@ -249,13 +250,17 @@
* have to specify the terminal type.
*/
errno = 0;
- if (newterm(__UNCONST(ttype), stdout, stdin) == NULL) {
+ if (!newterm_done && newterm(__UNCONST(ttype), stdout, stdin) == NULL) {
if (errno)
msgq(sp, M_SYSERR, "%s", ttype);
else
msgq(sp, M_ERR, "%s: unknown terminal type", ttype);
return (1);
+ } else if (newterm_done) {
+ setterm(__UNCONST(ttype));
+ resizeterm(O_VAL(sp, O_LINES), O_VAL(sp, O_COLUMNS));
}
+ newterm_done = 1;

if (o_term == NULL)
cl_unsetenv(sp, "TERM");
--- src/external/bsd/nvi/dist/cl/cl_term.c.orig 2015-07-25 08:31:16.000000000 +0900
+++ src/external/bsd/nvi/dist/cl/cl_term.c 2015-07-26 01:20:08.000000000 +0900
@@ -417,7 +417,6 @@
*rowp = row;
if (colp != NULL)
*colp = col;
- resizeterm(row, col);
return (0);
}

--- src/lib/libcurses/screen.c.orig 2015-07-25 09:11:42.000000000 +0900
+++ src/lib/libcurses/screen.c 2015-07-26 00:06:12.000000000 +0900
@@ -207,6 +207,9 @@
return new_screen;

error_exit:
+ if (new_screen->term != NULL)
+ (void)del_curterm(new_screen->term);
+ free(new_screen->unget_list);
free(new_screen);
return NULL;
}
@@ -239,7 +242,7 @@
_cursesi_free_keymap(screen->base_keymap);

free(screen->stdbuf);
- screen->stdbuf = NULL;
+ free(screen->unget_list);
if (_cursesi_screen == screen)
_cursesi_screen = NULL;
free(screen);
--- src/lib/libcurses/setterm.c.orig 2015-07-25 09:04:58.000000000 +0900
+++ src/lib/libcurses/setterm.c 2015-07-25 09:25:16.000000000 +0900
@@ -66,6 +66,8 @@
struct winsize win;
char *p;

+ if (screen->term != NULL)
+ (void)del_curterm(screen->term);
if (type[0] == '\0')
type = "xx";
unknown = 0;
--- src/lib/libterminfo/compile.c.orig 2015-07-25 22:09:17.000000000 +0900
+++ src/lib/libterminfo/compile.c 2015-07-25 22:09:58.000000000 +0900
@@ -653,10 +653,10 @@
free(tic->name);
free(tic->alias);
free(tic->desc);
- free(tic->extras.buf);
free(tic->flags.buf);
free(tic->nums.buf);
free(tic->strs.buf);
+ free(tic->extras.buf);
free(tic);
}
}
--- src/lib/libterminfo/curterm.c.orig 2015-07-25 09:13:21.000000000 +0900
+++ src/lib/libterminfo/curterm.c 2015-07-25 22:03:18.000000000 +0900
@@ -134,11 +134,12 @@

if (oterm == NULL)
return ERR;
- free(oterm->_area);
- free(oterm->strs);
- free(oterm->nums);
free(oterm->flags);
+ free(oterm->nums);
+ free(oterm->strs);
+ free(oterm->_area);
free(oterm->_userdefs);
+ free(oterm->_buf);
free(oterm);
return OK;
}
--- src/lib/libterminfo/term.c.orig 2015-07-25 22:17:02.000000000 +0900
+++ src/lib/libterminfo/term.c 2015-07-26 00:26:29.000000000 +0900
@@ -68,20 +68,30 @@
return -1;
}

- term->flags = calloc(TIFLAGMAX + 1, sizeof(char));
- if (term->flags == NULL)
- return -1;
- term->nums = malloc((TINUMMAX + 1) * sizeof(short));
- if (term->nums == NULL)
- return -1;
+ if (term->flags == NULL) {
+ term->flags = calloc(TIFLAGMAX + 1, sizeof(char));
+ if (term->flags == NULL)
+ return -1;
+ } else
+ memset(term->flags, 0, (TIFLAGMAX + 1) * sizeof(char));
+ if (term->nums == NULL) {
+ term->nums = malloc((TINUMMAX + 1) * sizeof(short));
+ if (term->nums == NULL)
+ return -1;
+ }
memset(term->nums, (short)-1, (TINUMMAX + 1) * sizeof(short));
- term->strs = calloc(TISTRMAX + 1, sizeof(char *));
- if (term->strs == NULL)
- return -1;
+ if (term->strs == NULL) {
+ term->strs = calloc(TISTRMAX + 1, sizeof(char *));
+ if (term->strs == NULL)
+ return -1;
+ } else
+ memset(term->strs, 0, (TISTRMAX + 1) * sizeof(char *));
term->_arealen = caplen;
- term->_area = malloc(term->_arealen);
- if (term->_area == NULL)
- return -1;
+ if (term->_area == NULL) {
+ term->_area = malloc(term->_arealen);
+ if (term->_area == NULL)
+ return -1;
+ }
memcpy(term->_area, cap, term->_arealen);

cap = term->_area;
@@ -352,11 +362,11 @@
e = strdup(e); /* So we don't destroy env */
if (e == NULL)
tic = NULL;
- else
+ else {
tic = _ti_compile(e, TIC_WARNING |
TIC_ALIAS | TIC_DESCRIPTION | TIC_EXTRA);
- if (c == NULL && e != NULL)
free(e);
+ }
if (tic != NULL && ticcmp(tic, name) == 0) {
len = _ti_flatten(&f, tic);
if (len != -1) {
--- src/lib/libterminfo/termcap.c.orig 2015-07-25 22:38:49.000000000 +0900
+++ src/lib/libterminfo/termcap.c 2015-07-25 22:39:29.000000000 +0900
@@ -553,8 +553,11 @@
else
len += rl;
p = realloc(info, len);
- if (p == NULL)
+ if (p == NULL) {
+ if (fv)
+ free(val);
return NULL;
+ }
info = p;
}



--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-...@muc.de

[Julian Coleman]

Hi,

> I shall add one more comment. If you change the screen size rapidly,
> sometimes you get error messages and additional memory consumption.
> I guess that this is because a succeeding SIGWINCH interrupts the
> reinitialization of screen due to a preceding SIGWINCH. It would be
> better to neglect SIGWINCH during the (re)initialization of curses
> screen. I'd like to hear your opinion.

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.

Regards,

J

--
My other computer runs NetBSD too - http://www.netbsd.org/

[Julian Coleman]

The following reply was made to PR bin/50092; it has been noted by GNATS.

[Rin Okuyama]

The following reply was made to PR bin/50092; it has been noted by GNATS.

From: Rin Okuyama <oku...@flex.phys.tohoku.ac.jp>
To: Julian Coleman <j...@coris.org.uk>, gnats...@NetBSD.org
Cc:
Subject: Re: bin/50092: huge memory leaks in vi(1) when changing screen size

Date: Tue, 28 Jul 2015 00:10:55 +0900

Thank you for your comment.

> 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).

Exactly.

> 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 think that your suggestion is a right way to deal with the problem.
Theoretically, we can do so by switching signal handlers appropriately.
Let me think for a while about actual implementation.

Except this problem, my patch improves the situation. At least, vi no
longer wastes MB of memory. Other commands using terminfo/curses, less,
more, rogue, tetris, and sysinst work fine. Could anyone please review
and commit it?

Rin