Mailing-List: contact cygwin-developers-help AT cygwin DOT com; run by ezmlm
List-Subscribe: <mailto:cygwin-developers-subscribe AT cygwin DOT com>
List-Archive: <http://sources.redhat.com/ml/cygwin-developers/>
List-Post: <mailto:cygwin-developers AT cygwin DOT com>
List-Help: <mailto:cygwin-developers-help AT cygwin DOT com>, <http://sources.redhat.com/ml/#faqs>
Sender: cygwin-developers-owner AT cygwin DOT com
Delivered-To: mailing list cygwin-developers AT cygwin DOT com
Date: Fri, 8 Nov 2002 17:19:18 +0100
From: Corinna Vinschen <vinschen AT redhat DOT com>
To: "cygwin-developers AT cygwin DOT com" <cygwin-developers AT cygwin DOT com>
Subject: Re: ntsec patch #4: passwd and group
Message-ID: <20021108171918.P21920@cygbert.vinschen.de>
Reply-To: cygwin-developers AT cygwin DOT com
Mail-Followup-To: "cygwin-developers AT cygwin DOT com" <cygwin-developers AT cygwin DOT com>
References: <3DCBD52C DOT A1F794FD AT ieee DOT org>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <3DCBD52C.A1F794FD@ieee.org>
User-Agent: Mutt/1.3.22.1i

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 AT cygwin DOT com
Red Hat, Inc.