On Jun 21,  1:02pm, jonathan@dsg.stanford.edu (jonathan@dsg.stanford.edu) wrote:
-- Subject: NetBSD-3 NIS-compat getpwnam()/getpwuid  iterate entire map [was 

| 
| In message <20060621190837.GA8936@NetBSD.org>, Brian Ginsbach writes:
| 
| >On Wed, Jun 21, 2006 at 09:42:15AM -0700, jonathan@dsg.stanford.edu wrote:
| >> I'm pretty sure the bug is  the client NIS library. If you look at
| >> 	lib/libc/gen/getpwent.c
| >> 
| >> you will notice that file changed radically between (CVS branches)
| >> netbsd-2 and netbsd-3.  getpwent.c only calls yp_match() or
| >> yp_first()/yp_next() in a couple of places.  Late last night I emailed
| >> you and Stephen and Soda-san a walk through the relevant code-paths. I
| >> think I've identified the problem, and suggested a workaround: don't
| >> use the supplied default nsswitch.conf
| >> 
| >> 	passwd:  compat
| >> 
| >> line, but instead use
| >> 
| >> 	passswd: nis [notfound=return] files
| >> 
| >> which avoids the compat_ parsing routine where I'm pretty this bug
| >> resides.  I also suggested a fix (assuming the workaround does fix
| >> Stephen's problem).
| >> 
| >
| >Where exactly do you see the problem?  I think that _compat_pwscan()
| >is broken.  Specifically the call to _passwdcompat_pwscan() for
| >COMPAT_FULL should probably pass search rather than _PW_KEYBYNUM.
| >Was this your suggested fix?
| 
| 
| Nope. The analysis I emailed to Christos was _compat_pwscan() does not
| meet the contract expected by its callers, as described in the
| function-header comment of _compat_pwscan().
| 
| _compat_pwscan is called to handle "passwd: compat" handling for each
| of getpwent(), getpwnam(), and getpwuid().  Those three callers pass a
| `search' (third-to-last) argument of _PW_KEYBYNUM, _PW_KEYBYNAME, and
| _PW_KEYBYUID, respectively.
| _compat_pwscan is expected to do the appropriate thing. However,
| as part of implementing the "compat" local-passwd redirection for
| 	-
| 	-name
| 	-@netgroup
| 	+
| 	+name
| 	+@netgroup
| 
| entries, _compat_pwscan() uses its compat_state->mode argument to
| determine how to proceed.  In the switch for compat_state->mode ==
| _PW_FULL (a "+" line), _compat_pwscan() is unconditionally calling
| _passwdcompat_pwscan() with _PW_KEYBYNUM.  *THAT* is what forces
| iteration through the entire map, via yp_first()/yp_next(). The
| surrounding state machinery (at the end of the loop; terrible style)
| ensures that when compat_state is set to _PW_FULL, we also call
| _passwdcompat_setpassent(). the for(;;) loop then iterates over the entire compat map.
| 
| Given that analysis, my suggested fix is to rework the code
| 
|                         switch (state->mode) {
| 
|                         case COMPAT_FULL:
|                                         /* get next user */
|                                 rv = _passwdcompat_pwscan(&cpw,
|                                     cbuf, sizeof(cbuf),
|                                     _PW_KEYBYNUM, NULL, 0);
|                                 
| to instead dispatch on the `search' argument, and handle each of
| _PW_KEYBYNUM, _PW_KEYBYNAME, and _PW_KEYBYUID appropriately.
| 
| For the first case, we follow the current code. For the latter two, we
| have the key for passwd.byname or passwd.byuid, respectively, so w can
| just yp_match(), process the entry if found and update the loop state
| to proceed to the next line.

Something like this?

Index: getpwent.c
===================================================================
RCS file: /cvsroot/src/lib/libc/gen/getpwent.c,v
retrieving revision 1.71
diff -u -u -r1.71 getpwent.c
--- getpwent.c	19 Mar 2006 03:05:57 -0000	1.71
+++ getpwent.c	21 Jun 2006 20:15:23 -0000
@@ -1934,9 +1934,22 @@
 
 			case COMPAT_FULL:
 					/* get next user */
-				rv = _passwdcompat_pwscan(&cpw,
-				    cbuf, sizeof(cbuf),
-				    _PW_KEYBYNUM, NULL, 0);
+				switch (search) {
+				case _PW_KEYBYNUM:
+					rv = _passwdcompat_pwscan(&cpw, cbuf,
+					    sizeof(cbuf), search, NULL, 0);
+					break;
+				case _PW_KEYBYNAME:
+					rv = _passwdcompat_pwscan(&cpw, cbuf,
+					    sizeof(cbuf), search, name, 0);
+					break;
+				case _PW_KEYBYUID:
+					rv = _passwdcompat_pwscan(&cpw, cbuf,
+					    sizeof(cbuf), search, NULL, uid);
+					break;
+				default:
+					abort();
+				}
 				if (rv != NS_SUCCESS)
 					state->mode = COMPAT_NONE;
 				break;
| The non-"compat" lines shown in nsswitch.conf(5) don't go through
| _compat_pwscan; so Stephen should be able to work around the
| map-enumeration by reworking his nsswitch.conf to not use the
| "passwd" compat".  Hmm, whilst I typed this, Stephen confirmed that my
| workaround masked the problem.
| 
| 
| Oh... and I noticed a bug in clnt_generic(): for RPC-over-TCP, we
| disable RFC-896 (Nagle) processing for TCP over IPv4, but not for TCP
| over IPv6[1].  Makes a grown man cry.

And something like this?

Index: clnt_generic.c
===================================================================
RCS file: /cvsroot/src/lib/libc/rpc/clnt_generic.c,v
retrieving revision 1.25
diff -u -u -r1.25 clnt_generic.c
--- clnt_generic.c	2 Dec 2005 12:19:16 -0000	1.25
+++ clnt_generic.c	21 Jun 2006 20:19:15 -0000
@@ -333,10 +333,8 @@
 		cl = clnt_vc_create(fd, svcaddr, prog, vers, sendsz, recvsz);
 		if (!nconf || !cl)
 			break;
-		/* XXX fvdl - is this useful? */
-		if (strncmp(nconf->nc_protofmly, "inet", (size_t)4) == 0)
-			setsockopt(fd, IPPROTO_TCP, TCP_NODELAY, &one,
-			    (socklen_t)sizeof (one));
+		setsockopt(fd, IPPROTO_TCP, TCP_NODELAY, &one,
+		    (socklen_t)sizeof (one));
 		break;
 	case NC_TPI_CLTS:
 		cl = clnt_dg_create(fd, svcaddr, prog, vers, sendsz, recvsz);


christos