making nsswitch(3) thread-safe (was Re: Removal of bogus gethostbyaddr_r())

[Mike Makonnen]

[ Jacques, I'm CC'ing you since this affects nss ]

I just took a look into what it would take to make the gethostby*_r functions
thread safe, and it isn't pretty. The "thread-unsafeness" goes all the way
down into the individual methods for the various nsswitch(3) databases (dns,
files, etc). So, the most expedient and least invasive way to go about it would
be to put a mutex in the gethostby* functions that is dependent on __isthreaded
being true. This, offcourse, means that the entire call-chain (from top to
bottom) is locked for the duration of a call to one of these functions, which
can be a considerable amount of time if it has to timeout. During this time no
other thread in the application can resolve a host because it will be blocked on
said mutex.

To my mind the way to fix this properly is to make the nsdispatch(3)
call-chain in libc thread-safe from the ground-up. This is a huge task in and of
itself, but is complicated even further by the fact that whatever we do can't
change the semantics of the existing user-visible functions.

I think there are three points to keep in mind:

1. The nsswitch(3) database functions should be made thread-safe, but at the
    same time not change the thread-unsafe semantics that third-party apps
    might expect (through nsdispatch(3)).
2. The thread-unsafeness starts at the bottom (low-level functions) of the
    call-chain, in the nsswitch(3) database functions.
3. Because so many databases (hosts, passwd, group, etc) will be affected by
    this it would be too huge a task to do it all at once. The safest approach
    is probably to introduce the changes separately a few at a time.

We can achieve this by
Introducing a variable that gets passed down the call-chain telling the
nsswitch(3) database functions whether the "destination address" (return
value) will by allocated by the caller or not. If this variable is false then
the routines can use a static buffer, otherwise, they will assume the caller has
allocated the necessary space.

Most of the functions will need a lot more work than this to make them
fully thread-safe. The advantage of doing it this way is that it maintains
the status-quo while allowing us to make the individual subsystems
thread-safe separately and as time and resources permit.

The following patch shows what I have in mind. It won't
compile just yet, but it might make what I'm saying a little clearer :-)
This initial phase is really just a mechanical change of argument lists. It
doesn't introduce any thread-safeness on its own. It just makes it easier to
introduce such changes safely on a subsytem by subsystem basis.

Basically, the _nsdispatch() functionality is moved into nsdispatch_common(),
which takes a va_list as its last argument instead of a variable number of
arguments. It also takes one additional argument(int is_r) so callers can tell
it what kind of semantics they want. Applications calling nsdispatch() get a
wrapper that passes a false (0) value in the is_r argument. Callers from within
libc will continue calling_nsdispatch(), which also grows an additional argument
(int is_r) that callers can pass down to the nsswitch(3) database functions
through nsdispatch_common().

What do you think? Should I continue with this or do you think it's bogus?

The important point to keep in mind is that we have to keep the dual semantics
(thread-safe and unsafe) all the way down the call-chain because only the
top-level and low-level functions know the actual memory type and size that
needs to be allocated for the return value to the library users. So, we can't
make the low-level nsswitch(2) database functions unconditionally thread-safe
and restrict the dual semantics to the top-level functions.

Cheers.
-- 
Mike Makonnen  | GPG-KEY: http://www.identd.net/~mtm/mtm.asc
mtm at identd.net | D228 1A6F C64E 120A A1C9  A3AA DAE1 E2AF DBCC 68B9
mtm at FreeBSD.Org| FreeBSD - The Power To Serve

Index: include/nsswitch.h
===================================================================
RCS file: /home/ncvs/src/include/nsswitch.h,v
retrieving revision 1.3
diff -u -r1.3 nsswitch.h
--- include/nsswitch.h	17 Apr 2003 14:14:21 -0000	1.3
+++ include/nsswitch.h	19 Jun 2003 03:13:29 -0000
@@ -104,13 +104,13 @@
 /*
  * ns_dtab `method' function signature.
  */ 
-typedef int (*nss_method)(void *_retval, void *_mdata, va_list _ap);
+typedef int (*nss_method)(void *_retval, void *_mdata, int is_r, va_list _ap);
 
 /*
  * Macro for generating method prototypes.
  */
 #define NSS_METHOD_PROTOTYPE(method) \
-	int method(void *, void *, va_list)
+	int method(void *, void *, int, va_list)
 
 /*
  * ns_dtab - `nsswitch dispatch table'
Index: lib/libc/net/gethostbydns.c
===================================================================
RCS file: /home/ncvs/src/lib/libc/net/gethostbydns.c,v
retrieving revision 1.43
diff -u -r1.43 gethostbydns.c
--- lib/libc/net/gethostbydns.c	1 May 2003 19:03:13 -0000	1.43
+++ lib/libc/net/gethostbydns.c	19 Jun 2003 03:33:43 -0000
@@ -470,7 +470,7 @@
 }
 
 int
-_dns_gethostbyname(void *rval, void *cb_data, va_list ap)
+_dns_gethostbyname(void *rval, void *cb_data, int is_r, va_list ap)
 {
 	const char *name;
 	int af;
@@ -604,7 +604,7 @@
 }
 
 int
-_dns_gethostbyaddr(void *rval, void *cb_data, va_list ap)
+_dns_gethostbyaddr(void *rval, void *cb_data, int is_r, va_list ap)
 {
 	const char *addr;	/* XXX should have been def'd as u_char! */
 	int len, af;
Index: lib/libc/net/nsdispatch.c
===================================================================
RCS file: /home/ncvs/src/lib/libc/net/nsdispatch.c,v
retrieving revision 1.8
diff -u -r1.8 nsdispatch.c
--- lib/libc/net/nsdispatch.c	24 Apr 2003 19:57:31 -0000	1.8
+++ lib/libc/net/nsdispatch.c	19 Jun 2003 03:17:17 -0000
@@ -557,13 +557,41 @@
 }
 
 
-__weak_reference(_nsdispatch, nsdispatch);
+__weak_reference(__nsdispatch, nsdispatch);
+
+int
+__nsdispatch(void *retval, const ns_dtab disp_tab[], const char *database,
+	     const char *method_name, const ns_src defaults[], ...)
+{
+	va_list	ap;
+	int	error;
+
+	va_start(ap, defaults);
+	error = nsdispatch_common(retval, disp_tab, database, method_name,
+			    defaults, 0, ap);
+	va_end(ap);
+	return (error);
+}
 
 int
 _nsdispatch(void *retval, const ns_dtab disp_tab[], const char *database,
-	    const char *method_name, const ns_src defaults[], ...)
+	    const char *method_name, const ns_src defaults[], int is_r, ...)
+{
+	va_list	ap;
+	int	error;
+
+	va_start(ap, is_r);
+	error = nsdispatch_common(retval, disp_tab, database, method_name,
+			    defaults, is_r, ap);
+	va_end(ap);
+	return (error);
+}
+
+int
+nsdispatch_common(void *retval, const ns_dtab disp_tab[], const char *database,
+	    const char *method_name, const ns_src defaults[], int is_r,
+	    va_list ap)
 {
-	va_list		 ap;
 	const ns_dbt	*dbt;
 	const ns_src	*srclist;
 	nss_method	 method;
@@ -597,9 +625,7 @@
 		method = nss_method_lookup(srclist[i].name, database,
 		    method_name, disp_tab, &mdata);
 		if (method != NULL) {
-			va_start(ap, defaults);
-			result = method(retval, mdata, ap);
-			va_end(ap);
+			result = method(retval, mdata, is_r, ap);
 			if (result & (srclist[i].flags))
 				break;
 		}