Re: nss_ldap broken

On Mon, Mar 29, 2004 at 06:44:38PM -0800, Sean McNeil wrote:
> My reasoning behind storing in a local variable was this:
> 
> 1) __isthreaded starts out 0 in those routines.
> 2) lock is called for unthreaded model (i.e. noop?)
> 3) module with pthreads gets loaded.
> 4) unlock is called for threaded model (i.e. real unlock of bogus lock)

I see.  I agree with your logic.  Hmm, hairy: loading a module could
cause e.g. nsdispatch() to be called again :-)  There may be a tiny race
if the /etc/nsswitch.conf is written several times in a row in a short
period, but otherwise I think it OK.

For some reason I'm still really uneasy about a process `suddenly'
becoming threaded.  Must be fear of the unknown: I'm not familiar with
the whole mechanism.

> So I thought that if you didn't actually lock it threaded, then you
> shouldn't unlock it threaded.
> 
> The most simple solution, I guess, is to just not unlock it in the
> nss_atexit.  Since the program is going away it isn't really necessary.

I refuse to not clean up after myself :-)  Besides, if atexit() is not
called, then the module's `unregister' function will never be called.
That function might do something important, e.g. release a system
resource such as a file or shared memory segment.

> I think the more correct solution is the one I proposed.

Thanks!  So, could you try the patch below?  I think it is now
basically identical with what you posted, except for the changes to
nss_compat.c.

Cheers,
-- 
Jacques Vidrine / nectar_at_celabo.org / jvidrine_at_verio.net / nectar_at_freebsd.org


Index: net/nsdispatch.c
===================================================================
RCS file: /home/ncvs/src/lib/libc/net/nsdispatch.c,v
retrieving revision 1.10
diff -c -r1.10 nsdispatch.c
*** net/nsdispatch.c	15 Mar 2004 08:14:35 -0000	1.10
--- net/nsdispatch.c	30 Mar 2004 03:01:04 -0000
***************
*** 316,324 ****
  	static pthread_mutex_t conf_lock = PTHREAD_MUTEX_INITIALIZER;
  	static time_t	 confmod;
  	struct stat	 statbuf;
! 	int		 result;
  	const char	*path;
  
  #if defined(_NSS_DEBUG) && defined(_NSS_SHOOT_FOOT)
  	/* NOTE WELL:  THIS IS A SECURITY HOLE. This must only be built
  	 * for debugging purposes and MUST NEVER be used in production.
--- 316,326 ----
  	static pthread_mutex_t conf_lock = PTHREAD_MUTEX_INITIALIZER;
  	static time_t	 confmod;
  	struct stat	 statbuf;
! 	int		 result, isthreaded;
  	const char	*path;
  
+ 	result = 0;
+ 	isthreaded = __isthreaded;
  #if defined(_NSS_DEBUG) && defined(_NSS_SHOOT_FOOT)
  	/* NOTE WELL:  THIS IS A SECURITY HOLE. This must only be built
  	 * for debugging purposes and MUST NEVER be used in production.
***************
*** 331,346 ****
  		return (0);
  	if (statbuf.st_mtime <= confmod)
  		return (0);
! 	result = _pthread_mutex_trylock(&conf_lock);
! 	if (result != 0)
! 		return (0);
! 	(void)_pthread_rwlock_unlock(&nss_lock);
! 	result = _pthread_rwlock_wrlock(&nss_lock);
! 	if (result != 0)
! 		goto fin2;
  	_nsyyin = fopen(path, "r");
! 	if (_nsyyin == NULL)
  		goto fin;
  	VECTOR_FREE(_nsmap, &_nsmapsize, sizeof(*_nsmap),
  	    (vector_free_elem)ns_dbt_free);
  	VECTOR_FREE(_nsmod, &_nsmodsize, sizeof(*_nsmod),
--- 333,352 ----
  		return (0);
  	if (statbuf.st_mtime <= confmod)
  		return (0);
! 	if (isthreaded) {
! 	    result = _pthread_mutex_trylock(&conf_lock);
! 	    if (result != 0)
! 		    return (0);
! 	    (void)_pthread_rwlock_unlock(&nss_lock);
! 	    result = _pthread_rwlock_wrlock(&nss_lock);
! 	    if (result != 0)
! 		    goto fin2;
! 	}
  	_nsyyin = fopen(path, "r");
! 	if (_nsyyin == NULL) {
! 		result = errno;
  		goto fin;
+ 	}
  	VECTOR_FREE(_nsmap, &_nsmapsize, sizeof(*_nsmap),
  	    (vector_free_elem)ns_dbt_free);
  	VECTOR_FREE(_nsmod, &_nsmodsize, sizeof(*_nsmod),
***************
*** 353,362 ****
  		(void)atexit(nss_atexit);
  	confmod = statbuf.st_mtime;
  fin:
! 	(void)_pthread_rwlock_unlock(&nss_lock);
! 	result = _pthread_rwlock_rdlock(&nss_lock);
  fin2:
! 	(void)_pthread_mutex_unlock(&conf_lock);
  	return (result);
  }
  
--- 359,372 ----
  		(void)atexit(nss_atexit);
  	confmod = statbuf.st_mtime;
  fin:
! 	if (isthreaded) {
! 	    (void)_pthread_rwlock_unlock(&nss_lock);
! 	    if (result == 0)
! 		    result = _pthread_rwlock_rdlock(&nss_lock);
! 	}
  fin2:
! 	if (isthreaded)
! 		(void)_pthread_mutex_unlock(&conf_lock);
  	return (result);
  }
  
***************
*** 510,521 ****
  static void
  nss_atexit(void)
  {
! 	(void)_pthread_rwlock_wrlock(&nss_lock);
  	VECTOR_FREE(_nsmap, &_nsmapsize, sizeof(*_nsmap),
  	    (vector_free_elem)ns_dbt_free);
  	VECTOR_FREE(_nsmod, &_nsmodsize, sizeof(*_nsmod),
  	    (vector_free_elem)ns_mod_free);
! 	(void)_pthread_rwlock_unlock(&nss_lock);
  }
  
  
--- 520,536 ----
  static void
  nss_atexit(void)
  {
! 	int isthreaded;
! 
! 	isthreaded = __isthreaded;
! 	if (isthreaded)
! 		(void)_pthread_rwlock_wrlock(&nss_lock);
  	VECTOR_FREE(_nsmap, &_nsmapsize, sizeof(*_nsmap),
  	    (vector_free_elem)ns_dbt_free);
  	VECTOR_FREE(_nsmod, &_nsmodsize, sizeof(*_nsmod),
  	    (vector_free_elem)ns_mod_free);
! 	if (isthreaded)
! 		(void)_pthread_rwlock_unlock(&nss_lock);
  }
  
  
***************
*** 568,580 ****
  	const ns_src	*srclist;
  	nss_method	 method;
  	void		*mdata;
! 	int		 serrno, i, result, srclistsize;
  
  	serrno = errno;
! 	result = _pthread_rwlock_rdlock(&nss_lock);
! 	if (result != 0) {
! 		result = NS_UNAVAIL;
! 		goto fin;
  	}
  	result = nss_configure();
  	if (result != 0) {
--- 583,598 ----
  	const ns_src	*srclist;
  	nss_method	 method;
  	void		*mdata;
! 	int		 isthreaded, serrno, i, result, srclistsize;
  
+ 	isthreaded = __isthreaded;
  	serrno = errno;
! 	if (isthreaded) {
! 		result = _pthread_rwlock_rdlock(&nss_lock);
! 		if (result != 0) {
! 			result = NS_UNAVAIL;
! 			goto fin;
! 		}
  	}
  	result = nss_configure();
  	if (result != 0) {
***************
*** 604,610 ****
  				break;
  		}
  	}
! 	(void)_pthread_rwlock_unlock(&nss_lock);
  fin:
  	errno = serrno;
  	return (result);
--- 622,629 ----
  				break;
  		}
  	}
! 	if (isthreaded)
! 		(void)_pthread_rwlock_unlock(&nss_lock);
  fin:
  	errno = serrno;
  	return (result);
Index: net/nss_compat.c
===================================================================
RCS file: /home/ncvs/src/lib/libc/net/nss_compat.c,v
retrieving revision 1.2
diff -c -r1.2 nss_compat.c
*** net/nss_compat.c	9 Jan 2004 13:43:49 -0000	1.2
--- net/nss_compat.c	30 Mar 2004 02:16:03 -0000
***************
*** 41,46 ****
--- 41,47 ----
  #include <pthread.h>
  #include <pthread_np.h>
  #include "un-namespace.h"
+ #include "libc_private.h"
  
  
  struct group;
***************
*** 60,66 ****
  
  #define SET_TERMINATOR(x, y)						\
  do {									\
! 	if (_pthread_main_np())						\
  		_term_main_##x = (y);					\
  	else {								\
  		(void)_pthread_once(&_term_once_##x, _term_create_##x);	\
--- 61,67 ----
  
  #define SET_TERMINATOR(x, y)						\
  do {									\
! 	if (!__isthreaded || _pthread_main_np())			\
  		_term_main_##x = (y);					\
  	else {								\
  		(void)_pthread_once(&_term_once_##x, _term_create_##x);	\
***************
*** 69,75 ****
  } while (0)
  
  #define CHECK_TERMINATOR(x)					\
! (_pthread_main_np() ?						\
      (_term_main_##x) :						\
      ((void)_pthread_once(&_term_once_##x, _term_create_##x),	\
      _pthread_getspecific(_term_key_##x)))
--- 70,76 ----
  } while (0)
  
  #define CHECK_TERMINATOR(x)					\
! (!__isthreaded || _pthread_main_np() ?				\
      (_term_main_##x) :						\
      ((void)_pthread_once(&_term_once_##x, _term_create_##x),	\
      _pthread_getspecific(_term_key_##x)))