On Wed, Jun 21, 2006 at 01:02:40PM -0700, jonathan@dsg.stanford.edu wrote:
> 
> In message <20060621190837.GA8936@NetBSD.org>, Brian Ginsbach writes:
> 
> >
> >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().

Hmm, I'd say "nope" is a bit harsh.  I just simplified things a bit
(and over looked that it needs the search arguments passed too).

> 
> _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.

Like I said _compat_pwscan when state->mode is COMPAT_FULL should NOT
call _passwdcompat_pwscan() with _PW_KEYBYNUM -- yes this is what
forces iteration through the entire 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.

It should be as simple as changing the above call to
_passwdcompat_pwscan().

                                rv = _passwdcompat_pwscan(&cpw,
                                    cbuf, sizeof(cbuf),
                                    search, name, uid);

No need for anything fancier because _passwdcompat_pwscan() will
do the right thing and dispatch based on search as you suggest.

> 
> 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.

Yes, the problem can be partially worked around.  If one is not
using the +/- user stuff and only the simple + (include everything)
entry.  But the proper way would be to have nsswitch.conf read

  passwd: files nis