From owner-freebsd-current@FreeBSD.ORG Mon Mar 29 19:02:45 2004 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id A674F16A4CE for ; Mon, 29 Mar 2004 19:02:45 -0800 (PST) Received: from gw.celabo.org (gw.celabo.org [208.42.49.153]) by mx1.FreeBSD.org (Postfix) with ESMTP id D406243D1D for ; Mon, 29 Mar 2004 19:02:44 -0800 (PST) (envelope-from nectar@celabo.org) Received: from madman.celabo.org (madman.celabo.org [10.0.1.111]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client CN "madman.celabo.org", Issuer "celabo.org CA" (not verified)) by gw.celabo.org (Postfix) with ESMTP id 67E365485D; Mon, 29 Mar 2004 21:02:44 -0600 (CST) Received: by madman.celabo.org (Postfix, from userid 1001) id 0AB276D465; Mon, 29 Mar 2004 21:02:44 -0600 (CST) Date: Mon, 29 Mar 2004 21:02:43 -0600 From: "Jacques A. Vidrine" To: Sean McNeil Message-ID: <20040330030243.GB5757@madman.celabo.org> Mail-Followup-To: "Jacques A. Vidrine" , Sean McNeil , Daniel Eischen , freebsd-current@freebsd.org References: <1080334840.11426.12.camel@server.mcneil.com> <20040330023247.GA5637@madman.celabo.org> <1080614678.52004.5.camel@server.mcneil.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1080614678.52004.5.camel@server.mcneil.com> X-Url: http://www.celabo.org/ User-Agent: Mutt/1.5.6i cc: freebsd-current@freebsd.org Subject: Re: nss_ldap broken X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 30 Mar 2004 03:02:45 -0000 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 #include #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)))