From owner-cvs-src@FreeBSD.ORG Sun May 15 03:00:58 2005 Return-Path: Delivered-To: cvs-src@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 77A1D16A4CE; Sun, 15 May 2005 03:00:58 +0000 (GMT) Received: from rwcrmhc13.comcast.net (rwcrmhc13.comcast.net [204.127.198.39]) by mx1.FreeBSD.org (Postfix) with ESMTP id B708343D60; Sun, 15 May 2005 03:00:57 +0000 (GMT) (envelope-from rodrigc@crodrigues.org) Received: from h00609772adf0.ne.client2.attbi.com ([66.30.114.143]) by comcast.net (rwcrmhc13) with ESMTP id <2005051503005601500d4996e>; Sun, 15 May 2005 03:00:57 +0000 Received: from h00609772adf0.ne.client2.attbi.com (localhost.127.in-addr.arpa [127.0.0.1])j4F31U6U063866; Sat, 14 May 2005 23:01:30 -0400 (EDT) (envelope-from rodrigc@h00609772adf0.ne.client2.attbi.com) Received: (from rodrigc@localhost)j4F31Uij063865; Sat, 14 May 2005 23:01:30 -0400 (EDT) (envelope-from rodrigc) Date: Sat, 14 May 2005 23:01:30 -0400 From: Craig Rodrigues To: Xin LI Message-ID: <20050515030130.GA63641@crodrigues.org> References: <200505141403.j4EE3LYX097762@repoman.freebsd.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="3MwIy2ne0vdjdPXF" Content-Disposition: inline In-Reply-To: <200505141403.j4EE3LYX097762@repoman.freebsd.org> User-Agent: Mutt/1.5.9i cc: cvs-src@freebsd.org cc: das@freebsd.org cc: src-committers@freebsd.org cc: cvs-all@freebsd.org Subject: Re: cvs commit: src/lib/libc/gen ttyname.c X-BeenThere: cvs-src@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: CVS commit messages for the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 15 May 2005 03:00:58 -0000 --3MwIy2ne0vdjdPXF Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Sat, May 14, 2005 at 02:03:21PM +0000, Xin LI wrote: > delphij 2005-05-14 14:03:21 UTC > > FreeBSD src repository > > Modified files: > lib/libc/gen ttyname.c > Log: > Revert to old ttyname_r behavior that when _ioctl() returns 0 (SUCCEEDED), > return the buffer immediately. This will permit ssh and/or PAM logins > broken by previous commit. > > The (potential) underlying problem is still under investigation. > > Point hat to: me Thanks for committing this. To some extent, I think I deserve a pointy hat too, because my original patch included the "return (EINVAL);" which you had to change to "return 0;" to solve the problem with ssh. I would like to ask a question. I believe that the original code in our ttyname() which attempts to make it thread-safe if linked with -pthread is kind of bogus. If you link code with -pthread (and __isthreaded=1), then in ttyname() in /usr/src/lib/libc/gen/ttyname.c, there is a malloc() called, but there is no corresponding free(). malloc() is never called if __isthreaded=0. This is totally bogus, because ttyname() is not documented to require the caller to call free() on any buffer. So we are introducing a potential memory leak, because ttyname() does not do a malloc if __isthreaded=0, but it does a malloc() without a corresponding free if __isthreaded=1. My opinion is that ttyname() should behave the same way with respect to memory allocation if linked with -pthread or not. ttyname() was never a thread-safe interface, and previous attempts to make it so were well-intentioned, but in my opinion, wrong. Now that we have a standards-compliant ttyname_r() in the tree which is thread-safe, I propose the attached patch. I did not submit this patch originally, because I thought it would be too much, and prevent a standards-compliant ttyname_r() from going in the tree. Now that we have a standards-compliant ttyname_r() in the tree, I'd like to see this potential memory leak in ttyname() plugged. The behavior is non-standard, if linked with -pthread.......these kinds of things are so annoying when you are trying to track down memory leaks. What do you think? -- Craig Rodrigues rodrigc@crodrigues.org --3MwIy2ne0vdjdPXF Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="patch1.txt" Index: ttyname.c =================================================================== RCS file: /home/ncvs/src/lib/libc/gen/ttyname.c,v retrieving revision 1.19 diff -u -r1.19 ttyname.c --- ttyname.c 14 May 2005 14:03:21 -0000 1.19 +++ ttyname.c 15 May 2005 02:53:04 -0000 @@ -57,10 +57,6 @@ static char ttyname_buf[sizeof(_PATH_DEV) + MAXNAMLEN]; -static pthread_mutex_t ttyname_lock = PTHREAD_MUTEX_INITIALIZER; -static pthread_key_t ttyname_key; -static int ttyname_init = 0; - int ttyname_r(int fd, char *buf, size_t len) { @@ -92,39 +88,10 @@ char * ttyname(int fd) { - char *buf; + if (ttyname_r(fd, ttyname_buf, sizeof ttyname_buf) != 0) + return (NULL); + else + return (ttyname_buf); - if (__isthreaded == 0) { - if (ttyname_r(fd, ttyname_buf, sizeof ttyname_buf) != 0) - return (NULL); - else - return (ttyname_buf); - } - - if (ttyname_init == 0) { - _pthread_mutex_lock(&ttyname_lock); - if (ttyname_init == 0) { - if (_pthread_key_create(&ttyname_key, free)) { - _pthread_mutex_unlock(&ttyname_lock); - return (NULL); - } - ttyname_init = 1; - } - _pthread_mutex_unlock(&ttyname_lock); - } - - /* Must have thread specific data field to put data */ - if ((buf = _pthread_getspecific(ttyname_key)) == NULL) { - if ((buf = malloc(sizeof(_PATH_DEV) + MAXNAMLEN)) != NULL) { - if (_pthread_setspecific(ttyname_key, buf) != 0) { - free(buf); - return (NULL); - } - } else { - return (NULL); - } - } - ttyname_r(fd, buf, sizeof(_PATH_DEV) + MAXNAMLEN); - return (buf); } --3MwIy2ne0vdjdPXF--