[SCM] Samba Shared Repository - branch master updated - b881d2ee78f685aea7ae8b67b3e0fb3c4f5205ed

[Michael Adam]

Hi Derell,

Derrell Lipman wrote:
> On Mon, Oct 27, 2008 at 9:58 AM, Michael Adam <obnox at samba.org> wrote:
> 
> > The branch, master has been updated
> >       via  b881d2ee78f685aea7ae8b67b3e0fb3c4f5205ed (commit)
> >      from  68aa9bd67f9556f608bfb321d593617bd346915f (commit)
> >
> > http://gitweb.samba.org/?p=samba.git;a=shortlog;h=master
> >
> >
> > - Log -----------------------------------------------------------------
> > commit b881d2ee78f685aea7ae8b67b3e0fb3c4f5205ed
> > Author: Michael Adam <obnox at samba.org>
> > Date:   Mon Oct 27 14:28:44 2008 +0100
> >
> >    [s3]winbind: fix smbd hanging on Solaris when winbindd closes socket.
> >
> >    On some versions of Solaris, we observed a strange effect of close(2)
> >    on a socket: After the server (here winbindd) called close, the client
> > fd
> >    was not marked as readable for select. And a write call to the fd did
> >    not produce an error EPIPE but just returned as if successful.
> >
> >    So while winbindd had called remove_client(), the corresponding smbd
> >    still thought that it was connected, but failed to retrieve answers
> >    for its queries.
> >
> >    This patch works around the problem by forcing the client fd to
> >    the readable state: Just write one byte into the socket before
> >    closing.
> >
> >    Michael
> >
> > -----------------------------------------------------------------------
> >
> > Summary of changes:
> >  source3/winbindd/winbindd.c |    7 ++++++-
> >  1 files changed, 6 insertions(+), 1 deletions(-)
> >
> >
> > Changeset truncated at 500 lines:
> >
> > diff --git a/source3/winbindd/winbindd.c b/source3/winbindd/winbindd.c
> > index 5d4f21a..ac2a87f 100644
> > --- a/source3/winbindd/winbindd.c
> > +++ b/source3/winbindd/winbindd.c
> > @@ -727,12 +727,17 @@ static void new_connection(int listen_sock, bool
> > privileged)
> >
> >  static void remove_client(struct winbindd_cli_state *state)
> >  {
> > +       char c = 0;
> > +
> >        /* It's a dead client - hold a funeral */
> >
> >        if (state == NULL) {
> >                return;
> >        }
> > -
> > +
> > +       /* tell client, we are closing ... */
> > +       write(state->sock, &c, sizeof(c));
> > +
> >        /* Close socket */
> >
> >        close(state->sock);
> 
> 
> Michael, although this should help in the majority of cases, it looks like
> it still leaves a race condition where you write the single byte, the peer
> reads that byte so the socket is no longer in a readable state, you then
> close the socket, and the original problem occurs.  I wonder if there is
> some ioctl you can issue here that will force the peer into readable state
> upon close...???

In principle, I agree that this might introduce a race, but in
practice the scenario is as follows:

1. Winbind only closes the socket for an inactive connection
   (this is called from remove_idle_client() or on finished
   connections). 
   
   This means that the client (smbd in our case) expects the
   socket to be readable and winbindd to be waiting for the next
   command on the pipe.

2. On the client end, in nsswitch/wb_common.c:winbind_write_sock(),
   there is already the corresponding catch that will close the
   socket, when it detects (by calling select) that read() would not
   block on the socket (starting in line 387 in nsswitch/wb_common.c).

Does this reduce your concerns?

Cheers - Michael

-- 
Michael Adam <ma at sernet.de>  <obnox at samba.org>
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.SerNet.DE, mailto: Info @ SerNet.DE
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 206 bytes
Desc: not available
Url : http://lists.samba.org/archive/samba-technical/attachments/20081027/a8671349/attachment.bin