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

[Stefan Metzmacher]

Am 24.11.2015 um 22:01 schrieb 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).

smbXsrv_session_logoff() sets session->table = NULL;
and should exit immediately the 2nd time.
Am I missing something?

This can only explained if it's NOT called twice,
but the first time AFTER sconn is already gone.

A wild guess, could we have a bug in dbwrap_rbt traverse?
That may skip entries randomly?

> 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 :-).

Yes.

> 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;
>         }
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20151124/9f5e950e/signature.sig>