git: 5245bf7b92b7 - main - lib/libc/net/nsdispatch.c: Fix missing unlock and add locking annotations
Alex Richardson
arichardson at FreeBSD.org
Thu Mar 25 12:07:57 UTC 2021
The branch main has been updated by arichardson:
URL: https://cgit.FreeBSD.org/src/commit/?id=5245bf7b92b74e556527b4916a8deba386fe5772
commit 5245bf7b92b74e556527b4916a8deba386fe5772
Author: Alex Richardson <arichardson at FreeBSD.org>
AuthorDate: 2021-03-25 11:22:10 +0000
Commit: Alex Richardson <arichardson at FreeBSD.org>
CommitDate: 2021-03-25 11:22:10 +0000
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
---
lib/libc/net/nsdispatch.c | 201 +++++++++++++++++++++++++++++-----------------
1 file 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);
}
More information about the dev-commits-src-main
mailing list