[Patches] s3:smb2_create: avoid reusing the 'tevent_req' within smbd_smb2_create_send() (bug #12832)

[Stefan Metzmacher]

Hi,

here're patches to avoid reusing the tevent_req structure on deferred
SMB2 opens, see
https://bugzilla.samba.org/show_bug.cgi?id=12832

Please review and push:-)

Thanks!
metze
-------------- next part --------------
From 8035a4a58dca9c60227959a9bb1cb40bbb7ca698 Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Fri, 9 Jun 2017 12:30:33 +0200
Subject: [PATCH 1/2] s3:smb2_create: avoid reusing the 'tevent_req' within
 smbd_smb2_create_send()

As the caller ("smbd_smb2_request_process_create()") already sets the callback,
the first time, it's not safe to reuse the tevent_req structure.

The typical 'tevent_req_nterror(); return tevent_req_post()' will
crash as the tevent_req_nterror() already triggered the former callback,
which calls smbd_smb2_create_recv(), were tevent_req_received() invalidates
the tevent_req structure, so that tevent_req_post() will crash.

We just remember the required values from the old state
and move them to the new state.

We tried to write reproducers for this, but sadly weren't able to trigger
the backtrace we had from a create a customer (using recent code)
with commit 6beba782f1bf951236813e0b46115b8102212c03
included. And this patch fixed the situation for the
customer.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=12832

Pair-Programmed-With: Volker Lendecke <vl at samba.org>

Signed-off-by: Stefan Metzmacher <metze at samba.org>
Signed-off-by: Volker Lendecke <vl at samba.org>
---
 source3/smbd/smb2_create.c | 43 +++++++++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/source3/smbd/smb2_create.c b/source3/smbd/smb2_create.c
index 81e0818..346e0a6 100644
--- a/source3/smbd/smb2_create.c
+++ b/source3/smbd/smb2_create.c
@@ -483,35 +483,38 @@ static struct tevent_req *smbd_smb2_create_send(TALLOC_CTX *mem_ctx,
 		requested_oplock_level = in_oplock_level;
 	}
 
-
-	if (smb2req->subreq == NULL) {
-		/* New create call. */
-		req = tevent_req_create(mem_ctx, &state,
+	req = tevent_req_create(mem_ctx, &state,
 				struct smbd_smb2_create_state);
-		if (req == NULL) {
-			return NULL;
-		}
-		state->smb2req = smb2req;
+	if (req == NULL) {
+		return NULL;
+	}
+	state->smb2req = smb2req;
 
-		smb1req = smbd_smb2_fake_smb_request(smb2req);
-		if (tevent_req_nomem(smb1req, req)) {
-			return tevent_req_post(req, ev);
-		}
-		state->smb1req = smb1req;
-		smb2req->subreq = req;
+	smb1req = smbd_smb2_fake_smb_request(smb2req);
+	if (tevent_req_nomem(smb1req, req)) {
+		return tevent_req_post(req, ev);
+	}
+	state->smb1req = smb1req;
+
+	if (smb2req->subreq == NULL) {
 		DEBUG(10,("smbd_smb2_create: name[%s]\n",
 			in_name));
 	} else {
-		/* Re-entrant create call. */
-		req = smb2req->subreq;
-		state = tevent_req_data(req,
-				struct smbd_smb2_create_state);
-		smb1req = state->smb1req;
-		TALLOC_FREE(state->out_context_blobs);
+		struct smbd_smb2_create_state *old_state = tevent_req_data(
+			smb2req->subreq, struct smbd_smb2_create_state);
+
 		DEBUG(10,("smbd_smb2_create_send: reentrant for file %s\n",
 			in_name ));
+
+		state->id = old_state->id;
+		state->request_time = old_state->request_time;
+		state->open_rec = talloc_move(state, &old_state->open_rec);
+		state->open_was_deferred = old_state->open_was_deferred;
 	}
 
+	TALLOC_FREE(smb2req->subreq);
+	smb2req->subreq = req;
+
 	state->out_context_blobs = talloc_zero(state, struct smb2_create_blobs);
 	if (tevent_req_nomem(state->out_context_blobs, req)) {
 		return tevent_req_post(req, ev);
-- 
1.9.1


From 6ca14d8528fb2f55333f1ae13bf18e9d91afe0dc Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Fri, 9 Jun 2017 18:22:19 +0200
Subject: [PATCH 2/2] s3:smb2_create: remove unused timer pointer from
 smbd_smb2_create_state

This finishes commits 4e4376164bafbd3a883b6ce8033dcd714f971d51
and 8da5a0f1e33a85281610700b58b534bc985894f0.

Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
 source3/smbd/smb2_create.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/source3/smbd/smb2_create.c b/source3/smbd/smb2_create.c
index 346e0a6..c4fe247 100644
--- a/source3/smbd/smb2_create.c
+++ b/source3/smbd/smb2_create.c
@@ -431,7 +431,6 @@ struct smbd_smb2_create_state {
 	struct smbd_smb2_request *smb2req;
 	struct smb_request *smb1req;
 	bool open_was_deferred;
-	struct tevent_timer *te;
 	struct tevent_immediate *im;
 	struct timeval request_time;
 	struct file_id id;
@@ -1566,8 +1565,6 @@ static void remove_deferred_open_message_smb2_internal(struct smbd_smb2_request
 		(unsigned long long)mid ));
 
 	state->open_was_deferred = false;
-	/* Ensure we don't have any outstanding timer event. */
-	TALLOC_FREE(state->te);
 	/* Ensure we don't have any outstanding immediate event. */
 	TALLOC_FREE(state->im);
 }
@@ -1635,8 +1632,6 @@ bool schedule_deferred_open_message_smb2(
 		return false;
 	}
 
-	/* Ensure we don't have any outstanding timer event. */
-	TALLOC_FREE(state->te);
 	/* Ensure we don't have any outstanding immediate event. */
 	TALLOC_FREE(state->im);
 
-- 
1.9.1

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20170626/904931eb/signature.sig>