From b19aca715a951cd64cdd9f1e4849174b5759860b Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 25 Aug 2011 14:01:01 -0700 Subject: [PATCH 1/2] Fix bug 8407 - SMB2 server can return requests out-of-order when processing a compound request. --- source3/smbd/globals.h | 5 +- source3/smbd/smb2_server.c | 113 +++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 108 insertions(+), 10 deletions(-) diff --git a/source3/smbd/globals.h b/source3/smbd/globals.h index b9bd212..2b150a1 100644 --- a/source3/smbd/globals.h +++ b/source3/smbd/globals.h @@ -296,9 +296,6 @@ NTSTATUS smbd_smb2_request_process_getinfo(struct smbd_smb2_request *req); NTSTATUS smbd_smb2_request_process_setinfo(struct smbd_smb2_request *req); NTSTATUS smbd_smb2_request_process_break(struct smbd_smb2_request *req); NTSTATUS smbd_smb2_request_dispatch(struct smbd_smb2_request *req); -void smbd_smb2_request_dispatch_immediate(struct tevent_context *ctx, - struct tevent_immediate *im, - void *private_data); /* SMB1 -> SMB2 glue. */ void send_break_message_smb2(files_struct *fsp, int level); @@ -355,6 +352,7 @@ struct smbd_smb2_request { bool do_signing; bool async; bool cancelled; + bool dispatched; /* fake smb1 request. */ struct smb_request *smb1req; @@ -605,6 +603,7 @@ struct smbd_server_connection { uint32_t credits_granted; uint32_t max_credits; struct bitmap *credits_bitmap; + bool compound_in_progress; } smb2; }; diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c index 6fc4b5d..fe3f8b0 100644 --- a/source3/smbd/smb2_server.c +++ b/source3/smbd/smb2_server.c @@ -30,6 +30,10 @@ #include "../librpc/gen_ndr/krb5pac.h" #include "auth.h" +static void smbd_smb2_request_dispatch_immediate(struct tevent_context *ctx, + struct tevent_immediate *im, + void *private_data); + #define OUTVEC_ALLOC_SIZE (SMB2_HDR_BODY + 9) static const char *smb2_names[] = { @@ -867,6 +871,10 @@ NTSTATUS smbd_smb2_request_pending_queue(struct smbd_smb2_request *req, * Cancel the outstanding request. */ tevent_req_cancel(req->subreq); + + /* The following call takes care + of the compound_in_progress flag + when it calls smbd_smb2_request_reply(). */ return smbd_smb2_request_error(req, NT_STATUS_INSUFFICIENT_RESOURCES); } @@ -887,6 +895,15 @@ NTSTATUS smbd_smb2_request_pending_queue(struct smbd_smb2_request *req, if (!NT_STATUS_IS_OK(status)) { return status; } + + /* We're splitting off the last SMB2 + request in a compound set, and the + smb2_send_async_interim_response() + call above just sent all the replies + for the previous SMB2 requests in + this compound set. So we're no longer + in the "compound_in_progress" state. */ + req->sconn->smb2.compound_in_progress = false; } /* Don't return an intermediate packet on a pipe read/write. */ @@ -1244,6 +1261,9 @@ NTSTATUS smbd_smb2_request_dispatch(struct smbd_smb2_request *req) uint32_t allowed_flags; NTSTATUS return_value; + /* Ensure we know this is in flight. */ + req->dispatched = true; + inhdr = (const uint8_t *)req->in.vector[i].iov_base; /* TODO: verify more things */ @@ -1723,6 +1743,48 @@ NTSTATUS smbd_smb2_request_dispatch(struct smbd_smb2_request *req) return return_value; } +static struct smbd_smb2_request *smbd_smb2_find_next_request_to_dispatch(struct smbd_server_connection *sconn) +{ + struct smbd_smb2_request *req; + + for (req = sconn->smb2.requests; req; req = req->next) { + /* Return the first undispatched entry on the queue. */ + if (!req->dispatched) { + return req; + } + } + return NULL; +} + +static void smbd_smb2_request_dispatch_from_queue(struct tevent_context *ctx, + struct tevent_immediate *im, + void *private_data) +{ + NTSTATUS status; + struct smbd_server_connection *sconn = talloc_get_type_abort(private_data, + struct smbd_server_connection); + struct smbd_smb2_request *req = smbd_smb2_find_next_request_to_dispatch(sconn); + + TALLOC_FREE(im); + + if (req == NULL) { + /* Someone else already dispatched - ignore. */ + return; + } + + if (DEBUGLEVEL >= 10) { + DEBUG(10,("smbd_smb2_request_dispatch_from_queue: idx[%d] of %d vectors\n", + req->current_idx, req->in.vector_count)); + print_req_vectors(req); + } + + status = smbd_smb2_request_dispatch(req); + if (!NT_STATUS_IS_OK(status)) { + smbd_server_connection_terminate(sconn, nt_errstr(status)); + } +} + + static NTSTATUS smbd_smb2_request_reply(struct smbd_smb2_request *req) { struct tevent_req *subreq; @@ -1745,6 +1807,7 @@ static NTSTATUS smbd_smb2_request_reply(struct smbd_smb2_request *req) if (!im) { return NT_STATUS_NO_MEMORY; } + req->sconn->smb2.compound_in_progress = true; tevent_schedule_immediate(im, req->sconn->smb2.event_ctx, smbd_smb2_request_dispatch_immediate, @@ -1752,6 +1815,10 @@ static NTSTATUS smbd_smb2_request_reply(struct smbd_smb2_request *req) return NT_STATUS_OK; } + /* We're sending out a full PDU reply - compound must + be finished. */ + req->sconn->smb2.compound_in_progress = false; + smb2_setup_nbt_length(req->out.vector, req->out.vector_count); /* Set credit for this operation (zero credits if this @@ -1799,10 +1866,32 @@ static NTSTATUS smbd_smb2_request_reply(struct smbd_smb2_request *req) */ DLIST_REMOVE(req->sconn->smb2.requests, req); + /* If there is an undispatched request on + the queue, schedule it to be dispatched. + We have to do it here in addition to + calling dispatch in smbd_smb2_request_incoming() + because not every incoming SMB2 PDU will + trigger a dispatch, we might have had to + wait for a compound request to finish. + We could just call smbd_smb2_request_dispatch() + but that would lead to a deep call stack. JRA. */ + + req = smbd_smb2_find_next_request_to_dispatch(req->sconn); + if (req) { + struct tevent_immediate *im = tevent_create_immediate(req->sconn); + if (!im) { + return NT_STATUS_NO_MEMORY; + } + tevent_schedule_immediate(im, + req->sconn->smb2.event_ctx, + smbd_smb2_request_dispatch_from_queue, + req->sconn); + } + return NT_STATUS_OK; } -void smbd_smb2_request_dispatch_immediate(struct tevent_context *ctx, +static void smbd_smb2_request_dispatch_immediate(struct tevent_context *ctx, struct tevent_immediate *im, void *private_data) { @@ -2577,13 +2666,23 @@ static void smbd_smb2_request_incoming(struct tevent_req *subreq) return; } - status = smbd_smb2_request_dispatch(req); - if (!NT_STATUS_IS_OK(status)) { - smbd_server_connection_terminate(sconn, nt_errstr(status)); - return; - } - next: + /* Only dispatch a new request if we don't have a compound + request in flight. */ + if (!sconn->smb2.compound_in_progress) { + req = smbd_smb2_find_next_request_to_dispatch(sconn); + if (req) { + status = smbd_smb2_request_dispatch(req); + if (!NT_STATUS_IS_OK(status)) { + smbd_server_connection_terminate(sconn, nt_errstr(status)); + return; + } + } + } else { + DEBUG(10,("smbd_smb2_request_incoming: compound_in_progress - " + "deferring dispatch\n")); + } + status = smbd_smb2_request_next_incoming(sconn); if (!NT_STATUS_IS_OK(status)) { smbd_server_connection_terminate(sconn, nt_errstr(status)); -- 1.7.3.1 From dc11b912f13e862a957499cec7d2557f1a4d13b5 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 25 Aug 2011 15:04:36 -0700 Subject: [PATCH 2/2] Remove the "dispatched" bool from the request struct. Split out the queues into "pending_requests" and "requests". --- source3/smbd/globals.h | 2 +- source3/smbd/smb2_server.c | 34 ++++++++++++++++++---------------- 2 files changed, 19 insertions(+), 17 deletions(-) diff --git a/source3/smbd/globals.h b/source3/smbd/globals.h index 2b150a1..7abbf67 100644 --- a/source3/smbd/globals.h +++ b/source3/smbd/globals.h @@ -352,7 +352,6 @@ struct smbd_smb2_request { bool do_signing; bool async; bool cancelled; - bool dispatched; /* fake smb1 request. */ struct smb_request *smb1req; @@ -598,6 +597,7 @@ struct smbd_server_connection { struct timed_event *brl_timeout; bool blocking_lock_unlock_state; } locks; + struct smbd_smb2_request *pending_requests; struct smbd_smb2_request *requests; uint64_t seqnum_low; uint32_t credits_granted; diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c index fe3f8b0..58230b4 100644 --- a/source3/smbd/smb2_server.c +++ b/source3/smbd/smb2_server.c @@ -606,7 +606,8 @@ static NTSTATUS smbd_smb2_request_setup_out(struct smbd_smb2_request *req) /* setup the length of the NBT packet */ smb2_setup_nbt_length(req->out.vector, req->out.vector_count); - DLIST_ADD_END(req->sconn->smb2.requests, req, struct smbd_smb2_request *); + /* Put it on the pending queue - not dispatched yet. */ + DLIST_ADD_END(req->sconn->smb2.pending_requests, req, struct smbd_smb2_request *); return NT_STATUS_OK; } @@ -1261,9 +1262,6 @@ NTSTATUS smbd_smb2_request_dispatch(struct smbd_smb2_request *req) uint32_t allowed_flags; NTSTATUS return_value; - /* Ensure we know this is in flight. */ - req->dispatched = true; - inhdr = (const uint8_t *)req->in.vector[i].iov_base; /* TODO: verify more things */ @@ -1743,17 +1741,20 @@ NTSTATUS smbd_smb2_request_dispatch(struct smbd_smb2_request *req) return return_value; } -static struct smbd_smb2_request *smbd_smb2_find_next_request_to_dispatch(struct smbd_server_connection *sconn) +static struct smbd_smb2_request *smbd_smb2_get_next_request_to_dispatch(struct smbd_server_connection *sconn) { - struct smbd_smb2_request *req; + struct smbd_smb2_request *req = sconn->smb2.pending_requests; - for (req = sconn->smb2.requests; req; req = req->next) { - /* Return the first undispatched entry on the queue. */ - if (!req->dispatched) { - return req; - } + if (req == NULL) { + return NULL; } - return NULL; + + /* Remove from the pending queue. */ + DLIST_REMOVE(req->sconn->smb2.pending_requests, req); + /* And add to the dispatched queue. */ + DLIST_ADD_END(req->sconn->smb2.requests, req, struct smbd_smb2_request *); + + return req; } static void smbd_smb2_request_dispatch_from_queue(struct tevent_context *ctx, @@ -1763,7 +1764,7 @@ static void smbd_smb2_request_dispatch_from_queue(struct tevent_context *ctx, NTSTATUS status; struct smbd_server_connection *sconn = talloc_get_type_abort(private_data, struct smbd_server_connection); - struct smbd_smb2_request *req = smbd_smb2_find_next_request_to_dispatch(sconn); + struct smbd_smb2_request *req = smbd_smb2_get_next_request_to_dispatch(sconn); TALLOC_FREE(im); @@ -1876,8 +1877,7 @@ static NTSTATUS smbd_smb2_request_reply(struct smbd_smb2_request *req) We could just call smbd_smb2_request_dispatch() but that would lead to a deep call stack. JRA. */ - req = smbd_smb2_find_next_request_to_dispatch(req->sconn); - if (req) { + if (req->sconn->smb2.pending_requests) { struct tevent_immediate *im = tevent_create_immediate(req->sconn); if (!im) { return NT_STATUS_NO_MEMORY; @@ -2611,6 +2611,8 @@ void smbd_smb2_first_negprot(struct smbd_server_connection *sconn, return; } + req = smbd_smb2_get_next_request_to_dispatch(sconn); + status = smbd_smb2_request_dispatch(req); if (!NT_STATUS_IS_OK(status)) { smbd_server_connection_terminate(sconn, nt_errstr(status)); @@ -2670,7 +2672,7 @@ next: /* Only dispatch a new request if we don't have a compound request in flight. */ if (!sconn->smb2.compound_in_progress) { - req = smbd_smb2_find_next_request_to_dispatch(sconn); + req = smbd_smb2_get_next_request_to_dispatch(sconn); if (req) { status = smbd_smb2_request_dispatch(req); if (!NT_STATUS_IS_OK(status)) { -- 1.7.3.1