cvs commit: src/lib/libc/gen ttyname.c

[Craig Rodrigues]

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 at crodrigues.org
-------------- next part --------------
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);
 }