--tJpRIM2aoWGz8Rwb
Content-Type: multipart/mixed; boundary="R6d/YZw7aFPcgCGm"
Content-Disposition: inline


--R6d/YZw7aFPcgCGm
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Nov 14, 2001 at 09:37:44PM -0500, gabriel rosenkoetter wrote:
> Any thoughts?

I always send these emails about five seconds before I should.

The real deal is with revision 1.31, where itojun sez:

  ----------------------------
  revision 1.31
  date: 2000/07/31 18:55:35;  author: itojun;  state: Exp;  lines: +6 -3
  free region got from t_getstr().  we will experience memory leak if
  we call initscr() multiple times (rare, but it's better to be
  pedant).
  ----------------------------

I believe him that this could be a problem when he commited that,
but I'm not so sure it was the right fix, and I'm almost certain
that it's no longer necessary.

The thing is, I'm sure I've used a libcurses newer than July 2000
without seeing this problem, which suggests that we're actually
interested in changes to t_getstr()...

Which brings us much closer to the problem at hand, with a couple of
commits to src/lib/libterm/termcap.c:

  ----------------------------
  revision 1.40
  date: 2001/11/02 18:24:20;  author: christos;  state: Exp;  lines: +35 -22
  PR/10266: Jason R. Thorpe: curses programs totally broken.
  Re-write t_agetstr() so that it does not use realloc so userland
  programs don't break. We now use an internal buffer to keep track
  of the memory we allocate. This changes the api of t_agetstr() to
  take 2 fewer arguments, but there are not many programs that use it.
  Please note that this does not change binary compatibility with the
  previous t_agetstr() since the usage was:
 =20
    char *area, *p;
 =20
    *area =3D NULL;
    t_agetstr(ti, "ic", &area, &p);
    ...
    free(area);
 =20
  Since we don't touch the arguments and free(NULL) is a no-op, nothing
  breaks.
 =20
  Since we don't break binary compatibility there is no reason to bump
  the library's major number, but since we change t_agetstr() I'll bump
  the minor number for good measure.
  ----------------------------
  revision 1.39
  date: 2001/10/31 21:52:17;  author: christos;  state: Exp;  lines: +29 -17
  PR/10266: t_getstr() leaks memory. This PR will stay in feedback
  until the problem gets addressed properly. The following fix
  is a stopgap measure to stop the leaking :-(
 =20
  I fixed the t_getstr() memory leak problem, but that instantly
  revealed a problem in t_agetstr() which is an extremely broken
  interface. It realloc's memory, potentially moving the area where
  it returned pointers into in previous calls. This function needs
  to be removed and or changed. I added a horrible work-around for
  now, but I will revisit the problem shortly. In the meantime nobody
  should be using the t_agetstr() API, and I'll be fixing the rest
  of the programs and or the API when I figure out the best solution...
  This is t_agetstr() is used by:
 =20
    games/hack/hack.termcap.c
    games/larn/io.c
    games/tetris/screen.c
    lib/libterm/termcap.c
    lib/libterm/termcap.h
    lib/libterm/termcap.h
    libexec/getty/main.c
    usr.bin/top/screen.c
    usr.bin/ul/ul.c
  ----------------------------

So, it sounds to me like this memory leak has been fixed further up
the chain by christos, which means that the free()ing in
src/lib/libcurses/getch.c superfluous, as tron noted in revision
1.34. But his removal of the free() was not enough to make the
runtime errors go away (because it didn't make the cp variable go
away, so that there was still a pointer to it when christos's code,
rightly, free()ed it? I'm not sure).

Enclosed is a patch against revision 1.34 that removes the rest of
itojun's test code to check for and free t_getstr() if necessary,
since it's theoretically no longer necessary to free it.

This makes the complaints from vi(1) and robots(6) go away for me.
But I have NOT checked for memory leaks, since I don't quite
understand the circumstances necessary for tickling that problem.

If someone who knows more about what's going on here could look at
this, I'd appreciate it.

(If I don't hear anything in a couple of days, I guess I'll just
send-pr it and be noisy about the fact that it needs to be
double-checked.)

--=20
       ~ g r @ eclipsed.net

--R6d/YZw7aFPcgCGm
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="getch.c.diff"

Index: lib/libcurses/getch.c
===================================================================
RCS file: /cvsroot/basesrc/lib/libcurses/getch.c,v
retrieving revision 1.34
diff -u -r1.34 getch.c
--- getch.c	2001/11/01 16:06:59	1.34
+++ getch.c	2001/11/15 03:04:42
@@ -379,7 +379,6 @@
 	size_t limit;
 	key_entry_t *tmp_key;
 	keymap_t *current;
-	char *cp;
 #ifdef DEBUG
 	int k;
 #endif
@@ -397,8 +396,7 @@
 	for (i = 0; i < num_tcs; i++) {
 		p = entry;
 		limit = 1023;
-		cp = t_getstr(_cursesi_genbuf, tc[i].name, &p, &limit);
-		if (cp != NULL) {
+		if (t_getstr(_cursesi_genbuf,  tc[i].name, &p, &limit) != NULL) {
 			current = base_keymap;	/* always start with
 						 * base keymap. */
 			length = (int) strlen(entry);

--R6d/YZw7aFPcgCGm--

--tJpRIM2aoWGz8Rwb
Content-Type: application/pgp-signature
Content-Disposition: inline

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.0.6 (NetBSD)
Comment: For info see http://www.gnupg.org

iEYEARECAAYFAjvzMfwACgkQ9ehacAz5CRoKrwCgruqOLc+3uvTip+lXR98x+32O
YOcAn1DwlcB24SPuR+usSrjM6f42KRGZ
=q8lS
-----END PGP SIGNATURE-----

--tJpRIM2aoWGz8Rwb--