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

[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 ?

I don't think it makes the code any worse, and does
add a couple of needed torture tests that we previously
got wrong (i.e. didn't do what Windows does).

Cheers,

	Jeremy.