netconfig is not threadsafe
Alfred Perlstein
alfred at freebsd.org
Fri Aug 24 20:17:46 UTC 2012
I did a bit of research and this manual page:
http://www.unix.com/man-page/All/3n/setnetconfig/
seems to indicate that this is the best that is "expected"
for thread safety:
MULTITHREAD USAGE
Thread Safe: Yes
Cancel Safe: Yes
Fork Safe: No
Async-cancel Safe: No
Async-signal Safe: No
These functions can be called safely in a multithreaded environment.
They may be cancellation points in that they call functions that are
cancel points.
In a multithreaded environment, these functions are not safe to be
called by a child process after and before These functions should not
be called by a multithreaded application that supports asynchronous
cancellation or asynchronous signals.
So this is awesome. Thank you.
-Alfred
* Alfred Perlstein <alfred at freebsd.org> [120824 13:14] wrote:
> I suppose it can be fixed like this and the idea is pretty sound,
> however I was wondering if you can you look at how Linux/Solaris/OSX
> handle this?
>
> I would think there would be some form of "_r" routines that they
> would use instead. Or maybe thier libc avoids calling these routines
> somehow?
>
> That said this is good work and should go in barring other
> implementations that may make more sense.
>
> -Alfred
>
> * Steven Haber <steven.haber at isilon.com> [120824 13:00] wrote:
> > netconfig (libc routines for reading the netconfig file) is currently
> > not threadsafe. It is a transactional API set, that is, you call
> > setnetconfig() to open the file, read from the file with getnetconfig(),
> > and close/cleanup with endnetconfig(). The problem is that the order
> > that these routines are called in is important, but the state of the
> > current transaction is stored in a per-process static, so only a single
> > thread can do a transaction at one time. A patch from five years ago
> > attempted to fix this:
> >
> >
> >
> > http://lists.freebsd.org/mailman/confirm/freebsd-threads/292f06b38808587
> > 41c9034fb4173a14740c63a9e
> >
> >
> >
> > This patch does not solve the problem. It adds protection for the
> > statics so that concurrent calls into set/get/endnetgrent() don't trash
> > the statics. However, there is no use case where you would want two
> > threads accessing any of these statics concurrently. To solve this
> > problem, I have stored the FILE and netconfig_info statics on a
> > per-thread basis using the pthreads key setup that was already in place
> > for the error code. Now threads have their own set of stored state data,
> > so no statics are necessary. I removed the original locking that had no
> > effect.
> >
> >
> >
> > My repro is to spawn 32 threads that all create an RPC client
> > simultaneously using libc's clnt_create(), which calls into netconfig.
> > After patching, I no longer see the issue. Here is the stack when a
> > thread collision occurs (same as in the original bug from 2007):
> >
> >
> >
> > #0 fclose (fp=0x0) at
> > /root/Source/onefs/git/src/lib/libc/stdio/fclose.c:52
> >
> > #1 0x0000000802c83a79 in endnetconfig (handlep=0x80b1030a0) at
> > /root/Source/onefs/git/src/lib/libc/rpc/getnetconfig.c:433
> >
> > #2 0x0000000802c811d0 in __rpc_endconf (vhandle=0x80b103090) at
> > /root/Source/onefs/git/src/lib/libc/rpc/rpc_generic.c:445
> >
> > #3 0x0000000802c7331a in clnt_create_timed (hostname=0x804d7cd9b
> > "tgroup %s", prog=134420864, vers=1, netclass=Variable "netclass" is not
> > available.
> >
> > ) at /root/Source/onefs/git/src/lib/libc/rpc/clnt_generic.c:271
> >
> >
> >
> > This issue is present in FreeBSD head, though I'm reproducing on 7.1.
> >
> >
> >
> > Does the patch make sense? Should I open a bug on this somehow? I'm
> > relatively new to contributing to FreeBSD.
> >
> >
> >
> > Thanks,
> >
> > Steven
> >
> >
> >
> >
> >
> > >From f3639afbe7993f3c699f23f4d77749ea01e8e8c8 Mon Sep 17 00:00:00 2001
> >
> > From: Steven Haber <shaber at isilon.com>
> >
> > Date: Thu, 23 Aug 2012 17:45:51 -0700
> >
> > Subject: [PATCH] Fix for bug 95757. Make netconfig threadsafe so we can
> > make
> >
> > lots of RPC clients concurrently. The original locking had
> >
> > no effect; there is no point in protecting the static
> >
> > members since you should never have two threads in the same
> >
> > get/set/endnetconfig. The point of these functions is to be
> >
> > called in sequence as a transaction. You can't have
> >
> > multiple threads calling them if the state is stored as a
> >
> > process global. This fix moves the static vars to thread-
> >
> > specific storage so that each thread can have its own
> >
> > transaction, and removes the exiting locking.
> >
> > ---
> >
> > src/lib/libc/rpc/getnetconfig.c | 74
> > +++++++++++----------------------------
> >
> > 1 file changed, 21 insertions(+), 53 deletions(-)
> >
> >
> >
> > diff --git a/src/lib/libc/rpc/getnetconfig.c
> > b/src/lib/libc/rpc/getnetconfig.c
> >
> > index 0f7168f..2957d14 100644
> >
> > --- a/src/lib/libc/rpc/getnetconfig.c
> >
> > +++ b/src/lib/libc/rpc/getnetconfig.c
> >
> > @@ -119,22 +119,21 @@ struct netconfig_vars {
> >
> > struct netconfig_list *nc_configs; /* pointer to the current
> > netconfig entry */
> >
> > };
> >
> > +struct nc_thread_vars {
> >
> > + int nc_error;
> >
> > + FILE *nc_file;
> >
> > + struct netconfig_info nc_ni;
> >
> > +};
> >
> > +
> >
> > #define NC_VALID 0xfeed
> >
> > #define NC_STORAGE 0xf00d
> >
> > #define NC_INVALID 0
> >
> >
> >
> > -static int *__nc_error(void);
> >
> > static int parse_ncp(char *, struct netconfig *);
> >
> > static struct netconfig *dup_ncp(struct netconfig *);
> >
> >
> >
> > -static FILE *nc_file; /* for netconfig db */
> >
> > -static mutex_t nc_file_lock = MUTEX_INITIALIZER;
> >
> > -
> >
> > -static struct netconfig_info ni = { 0, 0, NULL, NULL};
> >
> > -static mutex_t ni_lock = MUTEX_INITIALIZER;
> >
> > -
> >
> > static thread_key_t nc_key;
> >
> > static once_t nc_once = ONCE_INITIALIZER;
> >
> > static int nc_key_error;
> >
> > @@ -148,34 +147,37 @@ nc_key_init(void)
> >
> > #define MAXNETCONFIGLINE 1000
> >
> > -static int *
> >
> > -__nc_error()
> >
> > +static struct nc_thread_vars *
> >
> > +__nc_thread()
> >
> > {
> >
> > - static int nc_error = 0;
> >
> > - int *nc_addr;
> >
> > + static struct nc_thread_vars nc_vars = {0, NULL, {0, 0,
> > NULL, NULL}};
> >
> > + struct nc_thread_vars *nc_addr;
> >
> > /*
> >
> > - * Use the static `nc_error' if we are the main thread
> >
> > + * Use the static `nc_vars' if we are the main thread
> >
> > * (including non-threaded programs), or if an allocation
> >
> > * fails.
> >
> > */
> >
> > if (thr_main())
> >
> > - return (&nc_error);
> >
> > + return (&nc_vars);
> >
> > if (thr_once(&nc_once, nc_key_init) != 0 || nc_key_error
> > != 0)
> >
> > - return (&nc_error);
> >
> > - if ((nc_addr = (int *)thr_getspecific(nc_key)) == NULL) {
> >
> > - nc_addr = (int *)malloc(sizeof (int));
> >
> > + return (&nc_vars);
> >
> > + if ((nc_addr = (struct nc_thread_vars
> > *)thr_getspecific(nc_key))
> >
> > + == NULL) {
> >
> > + nc_addr = (struct nc_thread_vars *)
> >
> > + calloc(1, sizeof(struct
> > nc_thread_vars));
> >
> > if (thr_setspecific(nc_key, (void *)
> > nc_addr) != 0) {
> >
> > if (nc_addr)
> >
> >
> > free(nc_addr);
> >
> > - return (&nc_error);
> >
> > + return (&nc_vars);
> >
> > }
> >
> > - *nc_addr = 0;
> >
> > }
> >
> > return (nc_addr);
> >
> > }
> >
> > -#define nc_error (*(__nc_error()))
> >
> > +#define nc_error (*(&(__nc_thread()->nc_error)))
> >
> > +#define nc_file (*(&(__nc_thread()->nc_file)))
> >
> > +#define ni (*(&(__nc_thread()->nc_ni)))
> >
> > /*
> >
> > * A call to setnetconfig() establishes a /etc/netconfig "session". A
> > session
> >
> > * "handle" is returned on a successful call. At the start of a
> > session (after
> >
> > @@ -209,23 +211,14 @@ setnetconfig()
> >
> > * For multiple calls, i.e. nc_file is not NULL, we just return the
> >
> > * handle without reopening the netconfig db.
> >
> > */
> >
> > - mutex_lock(&ni_lock);
> >
> > ni.ref++;
> >
> > - mutex_unlock(&ni_lock);
> >
> > -
> >
> > - mutex_lock(&nc_file_lock);
> >
> > if ((nc_file != NULL) || (nc_file = fopen(NETCONFIG, "r")) != NULL)
> > {
> >
> > nc_vars->valid = NC_VALID;
> >
> > nc_vars->flag = 0;
> >
> > nc_vars->nc_configs = ni.head;
> >
> > - mutex_unlock(&nc_file_lock);
> >
> > return ((void *)nc_vars);
> >
> > }
> >
> > - mutex_unlock(&nc_file_lock);
> >
> > -
> >
> > - mutex_lock(&ni_lock);
> >
> > ni.ref--;
> >
> > - mutex_unlock(&ni_lock);
> >
> > nc_error = NC_NONETCONFIG;
> >
> > free(nc_vars);
> >
> > @@ -254,13 +247,10 @@ void *handlep;
> >
> > /*
> >
> > * Verify that handle is valid
> >
> > */
> >
> > - mutex_lock(&nc_file_lock);
> >
> > if (ncp == NULL || nc_file == NULL) {
> >
> > nc_error = NC_NOTINIT;
> >
> > - mutex_unlock(&nc_file_lock);
> >
> > return (NULL);
> >
> > }
> >
> > - mutex_unlock(&nc_file_lock);
> >
> > switch (ncp->valid) {
> >
> > case NC_VALID:
> >
> > @@ -274,9 +264,7 @@ void *handlep;
> >
> > */
> >
> > if (ncp->flag == 0) { /* first time */
> >
> > ncp->flag = 1;
> >
> > - mutex_lock(&ni_lock);
> >
> > ncp->nc_configs = ni.head;
> >
> > - mutex_unlock(&ni_lock);
> >
> > if (ncp->nc_configs != NULL) /* entry already exist
> > */
> >
> > return(ncp->nc_configs->ncp);
> >
> > }
> >
> > @@ -289,12 +277,9 @@ void *handlep;
> >
> > * If we cannot find the entry in the list and is end of
> > file,
> >
> > * we give up.
> >
> > */
> >
> > - mutex_lock(&ni_lock);
> >
> > if (ni.eof == 1) {
> >
> > - mutex_unlock(&ni_lock);
> >
> > return(NULL);
> >
> > }
> >
> > - mutex_unlock(&ni_lock);
> >
> > break;
> >
> > default:
> >
> > @@ -316,18 +301,13 @@ void *handlep;
> >
> > /*
> >
> > * Read a line from netconfig file.
> >
> > */
> >
> > - mutex_lock(&nc_file_lock);
> >
> > do {
> >
> > if (fgets(stringp, MAXNETCONFIGLINE, nc_file) == NULL) {
> >
> > free(stringp);
> >
> > - mutex_lock(&ni_lock);
> >
> > ni.eof = 1;
> >
> > - mutex_unlock(&ni_lock);
> >
> > - mutex_unlock(&nc_file_lock);
> >
> > return (NULL);
> >
> > }
> >
> > } while (*stringp == '#');
> >
> > - mutex_unlock(&nc_file_lock);
> >
> > list = (struct netconfig_list *) malloc(sizeof (struct
> > netconfig_list));
> >
> > if (list == NULL) {
> >
> > @@ -357,7 +337,6 @@ void *handlep;
> >
> > * Reposition the current pointer of the handle to the
> > last entry
> >
> > * in the list.
> >
> > */
> >
> > - mutex_lock(&ni_lock);
> >
> > if (ni.head == NULL) { /* first entry */
> >
> > ni.head = ni.tail = list;
> >
> > }
> >
> > @@ -367,7 +346,6 @@ void *handlep;
> >
> > }
> >
> > ncp->nc_configs = ni.tail;
> >
> > result = ni.tail->ncp;
> >
> > - mutex_unlock(&ni_lock);
> >
> > return(result);
> >
> > }
> >
> > }
> >
> > @@ -402,9 +380,7 @@ void *handlep;
> >
> > nc_handlep->valid = NC_INVALID;
> >
> > nc_handlep->flag = 0;
> >
> > nc_handlep->nc_configs = NULL;
> >
> > - mutex_lock(&ni_lock);
> >
> > if (--ni.ref > 0) {
> >
> > - mutex_unlock(&ni_lock);
> >
> > free(nc_handlep);
> >
> > return(0);
> >
> > }
> >
> > @@ -417,7 +393,6 @@ void *handlep;
> >
> > ni.eof = ni.ref = 0;
> >
> > ni.head = NULL;
> >
> > ni.tail = NULL;
> >
> > - mutex_unlock(&ni_lock);
> >
> > while (q != NULL) {
> >
> > p = q->next;
> >
> > @@ -429,10 +404,8 @@ void *handlep;
> >
> > }
> >
> > free(nc_handlep);
> >
> > - mutex_lock(&nc_file_lock);
> >
> > fclose(nc_file);
> >
> > nc_file = NULL;
> >
> > - mutex_unlock(&nc_file_lock);
> >
> > return (0);
> >
> > }
> >
> > @@ -481,21 +454,16 @@ getnetconfigent(netid)
> >
> > * If all the netconfig db has been read and placed into the list
> > and
> >
> > * there is no match for the netid, return NULL.
> >
> > */
> >
> > - mutex_lock(&ni_lock);
> >
> > if (ni.head != NULL) {
> >
> > for (list = ni.head; list; list = list->next) {
> >
> > if (strcmp(list->ncp->nc_netid, netid) == 0) {
> >
> > - mutex_unlock(&ni_lock);
> >
> > return(dup_ncp(list->ncp));
> >
> > }
> >
> > }
> >
> > if (ni.eof == 1) { /* that's all the entries */
> >
> > - mutex_unlock(&ni_lock);
> >
> > return(NULL);
> >
> > }
> >
> > }
> >
> > - mutex_unlock(&ni_lock);
> >
> > -
> >
> > if ((file = fopen(NETCONFIG, "r")) == NULL) {
> >
> > nc_error = NC_NONETCONFIG;
> >
> > --
> >
> > 1.7.9.5
> >
> >
> >
> > _______________________________________________
> > freebsd-threads at freebsd.org mailing list
> > http://lists.freebsd.org/mailman/listinfo/freebsd-threads
> > To unsubscribe, send any mail to "freebsd-threads-unsubscribe at freebsd.org"
>
> --
> - Alfred Perlstein
> .- VMOA #5191, 03 vmax, 92 gs500, 85 ch250, 07 zx10
> .- FreeBSD committer
> _______________________________________________
> freebsd-threads at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-threads
> To unsubscribe, send any mail to "freebsd-threads-unsubscribe at freebsd.org"
--
- Alfred Perlstein
.- VMOA #5191, 03 vmax, 92 gs500, 85 ch250, 07 zx10
.- FreeBSD committer
More information about the freebsd-threads
mailing list