[PATCH] Patchset for bug #10344 - SessionLogoff on a signed connection with an outstanding notify request crashes smbd.

[Stefan (metze) Metzmacher]

Am 26.02.2014 17:38, schrieb Jeremy Allison:
> On Wed, Feb 26, 2014 at 07:01:23AM +0100, Stefan (metze) Metzmacher wrote:
>> Hi Jeremy
>>
>>> Finally I've added two tests
>>> that excersise these functions
>>> with pending requests outstanding
>>> in the smb2.notify smbtorture
>>> test.
>>>
>>> I'm pretty happy with this patchset,
>>> as it fixes a longstanding issue.
>>
>> I think we need to solve this a bit more generic,
>> a close on a file handle will also trigger this
>> and smbd_server_connection_terminate()
>>
>> I think we need something like this in
>> smbXsrv_open_close/smbXsrv_tcon_disconnect/smbXsrv_session_logoff:
>>
>> 1) After the ->table == NULL check, we first just set open->status =
>> NT_STATUS_FILE_CLOSED,
>>    tcon->status = NT_STATUS_NETWORK_NAME_DELETED or session->status =
>> NT_STATUS_USER_SESSION_DELETED.
>>    This makes sure they can't be used for new requests.
>>
>> 2a) Check if there're pending (for now only smb2) requests using the
>> specified smbXsrv_open/tcon/session,
>>     if so we call tevent_req_cancel() for them, if not goto step 3.
>>     (later we may need similar checks for smb1 requests).
>>
>> 2b) (optinal) we add a struct db_context *closed_db; to
>> smbXsrv_{open,tcon,session}_table->local
>>     and add the specified open, tcon or session to it with its
>> ->local_id, similar as it's added
>>     to smbXsrv_{open,tcon,session}_table->local->db_ctx.
>>
>> 2c) call smbXsrv_{open,tcon,session}_global_store, so that tool like
>> smbstatus show the correct state.
>>
>> 2d) return
>>
>> 3) continue to remove the open/tcon/session as before...
>>    remember to undo 2b)
>>
>> Then we add smbXsrv_open_remove_unused(struct smbXsrv_open_table *table),
>> smbXsrv_tcon_remove_unused(struct smbXsrv_tcon_table *table) and
>> smbXsrv_session_remove_unused(struct smbXsrv_session_table *table).
>>
>> This traverse smbXsrv_{open,tcon,session}_table->local->db_ctx or
>> if 2b) is implemented smbXsrv_{open,tcon,session}_table->local->closed_db
>> and call smbXsrv_open_close/smbXsrv_tcon_disconnect/smbXsrv_session_logoff
>> again.
>>
>> smbd_smb2_request_dispatch() and smbd_smb2_io_handler() will call
>> smbXsrv_open_remove_unused(),
>> smbXsrv_tcon_remove_unused() and smbXsrv_session_remove_unused() as
>> first action.
>>
>> This way we can directly response to SMB2 logoff, tdis and logoff.
> 
> That's not what Windows does. On logoff and tdis with a
> pending notify Windows returns the notify first, then
> responds to the logoff or tdis. So we need to make
> logoff and tdis async, as a first step (which this patch
> does).
> 
>> In smbd_server_connection_terminate() we need think about how to
>> remove pending requests so that we really remove all opens, tcons and
>> sessions
>> in the first run.
>>
>> As a shortcut we can just remove the pending requests from the list,
>> as smbd_server_connection_terminate() calls exit_server*()
>>
>> But in the long run when we start to implement multichannel support,
>> we may just talloc_free all requests, but we need to
>> be careful regarding the pthreadpool usage, we need to add a way to
>> destroy the state of pending requests, without triggering a segfault
>> when the blocking syscall returns, we need to gracefully terminate the
>> pthread...
> 
> Whilst I agree with some of this (see above), that's a
> completely different patchset than the one I posted
> (it's a rewrite of some core parts of the code :-).
> 
> Right now it looks like neither you nor I have time
> to do the rewrite (unless you're going to surprise
> me in the next day or so :-).
> 
> So I'll take this as a guide for how to move forward, but
> for now to fix the immediate crash bugs can you consider
> reviewing the code as posted to get us working again
> and a fix into the next 4.1.x and 4.0.x ?

If the other mails in this thread doesn't come up with a smaller hack,
I'll have a look :-)

metze