RE: [PATCH] Re: pthread -- Corinna?

["Robert Collins" <robert dot collins at itdomain dot com dot au>]

> -----Original Message-----
> From: Christopher Faylor [mailto:cgf@redhat.com]
> Sent: Tuesday, April 17, 2001 12:25 PM
> To: cygwin-patches@cygwin.com; cygwin-xfree@cygwin.com
> Subject: Re: [PATCH] Re: pthread -- Corinna?
> 
> 
> On Tue, Apr 17, 2001 at 07:48:00AM +1000, Robert Collins wrote:
> >----- Original Message -----
> >From: "Christopher Faylor" <cgf@redhat.com>
> >To: <cygwin-xfree@cygwin.com>; <cygwin-patches@cygwin.com>
> >Sent: Tuesday, April 17, 2001 1:20 AM
> >Subject: Re: [PATCH] Re: pthread
> >
> >
> >> On Mon, Apr 16, 2001 at 09:06:27PM +1000, Robert Collins wrote:
> >> >Hi Suhaib,
> >>>here are the two missing functions.  If you aren't setup to compile
> >>>cygwin1.dll let me know and I'll mail you mine.
> >>
> >>You seem to be adding a mutex that is supplanting the passwd_sem
> >>variable.  Shouldn't passwd_sem be eliminated if you are 
> adding this?
> >
> >No.  passwd_sem is not even a real semaphore.  It's simply 
> used to stop
> >security.cc looking calling getpwuid when fopen is called.  
> Passwd_sem
> >as a static variable has almost no overhead on access, a 
> real semaphore
> >or a trylock() on the mutex in security.cc will have a 
> performance hit.
> 
> I'm not convinced that there aren't two diverse mechanisms trying to
> accomplish the same thing here.  Since read_etc_passwd should only be
> called when "passwd_state == uninitialized" I assume that the 
> intent of
> this code is to avoid calling read_etc_passwd more than once.  This
> is already handled by the "passwd_state == uninitialized" test except
> if there is an executed race (don't know if this is the correct term).
> 
> I don't really understand what the passwd_sem code in security.cc is
> trying to do.  It seems to short circuit get_id_from_sid in the case
> where this function is called "simultaneously" from two 
> different threads?
> Or is this function somehow called recursively from getpwent?

I believe the recurse goes (witout function names - off top of head)
fopen->check security->getpwuid->read_etc_password->fopen.

the problem is that 
a) we can't change passwd_state from uninitialised until _after_ the
read completes, or calls to getpwuid et al will think it's been read and
attempt a search.
b) the recursive loop above will deadlock if it comes back into
read_etc_password.

So we need a variable (not passwd_state) to indicate if we are currently
in the protected area of read_etc_passwd.
 
> If that is the case, then the "passwd_sem" variable could be local to
> security.cc, couldn't it?  As it is implemented now, it seems like
> one thread calling getpwent could impact another thread that was
> calling get_id_from_sid.

It needs to be set in read_etc_passwd because if two threads call
1 - getpwnam
2 - fopen(foo)

both calls may result in read_etc_passwd being called, but only one will
enter the mutex via fopen, the other will enter the mutex via
read_etc_passwd. Instant deadlock.

Trylock isn't a solution because then the getpwnam and related functions
will return without actually having read the database.

The current code is thread safe because only one thread enters the
protected code section, and that protected section sets the sem variable
so that fopen won't recurse back into read_etc_passwd. 

A true semaphore could be used, but as the current one is mutex
protected there's no point in extra overhead.

Rob
 
> Corinna?
> 
> cgf
>