Date: Mon, 29 Mar 2004 21:02:43 -0600 From: "Jacques A. Vidrine" <nectar@FreeBSD.org> To: Sean McNeil <sean@mcneil.com> Cc: freebsd-current@freebsd.org Subject: Re: nss_ldap broken Message-ID: <20040330030243.GB5757@madman.celabo.org> In-Reply-To: <1080614678.52004.5.camel@server.mcneil.com> References: <1080334840.11426.12.camel@server.mcneil.com> <Pine.GSO.4.10.10403261747480.16871-100000@pcnet5.pcnet.com> <20040330023247.GA5637@madman.celabo.org> <1080614678.52004.5.camel@server.mcneil.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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@celabo.org / jvidrine@verio.net / nectar@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)))
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20040330030243.GB5757>