[PATCH] Fix bug #11375 - ordering issue in teardown in server exit.

[Jeremy Allison]

On Tue, Nov 24, 2015 at 09:42:00PM +0100, Stefan Metzmacher wrote:
> Hi Jeremy,
> 
> > Fix is pretty self-explainatory.
> >
> > Please review and push if happy !
> 
> This is very similar to https://bugzilla.samba.org/show_bug.cgi?id=11218
> and https://bugzilla.samba.org/show_bug.cgi?id=11394
> 
> I was trying to reproduce this for a long time now,
> without success.
> 
> How do you reproduce this reliable?

I can't. Richard's company was having this
happen 16 times / day.

> I'm sure your fix is able to fix the problem,
> but I don't understand how this can happen.

Yeah, me neither :-(.

> smbXsrv_session_logoff_all() called ealier should
> have removed all sessions already, so the session_table
> should be empty at that stage..., or am I wrong???

smbXsrv_session_logoff_all() calls smbXsrv_session_logoff_all_callback()
which calls smbXsrv_session_clear_and_logoff, which calls smbXsrv_session_logoff().

Inside that we do all the de-initialization,
but we don't free struct smbXsrv_session *session,
so it's still allocated and hanging off client->table.

Then we free global_smbXsrv_client. That calls
the destructor on the struct smbXsrv_session *session
memory allocated off client->table, so we call
smbXsrv_session_destructor(). That invokes
smbXsrv_session_clear_and_logoff(), which calls
smbXsrv_session_logoff() *again*.

So that's how it's getting called twice (and
without the patch client->sconn is now NULL).

The thing I can't figure out, is when
smbXsrv_session_logoff() is called the first
time, it checks session->compat and does
(essentially):

if (session->compat) {
	file_close_user(sconn, session->compat->vuid);
	.... other stuff..
	invalidate_vuid(sconn, session->compat->vuid);
	session->compat = NULL;
}

So the second time it gets called session->compat
should be NULL and the sconn references shouldn't
be made.

But still, this has been confirmed to work by
Richard.

Actually my first patch would also have worked I
think, as session is a child of global_smbXsrv_client->table,
but the logic behind that was less clear (even though
it's more explicit about avoiding any ordering issues
when global_smbXsrv_client talloc children are being
deleted).

The only other way to fix this is the below, but
I thought that was *way* too ugly :-).

Jeremy.

diff --git a/source3/smbd/smbXsrv_session.c b/source3/smbd/smbXsrv_session.c
index c5b7b79..27c5395 100644
--- a/source3/smbd/smbXsrv_session.c
+++ b/source3/smbd/smbXsrv_session.c
@@ -1400,7 +1400,7 @@ NTSTATUS smbXsrv_session_logoff(struct smbXsrv_session *session)
        }
        session->db_rec = NULL;

-       if (session->compat) {
+       if (sconn && session->compat) {
                file_close_user(sconn, session->compat->vuid);
        }

@@ -1419,7 +1419,9 @@ NTSTATUS smbXsrv_session_logoff(struct smbXsrv_session *session)
        }

        if (session->compat) {
-               invalidate_vuid(sconn, session->compat->vuid);
+               if (sconn) {
+                       invalidate_vuid(sconn, session->compat->vuid);
+               }
                session->compat = NULL;
        }