Re: ntsec patch #4: passwd and group

[Corinna Vinschen <vinschen at redhat dot com>]

On Fri, Nov 08, 2002 at 10:15:56AM -0500, Pierre A. Humblet wrote:
> The main changes are in the passwd/group emulation code,
> which has been totally redone to cover the four cases.
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
*Shudder*, is that actually needed?  Doesn't fixing single problems help?

> Additionally two new functions have been created:
> getpwsid and getgrsid.

Isn't that functionality already given by cygsid::getfrompw() and
cygsid::getfromgr()?

> There is one design decision: 
>  when the group entry is missing, the code calls 
>  LookupAccountSidA to get the current group name. 
>  That will cause delays for domain users starting 
>  Cygwin while disconnected from the PDC 
>  (but only if /etc/group is incomplete).
>  The alternative is to always use "unknown".

Yeah, we should think a bit about this.  Is that actually a problem?
AFAICS, the LookupAccountSidA() calls are only asking the local box.

> I have also detected 4 outstanding issue:
> 1) memory leak in grp.cc: grp.gr_mem is calloc'ed
>    but never freed.

Oops.

> 2) I thought that the passwd/group files where only
>    read "for the first cygwin process that start up 
>    on a given console", to use Chris' words in
>    http://cygwin.com/ml/cygwin-patches/2002-q4/msg00024.html

I discussed this with Chris in innumerable one-on-ones but we
never found a satisfactory solution for keeping the data just
once in memory.  I can't reiterate right away but every new
idea had a flaw.  I'm still at times thinking about something 
with shared memory but there are as usual security concerns.

> 3) Why is pthread_testcancel called after read_etc_passwd
>    in some (but not all) passwd.cc functions, but never after
>    read_etc_group in grp.cc ?

I don't know.  I'm not familar with pthread.  Robert?

> 4) Altough I see some mutual exclusion stuff, I don't see 
>    what prevents two threads from simultaneously updating
>    the internal copies, or one thread to free them while 
>    they are used by another.

The passwd_lock and group_locks.  Look into the other functions, e.g.:

  getgrnam32 (const char *name)
  {
    if (group_state  <= initializing)
	read_etc_group ();
    ...
  }

That means, if any thread is currently initializing (aka changing)
the group memory, every other thread is forced into read_etc_group()
where the mutex suspends it from execution.

>    In fact applications such as sshd would benefit from 
>    rereading the files (if needed) *before* forks or execs,
>    so that a single reread can serve all future children,
>    but that approach does not help with thread issues.

I don't think it's worth the effort.  The main reason is that
changes to passwd and group files are so seldom...

Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Developer                                mailto:cygwin@cygwin.com
Red Hat, Inc.