Attachment 9680 Details for Bug 10442 – Patch for v4-1-test

[patch] Patch for v4-1-test

tmp41.diff (text/plain), 4.86 KB, created by Stefan Metzmacher on 2014-02-14 17:18:11 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Stefan Metzmacher

Created: 2014-02-14 17:18:11 UTC

Size: 4.86 KB

patch

obsolete

>From 4dda9c0a47aceb562e13e4722da5c894693ab5e8 Mon Sep 17 00:00:00 2001
>From: Stefan Metzmacher <metze@samba.org>
>Date: Thu, 30 Jan 2014 16:12:44 +0100
>Subject: [PATCH] s3:smb2_notify: fix use after free on long living notify
> requests
>
>This is a hack, but it should fix the bug:
>
>   change_notify_add_request() talloc moves smb_request away,
>   which is not expected by the smb2_notify.c code...
>
>   smbd_smb2_notify_reply() uses tevent_req_defer_callback()
>   (in older versions an immediate event) to defer the response.
>   This is needed as change_notify_reply() will do more things
>   after calling reply_fn() (smbd_smb2_notify_reply is this case)
>   and often change_notify_remove_request() is called after
>   change_notify_reply().
>
>   change_notify_remove_request() implicitly free's the smb_request
>   that was passed to change_notify_add_request().
>
>   smbd_smb2_fake_smb_request() added the smb_request as smb2req->smb1req,
>   which is expected to be available after smbd_smb2_notify_recv() returned.
>
>The long term solution would be the following interface:
>
>struct tevent_req *change_notify_request_send(TALLOC_CTX *mem_ctx,
>                                              struct tevent_context *ev,
>                                              struct files_struct *fsp,
>                                              uint32_t max_length,
>                                              uint32_t filter,
>                                              bool recursive);
>NTSTATUS change_notify_request_recv(struct tevent_req *req,
>                                    TALLOC_CTX *mem_ctx,
>                                    DATA_BLOB *buffer);
>
>Bug: https://bugzilla.samba.org/show_bug.cgi?id=10442
>
>Signed-off-by: Stefan Metzmacher <metze@samba.org>
>Reviewed-by: Andreas Schneider <asn@samba.org>
>Reviewed-by: Jeremy Allison <jra@samba.org>
>
>Autobuild-User(master): Stefan Metzmacher <metze@samba.org>
>Autobuild-Date(master): Fri Feb 14 11:18:15 CET 2014 on sn-devel-104
>(cherry picked from commit e0bf930f23fe20ee00d0006a5f6c2ba1a8f592a0)
>---
> source3/smbd/smb2_notify.c |   55 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 55 insertions(+)
>
>diff --git a/source3/smbd/smb2_notify.c b/source3/smbd/smb2_notify.c
>index 81aa615..c35acc5 100644
>--- a/source3/smbd/smb2_notify.c
>+++ b/source3/smbd/smb2_notify.c
>@@ -28,6 +28,8 @@
> struct smbd_smb2_notify_state {
> 	struct smbd_smb2_request *smb2req;
> 	struct smb_request *smbreq;
>+	bool has_request;
>+	bool skip_reply;
> 	NTSTATUS status;
> 	DATA_BLOB out_output_buffer;
> };
>@@ -160,6 +162,44 @@ static void smbd_smb2_notify_reply(struct smb_request *smbreq,
> 				   uint8_t *buf, size_t len);
> static bool smbd_smb2_notify_cancel(struct tevent_req *req);
> 
>+static int smbd_smb2_notify_state_destructor(struct smbd_smb2_notify_state *state)
>+{
>+	if (!state->has_request) {
>+		return 0;
>+	}
>+
>+	state->skip_reply = true;
>+	smbd_notify_cancel_by_smbreq(state->smbreq);
>+	return 0;
>+}
>+
>+static int smbd_smb2_notify_smbreq_destructor(struct smb_request *smbreq)
>+{
>+	struct tevent_req *req = talloc_get_type_abort(smbreq->async_priv,
>+						       struct tevent_req);
>+	struct smbd_smb2_notify_state *state = tevent_req_data(req,
>+					       struct smbd_smb2_notify_state);
>+
>+	/*
>+	 * Our temporary parent from change_notify_add_request()
>+	 * goes away.
>+	 */
>+	state->has_request = false;
>+
>+	/*
>+	 * move it back to its original parent,
>+	 * which means we no longer need the destructor
>+	 * to protect it.
>+	 */
>+	talloc_steal(smbreq->smb2req, smbreq);
>+	talloc_set_destructor(smbreq, NULL);
>+
>+	/*
>+	 * We want to keep smbreq!
>+	 */
>+	return -1;
>+}
>+
> static struct tevent_req *smbd_smb2_notify_send(TALLOC_CTX *mem_ctx,
> 						struct tevent_context *ev,
> 						struct smbd_smb2_request *smb2req,
>@@ -183,6 +223,7 @@ static struct tevent_req *smbd_smb2_notify_send(TALLOC_CTX *mem_ctx,
> 	state->smb2req = smb2req;
> 	state->status = NT_STATUS_INTERNAL_ERROR;
> 	state->out_output_buffer = data_blob_null;
>+	talloc_set_destructor(state, smbd_smb2_notify_state_destructor);
> 
> 	DEBUG(10,("smbd_smb2_notify_send: %s - %s\n",
> 		  fsp_str_dbg(fsp), fsp_fnum_dbg(fsp)));
>@@ -266,6 +307,16 @@ static struct tevent_req *smbd_smb2_notify_send(TALLOC_CTX *mem_ctx,
> 		return tevent_req_post(req, ev);
> 	}
> 
>+	/*
>+	 * This is a HACK!
>+	 *
>+	 * change_notify_add_request() talloc_moves()
>+	 * smbreq away from us, so we need a destructor
>+	 * which moves it back at the end.
>+	 */
>+	state->has_request = true;
>+	talloc_set_destructor(smbreq, smbd_smb2_notify_smbreq_destructor);
>+
> 	/* allow this request to be canceled */
> 	tevent_req_set_cancel_fn(req, smbd_smb2_notify_cancel);
> 
>@@ -281,6 +332,10 @@ static void smbd_smb2_notify_reply(struct smb_request *smbreq,
> 	struct smbd_smb2_notify_state *state = tevent_req_data(req,
> 					       struct smbd_smb2_notify_state);
> 
>+	if (state->skip_reply) {
>+		return;
>+	}
>+
> 	state->status = error_code;
> 	if (!NT_STATUS_IS_OK(error_code)) {
> 		/* nothing */
>-- 
>1.7.9.5
>

Flags:

jra: review+
asn: review+

Actions: View

Attachments on bug 10442: 9680 | 9681