Date: Sat, 14 May 2005 23:01:30 -0400 From: Craig Rodrigues <rodrigc@crodrigues.org> To: Xin LI <delphij@freebsd.org> Cc: cvs-all@freebsd.org Subject: Re: cvs commit: src/lib/libc/gen ttyname.c Message-ID: <20050515030130.GA63641@crodrigues.org> In-Reply-To: <200505141403.j4EE3LYX097762@repoman.freebsd.org> References: <200505141403.j4EE3LYX097762@repoman.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20050515030130.GA63641>