[PATCH] Patchset for bug #10344 - SessionLogoff on a signed connection with an outstanding notify request crashes smbd.
Stefan (metze) Metzmacher
metze at samba.org
Tue Mar 4 04:30:52 MST 2014
Hi Jeremy,
I really hate this patches, but I don't have something better currently :-)
here my comments regarding the session case, most of them also apply to the
tcon case.
Please also go to the patchset and check if the comments are nicely
formated.
More comments below.
metze
> Add smbd_smb2_logoff_send()/smbd_smb2_logoff_recv()/smbd_smb2_request_logoff_done()
> and make smbd_smb2_logoff_send() re-entrant. First call to smbd_smb2_logoff_send()
> calls tevent_req_cancel() on any outstanding requests on this session
> then re-schedules itself to be processed after the canceled requests
> finish. It waits 30 seconds, re-scheduling itself until all events
> have been canceled.
>
> [Bug 10344] SessionLogoff on a signed connection with an outstanding notify request crashes smbd.
>
> https://bugzilla.samba.org/show_bug.cgi?id=10344
>
> Signed-off-by: Jeremy Allison <jra at samba.org>
> ---
> source3/smbd/smb2_sesssetup.c | 178 +++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 158 insertions(+), 20 deletions(-)
>
> diff --git a/source3/smbd/smb2_sesssetup.c b/source3/smbd/smb2_sesssetup.c
> index 7dbf603..3763a0c 100644
> --- a/source3/smbd/smb2_sesssetup.c
> +++ b/source3/smbd/smb2_sesssetup.c
> @@ -880,44 +880,182 @@ static bool cancel_outstanding_session_requests(const struct smbd_smb2_request *
> return ret;
> }
>
> +static struct tevent_req *smbd_smb2_logoff_send(TALLOC_CTX *mem_ctx,
> + struct tevent_context *ev,
> + struct smbd_smb2_request *smb2req);
> +static NTSTATUS smbd_smb2_logoff_recv(struct tevent_req *req);
> +static void smbd_smb2_request_logoff_done(struct tevent_req *subreq);
> +
> NTSTATUS smbd_smb2_request_process_logoff(struct smbd_smb2_request *req)
> {
> NTSTATUS status;
> - DATA_BLOB outbody;
> + struct tevent_req *subreq = NULL;
>
> status = smbd_smb2_request_verify_sizes(req, 0x04);
> if (!NT_STATUS_IS_OK(status)) {
> return smbd_smb2_request_error(req, status);
> }
>
> + subreq = smbd_smb2_logoff_send(req, req->sconn->ev_ctx, req);
> + if (subreq == NULL) {
> + return smbd_smb2_request_error(req, NT_STATUS_NO_MEMORY);
> + }
> + tevent_req_set_callback(subreq, smbd_smb2_request_logoff_done, req);
> + /* Ensure we don't reply with an async message on this call -
> + set timeout to 120 secs. */
> + return smbd_smb2_request_pending_queue(req, subreq, 120000000);
> +}
> +
> +static void smbd_smb2_request_logoff_done(struct tevent_req *subreq)
> +{
> + struct smbd_smb2_request *smb2req =
> + tevent_req_callback_data(subreq,
> + struct smbd_smb2_request);
> + DATA_BLOB outbody;
> + NTSTATUS status;
> + NTSTATUS error;
> +
> + status = smbd_smb2_logoff_recv(subreq);
> + TALLOC_FREE(subreq);
> + if (!NT_STATUS_IS_OK(status)) {
> + error = smbd_smb2_request_error(smb2req, status);
> + if (!NT_STATUS_IS_OK(error)) {
> + smbd_server_connection_terminate(smb2req->sconn,
> + nt_errstr(error));
> + return;
> + }
> + return;
> + }
> +
> + outbody = data_blob_talloc(smb2req->out.vector, NULL, 0x04);
> + if (outbody.data == NULL) {
> + error = smbd_smb2_request_error(smb2req, NT_STATUS_NO_MEMORY);
> + if (!NT_STATUS_IS_OK(error)) {
> + smbd_server_connection_terminate(smb2req->sconn,
> + nt_errstr(error));
> + return;
> + }
> + return;
> + }
> +
> + SSVAL(outbody.data, 0x00, 0x04); /* struct size */
> + SSVAL(outbody.data, 0x02, 0); /* reserved */
> +
> + error = smbd_smb2_request_done(smb2req, outbody, NULL);
> + if (!NT_STATUS_IS_OK(error)) {
> + smbd_server_connection_terminate(smb2req->sconn,
> + nt_errstr(error));
> + return;
> + }
> +}
> +
> +struct smbd_smb2_logout_state {
> + struct already_cancelled *acp;
> + struct timeval end_logout;
> +};
> +
> +static struct tevent_req *smbd_smb2_logoff_send(TALLOC_CTX *mem_ctx,
> + struct tevent_context *ev,
> + struct smbd_smb2_request *smb2req)
> +{
> + NTSTATUS status;
> + struct tevent_req *req;
> + struct smbd_smb2_logout_state *state;
> +
> + req = smb2req->subreq;
> + if (req == NULL) {
> + /* New logoff call. */
> + req = tevent_req_create(mem_ctx, &state,
> + struct smbd_smb2_logout_state);
> + if (req == NULL) {
> + return NULL;
This should be the only case where we should return NULL.
> + }
> +
> + state->acp = talloc_zero(state, struct already_cancelled);
> + if (state->acp == NULL) {
please use tevent_req_nomem() and tevent_req_post().
> + return NULL;
> + }
> +
> + /* Don't wait more than 30 seconds for cancel.
> + So remember time now + 30 seconds. If we are
> + re-scheduled after this the cancel requests
> + took too long and we exit. */
Please see README.Coding and reformat the comment.
> + state->end_logout = tevent_timeval_current_ofs(30, 0);
> + } else {
> + /* Re-entrant logoff call - we are being rescheduled
> + after we're requested other requests are canceled. */
> + struct timeval curr = tevent_timeval_current();
> + state = tevent_req_data(req, struct smbd_smb2_logout_state);
> +
> + if (tevent_timeval_compare(&curr, &state->end_logout) >= 0) {
> +
> + /* Ran out of time - terminate. */
> + smbd_server_connection_terminate(smb2req->sconn,
> + nt_errstr(NT_STATUS_DRIVER_CANCEL_TIMEOUT));
> + return NULL;
Please let smbd_smb2_logoff_recv() return an indication that the caller
should
terminate the connection. Maybe bool *is_error.
This is the wrong place.
I'm not sure maybe any non NT_STATUS_OK return from smbd_smb2_logoff_recv()
should terminate the connection?
> + }
> +
> + /* We're re-entrant. We must clear the callback
> + data in case we finish here with tevent_done.
> + If we re-schedule, caller will reset callback
> + correctly. */
Please see README.Coding and reformat the comment.
> + tevent_req_set_callback(req, NULL, NULL);
I really hate this :-) Please make it more clear that this is a really
ugly hack
and nobody should ever learn from this example :-)
> + }
> +
> /*
> - * TODO: cancel all outstanding requests on the session
> + * Try and cancel a request. If we succeed, reschedule ourselves.
> */
> - status = smbXsrv_session_logoff(req->session);
> - if (!NT_STATUS_IS_OK(status)) {
> - DEBUG(0, ("smbd_smb2_request_process_logoff: "
> - "smbXsrv_session_logoff() failed: %s\n",
> - nt_errstr(status)));
> - /*
> - * If we hit this case, there is something completely
> - * wrong, so we better disconnect the transport connection.
> - */
> - return status;
> + if (cancel_outstanding_session_requests(smb2req, state->acp)) {
> + struct tevent_immediate *im = tevent_create_immediate(smb2req);
> + if (!im) {
Please use tevent_req_nomem() and tevent_req_post()
> + return NULL;
> + }
Shouldn't we mark the session as deleted here?
New requests can still use the session and will trigger the same crash
as before.
> + /* Try again later. */
> + tevent_schedule_immediate(im,
> + smb2req->sconn->ev_ctx,
> + smbd_smb2_request_dispatch_immediate,
> + smb2req);
Doesn't this result in a 100% CPU loop? How will any really pending request
e.g. async pread, ever get scheduled?
> + return req;
> + }
> +
> + /*
Please fix the indentation here.
> + * When we get here, we are actually going to tear
> + * down the sesssion pointer.
> + */
> + status = smbXsrv_session_logoff(smb2req->session);
> + if (tevent_req_nterror(req, status)) {
> + DEBUG(0, ("smbXsrv_session_logoff() failed: %s\n",
> + nt_errstr(status)));
> + return tevent_req_post(req, ev);
> }
>
> /*
> * we may need to sign the response, so we need to keep
> * the session until the response is sent to the wire.
> */
> - talloc_steal(req, req->session);
> + talloc_steal(smb2req, smb2req->session);
>
> - outbody = data_blob_talloc(req->out.vector, NULL, 0x04);
> - if (outbody.data == NULL) {
> - return smbd_smb2_request_error(req, NT_STATUS_NO_MEMORY);
> - }
> + /*
> + * The return from smbXsrv_session_logoff()
> + * leaves all outstanding pointers to
> + * smb2req->session not on this request in an
> + * undefined state, ensure we don't try and
> + * access any req->session pointers once
> + * we return.
> + */
> + remove_outstanding_session_refs(smb2req);
>
> - SSVAL(outbody.data, 0x00, 0x04); /* struct size */
> - SSVAL(outbody.data, 0x02, 0); /* reserved */
> + tevent_req_done(req);
> + return tevent_req_post(req, ev);
> +}
> +
> +static NTSTATUS smbd_smb2_logoff_recv(struct tevent_req *req)
> +{
> + NTSTATUS status;
>
> - return smbd_smb2_request_done(req, outbody, NULL);
> + if (tevent_req_is_nterror(req, &status)) {
> + tevent_req_received(req);
> + return status;
> + }
> + tevent_req_received(req);
> + return NT_STATUS_OK;
> }
More information about the samba-technical
mailing list