Re: use initgroups_dyn-less nss impls in a thread-safer way

[Alexandre Oliva <aoliva at redhat dot com>]

On Jan 20, 2012, Roland McGrath <roland@hack.frob.com> wrote:

> I don't really understand the issue adequately.

Sorry, I guess I was too deep in the problem to describe it properly.

The problem is that we engage in thread-unsafe behavior in initgroups
compat_call, which we only call for nss modules that don't implement
initgroups_dyn.  In this case, given concurrent calls of getgrouplist,
we may end up stepping on our own toes, iterating over the group entries
supplied by the same nss implementation using the getgrent_r interface
that will hand out some group entries to one thread and other group
entries to the other.

Now, I don't see getgrouplist (or initgroups, for that better)
documented as thread-safe, but if it isn't, then nscd ought to be fixed
to not assume it is.  But if the intention is to make them more (*)
robust to concurrent calls, returning the same results to all threads
that call it concurrently, even in the presence of nss implementations
that don't implement the initgroups_dyn entry point, then we need
something like the patch I proposed, to stop compat_call from skipping
entries because of other threads running compat_call over the same nss
implementation.

(*) it can't be entirely robust for other threads might call
setgrent/getgrent(_r)?/endgrent on their own; if they do, they'd have to
ensure they don't step on each other's toes by means of mutual
exclusion, including calls to getgrouplist and initgroups that might
internally call the corresponding nss *grent* entry points and move
their internal iterators.


Here's a chain of events that demonstrates the problem:

grp/initgroups.c:getgrouplist
 grp/initgroups.c:internal_getgrouplist
  set up __nss_groups_database
  look up nss's initgroups_dyn, not found
   grp/compat-initgroups.c:compat_call
    look up nss's getgrent_r
    look up nss's setgrent, call it
    look up nss's endgrent
    process entries returned by nss's getgrent_r [*]
     
Now, see, if two threads reach [*], each one will get only some of the
entries, because the reentrancy of getgrent_r doesn't imply each thread
gets its own iterator, just that it won't use internal buffers for the
return values.  Further, calls to setgrent and endgrent might interfere
with other threads as well, by moving the internal iterators.

Now, compare with nss/nss_files/files-initgroups.c.  Rather than using
_nss_files_*grent*, that would fail in the same way compat_call does, it
opens its own exclusive copy of /etc/group and goes through it, without
interference from other threads.

> From your description I can't tell what the issue is except for
> catering to some buggy nss modules

Err, do you mean the lack of initgroups_dyn is a bug?  I could live with
that, but then that would be an argument to ditch compat_call
altogether, rather than for maintaining it fragile.  But as long as we
have a fallback, we have to either document the need for external mutual
exclusion (and implement that in nscd), or deal with it internally.

If you ask me, I'd say an ideal API would expose opaque internal nss
iterators, but that would be an nss interface change that, if not
mandatory and accompanied by a flag day, would still require a fallback
like the current compat_call, which IMHO would still call for a fix like
the one I offered.


> On your code itself, there is one missing space before a paren.

Ugh, thanks, fixed.

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer