lib/libc/net/nsdispatch.c: Fix missing unlock and add locking annotations

The error cases (goto fin) of _nsdispatch were missing the unlock.

This change also drops the checks for __isthreaded since the pthread stubs
are already no-ops if threads are not being used. Dropping those conditionals
allows clang's thread safety analysis to deal with the file and also makes
the code a bit more readable. While touching the file also add a few more
assertions in debug mode that the right locks are held.

Reviewed By:	markj
Differential Revision: https://reviews.freebsd.org/D29372

1 files changed, 129 insertions, 72 deletions

diff --git a/lib/libc/net/nsdispatch.c b/lib/libc/net/nsdispatch.c
index b0f80d079b0b..d504a86d524b 100644
--- a/lib/libc/net/nsdispatch.c
+++ b/lib/libc/net/nsdispatch.c
@@ -69,6 +69,7 @@ __FBSDID("$FreeBSD$");
 #include <sys/param.h>
 #include <sys/stat.h>
 
+#include <assert.h>
 #include <dlfcn.h>
 #include <errno.h>
 #include <fcntl.h>
@@ -76,6 +77,7 @@ __FBSDID("$FreeBSD$");
 #include <nsswitch.h>
 #include <pthread.h>
 #include <pthread_np.h>
+#include <stdatomic.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -97,7 +99,52 @@ enum _nss_constants {
  * Global NSS data structures are mostly read-only, but we update
  * them when we read or re-read the nsswitch.conf.
  */
-static	pthread_rwlock_t	nss_lock = PTHREAD_RWLOCK_INITIALIZER;
+static pthread_rwlock_t nss_lock = PTHREAD_RWLOCK_INITIALIZER;
+#ifndef NDEBUG
+static void *nss_wlock_owner __guarded_by(nss_lock);
+#endif
+
+static inline /* __lock_annotate(locks_excluded(nss_lock)) */
+__locks_exclusive(nss_lock) int
+nss_wlock(void)
+{
+	int err;
+
+	err = _pthread_rwlock_wrlock(&nss_lock);
+#ifndef NDEBUG
+	assert(nss_wlock_owner == NULL);
+	nss_wlock_owner = _pthread_self();
+#endif
+	assert(err == 0 && "Locking should not have failed!");
+	return (err);
+}
+
+/*
+ * XXX: The manpage says already held lock may result in EDEADLK, but
+ * actually libthr returns always EBUSY, so we need the extra owner
+ * variable for assertions.
+ */
+#define ASSERT_NSS_WLOCK_HELD()                                                \
+	do {                                                                   \
+		if (__isthreaded) {                                            \
+			assert(_pthread_rwlock_trywrlock(&nss_lock) == EBUSY); \
+			assert(nss_wlock_owner == _pthread_self());            \
+		}                                                              \
+	} while (0)
+
+static inline __requires_exclusive(nss_lock) __unlocks(nss_lock) int
+nss_wunlock(void)
+{
+	int err;
+
+	ASSERT_NSS_WLOCK_HELD();
+#ifndef NDEBUG
+	nss_wlock_owner = NULL;
+#endif
+	err = _pthread_rwlock_unlock(&nss_lock);
+	assert(err == 0 && "Unlocking should not have failed!");
+	return (err);
+}
 
 /*
  * Runtime determination of whether we are dynamically linked or not.
@@ -114,12 +161,12 @@ const ns_src __nsdefaultsrc[] = {
 };
 
 /* Database, source mappings. */
-static	unsigned int		 _nsmapsize;
-static	ns_dbt			*_nsmap = NULL;
+static	unsigned int		 _nsmapsize __guarded_by(nss_lock);
+static	ns_dbt			*_nsmap __guarded_by(nss_lock);
 
 /* NSS modules. */
-static	unsigned int		 _nsmodsize;
-static	ns_mod			*_nsmod;
+static	unsigned int		 _nsmodsize __guarded_by(nss_lock);
+static	ns_mod			*_nsmod __guarded_by(nss_lock);
 
 /* Placeholder for builtin modules' dlopen `handle'. */
 static	int			 __nss_builtin_handle;
@@ -166,8 +213,7 @@ typedef	int	(*vector_comparison)(const void *, const void *);
 typedef	void	(*vector_free_elem)(void *);
 static	void	  vector_sort(void *, unsigned int, size_t,
 		    vector_comparison);
-static	void	  vector_free(void *, unsigned int *, size_t,
-		    vector_free_elem);
+static	void	  _vector_free(void *, unsigned int, size_t, vector_free_elem);
 static	void	 *vector_ref(unsigned int, void *, unsigned int, size_t);
 static	void	 *vector_search(const void *, void *, unsigned int, size_t,
 		    vector_comparison);
@@ -238,22 +284,24 @@ vector_ref(unsigned int i, void *vec, unsigned int count, size_t esize)
 }
 
 
-#define VECTOR_FREE(v, c, s, f) \
-	do { vector_free(v, c, s, f); v = NULL; } while (0)
+#define VECTOR_FREE(vec, count, es, fe) do {	\
+	_vector_free(vec, count, es, fe);	\
+	vec = NULL;				\
+	count = 0;				\
+} while (0)
 static void
-vector_free(void *vec, unsigned int *count, size_t esize,
+_vector_free(void *vec, unsigned int count, size_t esize,
     vector_free_elem free_elem)
 {
 	unsigned int	 i;
 	void		*elem;
 
-	for (i = 0; i < *count; i++) {
-		elem = vector_ref(i, vec, *count, esize);
+	for (i = 0; i < count; i++) {
+		elem = vector_ref(i, vec, count, esize);
 		if (elem != NULL)
 			free_elem(elem);
 	}
 	free(vec);
-	*count = 0;
 }
 
 /*
@@ -282,13 +330,14 @@ mtab_compare(const void *a, const void *b)
 /*
  * NSS nsmap management.
  */
-void
+__requires_exclusive(nss_lock) void
 _nsdbtaddsrc(ns_dbt *dbt, const ns_src *src)
 {
 	const ns_mod	*modp;
 
-	dbt->srclist = vector_append(src, dbt->srclist, &dbt->srclistsize,
-	    sizeof(*src));
+	ASSERT_NSS_WLOCK_HELD();
+	dbt->srclist = vector_append(src, dbt->srclist,
+	    (unsigned int *)&dbt->srclistsize, sizeof(*src));
 	modp = vector_search(&src->name, _nsmod, _nsmodsize, sizeof(*_nsmod),
 	    string_compare);
 	if (modp == NULL)
@@ -325,26 +374,28 @@ _nsdbtdump(const ns_dbt *dbt)
 }
 #endif
 
+#ifndef NS_REREAD_CONF
+static int __guarded_by(nss_lock) already_initialized = 0;
+#endif
 
 /*
  * The first time nsdispatch is called (during a process's lifetime,
  * or after nsswitch.conf has been updated), nss_configure will
  * prepare global data needed by NSS.
  */
-static int
-nss_configure(void)
+static __requires_shared(nss_lock) int
+__lock_annotate(no_thread_safety_analysis) /* RWLock upgrade not supported. */
+do_nss_configure(void)
 {
-	static time_t	 confmod;
-	static int	 already_initialized = 0;
+	static time_t __guarded_by(nss_lock) confmod = 0;
 	struct stat	 statbuf;
-	int		 result, isthreaded;
+	int		 result;
 	const char	*path;
 #ifdef NS_CACHING
 	void		*handle;
 #endif
 
 	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.
@@ -353,36 +404,33 @@ nss_configure(void)
 	if (path == NULL)
 #endif
 		path = _PATH_NS_CONF;
+
+	result = _pthread_rwlock_unlock(&nss_lock);
+	if (result != 0)
+		return (result);
+	result = nss_wlock();
+	if (result != 0)
+		return (result);
 #ifndef NS_REREAD_CONF
 	/*
-	 * Define NS_REREAD_CONF to have nsswitch notice changes
-	 * to nsswitch.conf(5) during runtime.  This involves calling
-	 * stat(2) every time, which can result in performance hit.
+	 * Another thread could have already run the initialization
+	 * logic while we were waiting for the write lock. Check
+	 * already_initialized to avoid re-initializing.
 	 */
 	if (already_initialized)
-		return (0);
-	already_initialized = 1;
+		goto fin;
 #endif /* NS_REREAD_CONF */
+	ASSERT_NSS_WLOCK_HELD();
 	if (stat(path, &statbuf) != 0)
-		return (0);
+		goto fin;
 	if (statbuf.st_mtime <= confmod)
-		return (0);
-	if (isthreaded) {
-		(void)_pthread_rwlock_unlock(&nss_lock);
-		result = _pthread_rwlock_wrlock(&nss_lock);
-		if (result != 0)
-			return (result);
-		if (stat(path, &statbuf) != 0)
-			goto fin;
-		if (statbuf.st_mtime <= confmod)
-			goto fin;
-	}
+		goto fin;
 	_nsyyin = fopen(path, "re");
 	if (_nsyyin == NULL)
 		goto fin;
-	VECTOR_FREE(_nsmap, &_nsmapsize, sizeof(*_nsmap),
+	VECTOR_FREE(_nsmap, _nsmapsize, sizeof(*_nsmap),
 	    (vector_free_elem)ns_dbt_free);
-	VECTOR_FREE(_nsmod, &_nsmodsize, sizeof(*_nsmod),
+	VECTOR_FREE(_nsmod, _nsmodsize, sizeof(*_nsmod),
 	    (vector_free_elem)ns_mod_free);
 	if (confmod == 0)
 		(void)atexit(nss_atexit);
@@ -400,22 +448,42 @@ nss_configure(void)
 		dlclose(handle);
 	}
 #endif
+#ifndef NS_REREAD_CONF
+	already_initialized = 1;
+#endif
 fin:
-	if (isthreaded) {
-		(void)_pthread_rwlock_unlock(&nss_lock);
-		if (result == 0)
-			result = _pthread_rwlock_rdlock(&nss_lock);
-	}
+	result = nss_wunlock();
+	if (result == 0)
+		result = _pthread_rwlock_rdlock(&nss_lock);
+	return (result);
+}
+
+static __requires_shared(nss_lock) int
+nss_configure(void)
+{
+	int result;
+#ifndef NS_REREAD_CONF
+	/*
+	 * Define NS_REREAD_CONF to have nsswitch notice changes
+	 * to nsswitch.conf(5) during runtime.  This involves calling
+	 * stat(2) every time, which can result in performance hit.
+	 */
+	if (already_initialized)
+		return (0);
+#endif /* NS_REREAD_CONF */
+	result = do_nss_configure();
 	return (result);
 }
 
 
-void
+__requires_exclusive(nss_lock) void
 _nsdbtput(const ns_dbt *dbt)
 {
 	unsigned int	 i;
 	ns_dbt		*p;
 
+	ASSERT_NSS_WLOCK_HELD();
+
 	for (i = 0; i < _nsmapsize; i++) {
 		p = vector_ref(i, _nsmap, _nsmapsize, sizeof(*_nsmap));
 		if (string_compare(&dbt->name, &p->name) == 0) {
@@ -478,13 +546,15 @@ nss_load_builtin_modules(void)
  * argument is non-NULL, assume a built-in module and use reg_fn to
  * register it.  Otherwise, search for a dynamic NSS module.
  */
-static void
+static __requires_exclusive(nss_lock) void
 nss_load_module(const char *source, nss_module_register_fn reg_fn)
 {
 	char		 buf[PATH_MAX];
 	ns_mod		 mod;
 	nss_module_register_fn fn;
 
+	ASSERT_NSS_WLOCK_HELD();
+
 	memset(&mod, 0, sizeof(mod));
 	mod.name = strdup(source);
 	if (mod.name == NULL) {
@@ -548,9 +618,9 @@ fin:
 	vector_sort(_nsmod, _nsmodsize, sizeof(*_nsmod), string_compare);
 }
 
-static int exiting = 0;
+static int exiting __guarded_by(nss_lock) = 0;
 
-static void
+static __requires_exclusive(nss_lock) void
 ns_mod_free(ns_mod *mod)
 {
 
@@ -569,24 +639,19 @@ ns_mod_free(ns_mod *mod)
 static void
 nss_atexit(void)
 {
-	int isthreaded;
-
+	(void)nss_wlock();
 	exiting = 1;
-	isthreaded = __isthreaded;
-	if (isthreaded)
-		(void)_pthread_rwlock_wrlock(&nss_lock);
-	VECTOR_FREE(_nsmap, &_nsmapsize, sizeof(*_nsmap),
+	VECTOR_FREE(_nsmap, _nsmapsize, sizeof(*_nsmap),
 	    (vector_free_elem)ns_dbt_free);
-	VECTOR_FREE(_nsmod, &_nsmodsize, sizeof(*_nsmod),
+	VECTOR_FREE(_nsmod, _nsmapsize, sizeof(*_nsmod),
 	    (vector_free_elem)ns_mod_free);
-	if (isthreaded)
-		(void)_pthread_rwlock_unlock(&nss_lock);
+	(void)nss_wunlock();
 }
 
 /*
  * Finally, the actual implementation.
  */
-static nss_method
+static __requires_shared(nss_lock) nss_method
 nss_method_lookup(const char *source, const char *database,
     const char *method, const ns_dtab disp_tab[], void **mdata)
 {
@@ -625,7 +690,7 @@ fb_endstate(void *p)
 
 __weak_reference(_nsdispatch, nsdispatch);
 
-int
+__requires_unlocked(nss_lock) int
 _nsdispatch(void *retval, const ns_dtab disp_tab[], const char *database,
 	    const char *method_name, const ns_src defaults[], ...)
 {
@@ -634,7 +699,7 @@ _nsdispatch(void *retval, const ns_dtab disp_tab[], const char *database,
 	const ns_src	*srclist;
 	nss_method	 method, fb_method;
 	void		*mdata;
-	int		 isthreaded, serrno, i, result, srclistsize;
+	int		serrno, i, result, srclistsize;
 	struct fb_state	*st;
 	int		 saved_depth;
 
@@ -647,15 +712,8 @@ _nsdispatch(void *retval, const ns_dtab disp_tab[], const char *database,
 	dbt = NULL;
 	fb_method = NULL;
 
-	isthreaded = __isthreaded;
 	serrno = errno;
-	if (isthreaded) {
-		result = _pthread_rwlock_rdlock(&nss_lock);
-		if (result != 0) {
-			result = NS_UNAVAIL;
-			goto fin;
-		}
-	}
+	(void)_pthread_rwlock_rdlock(&nss_lock);
 
 	result = fb_getstate(&st);
 	if (result != 0) {
@@ -774,10 +832,9 @@ _nsdispatch(void *retval, const ns_dtab disp_tab[], const char *database,
 	}
 #endif /* NS_CACHING */
 
-	if (isthreaded)
-		(void)_pthread_rwlock_unlock(&nss_lock);
 	--st->dispatch_depth;
 fin:
+	(void)_pthread_rwlock_unlock(&nss_lock);
 	errno = serrno;
 	return (result);
 }