NetBSD-Bugs archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
bin/50092: huge memory leaks in vi(1) when changing screen size
>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;
}
Home |
Main Index |
Thread Index |
Old Index