[PATCH] Version 2: Patchset for bug #10344 - SessionLogoff on a signed connection with an outstanding notify request crashes smbd.
Stefan (metze) Metzmacher
metze at samba.org
Mon Mar 10 07:41:48 MDT 2014
Hi Jeremy,
> Here is a patchset to fix a bug Codenomicon
> scans found in our SMB2/3 implementation.
>
> [Bug 10344] SessionLogoff on a signed connection with an outstanding notify request crashes smbd.
> https://bugzilla.samba.org/show_bug.cgi?id=10344
>
> This is updated based on Metze's feedback on the
> last round, and completely removes the (admittedly
> horrid :-) tevent reentrancy.
Thanks!
> The critical part of the patch is actually in the first
> part:
>
> s3: smbd: SMB2 server. Add a generic wait_queue mechanism.
>
> What this does is adds a mechanism to allow
> one SMB2 request to wait on completion of
> another request, based upon Volker's tevent_wait
> code in source3/lib/tevent_wait.c and used in
> source3/smbd/smb2_close.c.
>
> It's a pretty simple change to the server
> code that just awakens any waiting SMB2 requests
> when the reply from an SMB2 request is removed
> from the processing queue (as its reply data has
> been queued to be sent to the client).
>
> The API inside the SMB2 server looks like this:
>
> NTSTATUS smbd_smb2_wait_for_request_to_complete(
> struct smbd_smb2_request *smb2req_to_awaken,
> struct smbd_smb2_request *smb2req_to_wait_for,
> struct tevent_context *ev,
> void (*callback_fn)(struct tevent_req *),
> void *priv_ptr)
>
> Parameters are:
>
> struct smbd_smb2_request *smb2req_to_awaken - SMB2 request to be awoken.
> struct smbd_smb2_request *smb2req_to_wait_for - SMB2 request to wait for.
> struct tevent_context *ev - Event context.
> void (*callback_fn)(struct tevent_req *) - Callback Function to call
> once request has completed.
> void *priv_ptr - Private pointer passed to
> callback.
I have problems with this API, as it introduces a new style of doing async
programming, which is very confusing.
By now hopefully everyone is used to tevent_req based
_send/recv() function pairs.
Why does it require 'struct smbd_smb2_request *smb2req_to_awaken'
instead of just TALLOC_CTX *mem_ctx, we may want to use this
for the terminate_connection code, which doesn't have a
struct smbd_smb2_request.
> static int smbd_smb2_cleanup_wait_list_entry(struct smb2_request_wait_list *pwe)
> {
> if (pwe->prev) {
> /*
> * This is only the case when the SMB2 request to
> * be awakened has finished and is being freed before
> * the SMB2 request that should awaken us. This should
> * be a very rare occurrence.
> */
> struct smb2_request_wait_list *wl_head;
> DLIST_HEAD(pwe, wl_head);
> DLIST_REMOVE(wl_head, pwe);
This doesn't cleanup smb2req_to_wait_for->wait_list !
wl_head will have the same value as smb2req_to_wait_for->wait_list,
but you have no chance to get the address of
smb2req_to_wait_for->wait_list.
So struct smb2_request_wait_list needs a pointer to smb2req_to_wait_for.
> }
> return 0;
> }
>
> In private email exchanges discussing the earlier
> versions Metze questioned why it only allows waiting for
> one request. The answer is that if you have
> more than one SMB2 request you must wait for,
> the simplest method is just to pick any one
> of them, wait on it, and when awoken re-scan the list
> to see if there are any others remaining. You
> have to wait for them all to complete before
> proceeding anyway, so there is no value in making
> this API more complicated by allowing waiting
> for more than one request. IMHO it's the simplest
> mechanism that will do the job in a generic
> fashion.
I don't see the reason for this looking at the intended users
of this api, logoff, tdis, close and terminate, would be possible users.
And in all cases we should make sure there will be no new requests
added to the list, which make use of the closed session, tcon, open or
connection.
I think it's much easier to just collect all related requests and get
notified
when all are done.
I prepared a patchset that is a bit simpler for my taste...
> This is like the pthread mutex/condition
> variable API, where on being awoken from
> holding a mutex you re-evalutate the condition
> to determine if you can complete or must
> sleep again.
>
> I could also rewrite the code in smb2_close.c
> to use this API, but that's a patch for another
> day I think :-).
>
> The rest of the code is built on top of this,
> and is split into a series of micro commits
> to make it easier to review.
For me they make it harder to review the real change,
because I don't see why a function is added and why
it's written in a specific way.
> Tested clean under valgrind and the additional
> smbtorture smb2.notify tests added in the
> patch.
But not with a full make test :-)
It fails this tests:
[3/23 in 12s, 1 errors] samba3.smb2.lock(s3dc)
Testing cancel by tree disconnect
Acquire first lock
Second lock should pend on first
Disconnect the tree
Check pending lock reply
UNEXPECTED(failure): samba3.smb2.lock.cancel-tdis(s3dc)
REASON: _StringException: _StringException:
../source4/torture/smb2/lock.c:1053: status was NT_STATUS_CANCELLED,
expected NT_STATUS_OK: (../source4/torture/smb2/lock.c:1053)
Testing cancel by ulogoff
Acquire first lock
Second lock should pend on first
Logoff user
Check pending lock reply
UNEXPECTED(failure): samba3.smb2.lock.cancel-logoff(s3dc)
REASON: _StringException: _StringException:
../source4/torture/smb2/lock.c:1140: status was NT_STATUS_CANCELLED,
expected NT_STATUS_OK: (../source4/torture/smb2/lock.c:1140)
In order to make some progress on I hope we can agree on attachments
tmp1.diff and tmp2.diff. And continue to discuss if we take metze.diff
or an updated version of jra.diff.
In jra.diff I squashed related commits together and moved functions
in a better order.
Now a few more comments about jra.diff:
1) the session/tcon is not marked as DELETED before
wait_for_outstanding_requests()
which means following request would be able to use the session/tcon
and the number of pending requests may not decrease.
2) find_outstanding_session_request() and find_outstanding_tcon_request()
both just skip requests with compound_related = true, which means
we don't wait for them to be completed and still have the original
problem.
3) you're not allowed to do anything after tevent_req_nterror()
other then 'return;' or 'return tevent_req_post(req, ev);'
DEBUG() or tevent_req_done(req); is wrong...
4) For think the jump between helper functions like
wait_for_outstanding_requests,
find_outstanding_tcon_request, try_to_tear_down_tcon is a bit confusing.
I think some
find_outstanding_session_request/find_outstanding_tcon_request
would be better inlined.
It's not the typical tevent_req style where all step functions have:
a) the same prefix smbd_smb2_logoff_ or smbd_smb2_tdis_
b) they only return void and the _send() function uses
if (!tevent_req_is_in_progress(req)) {
return tevent_req_post(req, ev);
}
to check the result.
c) they only take (struct tevent_req *req) or (struct tevent_req *subreq)
as arguments.
d) reading from the function from 'struct smbd_smb2_logout_state {' until
the end of smbd_smb2_logoff_recv() should make it almost as easy as to
read a sync function. I hope you see what I mean when looking at
metze.diff.
5) I'd skip the remove_outstanding_session_refs() patch as it will do
nothing in try_to_tear_down_session(), as all requests are finished.
Just leave smbd_smb2_session_setup_state_destructor() alone for now...
I hope you see how much my patch is, others should also have a look please!
If not we have to fix all of the above issues:-)
Apply tmp1.diff
Apply metze.diff or jra.diff
Apply tmp2.diff
One minor comment to tmp2.diff should we change
torture_smb2_notify_ulogoff()
to torture_suite_add_1smb2_test() tree2 is not really needed after your
changes.
metze
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tmp1.diff
Type: text/x-diff
Size: 14452 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140310/588b41f5/attachment.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: jra.diff
Type: text/x-diff
Size: 32220 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140310/588b41f5/attachment-0001.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: metze.diff
Type: text/x-diff
Size: 7924 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140310/588b41f5/attachment-0002.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tmp2.diff
Type: text/x-diff
Size: 5429 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140310/588b41f5/attachment-0003.diff>
More information about the samba-technical
mailing list