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

[Jeremy Allison]

On Mon, Jun 26, 2017 at 11:26:14PM +0200, Stefan Metzmacher via samba-technical wrote:
> 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:-)

Oh good ! I've been waiting for these to come through.

I'll review asap !

Cheers,

	Jeremy.


> 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
>