[PATCH] Fix bug 9412 - SMB2 server doesn't support recvfile.

[Jeremy Allison]

On Thu, Apr 04, 2013 at 09:56:16AM +0200, Stefan (metze) Metzmacher wrote:
> 
> Also a helper variable to avoid the function call within the if
> statement would be nice.

Will do.

> I think it would be also better to return an error and correctly
> terminate the connection
> instead of using smb_panic(), it's not unlikely that the connection got
> broken.
> For a broken connection we should terminate gracefully and let the
> client reopen
> durable handles.

Good idea - will do.

> I think we should add some protection to req->sconn->smb2.recv_queue,
> when we do a short read we should place a dummy tevent_req into the
> recv_queue,
> which blocks the socket, and remove it when the socket is drained or
> recvfile is done.
> 
> next to smb1req->unread_bytes we could setup smb2req->recvfile_blocker;
> 
> struct smbd_smb2_recvfile_blocker {
> 	uint8_t dummy;
> };
> 
> static void smbd_smb2_recvfile_blocker_trigger(struct tevent_req *req,
> 					       void *private_data)
> {
> 	return;
> }
> 
> ...
> 	struct smbd_smb2_recvfile_blocker *recvfile_blocker;
> 	smb2req->recvfile_blocker = tevent_req_create(smb2req, &recvfile_blocker,
> 						      smbd_smb2_recvfile_blocker);
> 	if (smb2req->recvfile_blocker == NULL) {
> 		...
> 	}
> 	ok = tevent_queue_add(smb2req->sconn->smb2.recv_queue,
> 			      ...event_context,
> 			      smb2req->recvfile_blocker,
> 			      smbd_smb2_recvfile_blocker_trigger,
> 			      NULL);
> 
> smbd_smb2_request_error_ex() would then also do
> TALLOC_FREE(smb2req->recvfile_blocker);
> after drain_socket() and even if unread_bytes was 0.

Ok, I see the paranoia behind doing it :-), but is it really
neccessary ? Under what circumstances would we need this ?

The SMB1 recvfile code doesn't do this, and if the stream
gets out of sync it's really obvious immediately.

Just seems more change than is needed IMHO, and
greatly increases the complexity of the change.

> > +		} else {
> > +			/*
> > +			 * Either this is a receivefile write so we've
> > +			 * done a short read, or if not we have all the data.
> > +			 * Either way, we're done and the caller will handle
> > +			 * and short read case by looking at the
> > +			 * state->doing_receivefile value.
> > +			 */
> > +			*_vector = NULL;
> > +			*_count = 0;
> > +		}
> 
> Shouldn't the caller take a look at smb1req->unread_bytes?
> state->doing_receivefile is not visible to the caller.

smb1req->unread_bytes isn't set up until all the
smbd_smb2_request_next_vector() calls are finished,
and smbd_smb2_request_read_done() is called.

smbd_smb2_request_read_done() has full access to
struct smbd_smb2_request_read_state *state
and so uses state->doing_receivefile to then set
up smb1req->unread_bytes.

I guess I don't fully understand the comment here.
state->doing_receivefile is a temporary state whilst
we're deciding if we can do a recvfile, and the permanent
state that tells the callers that we are doing a
recveivefile is smb1req->unread_bytes.

> >  	vector[0].iov_base = (void *)state->pktbuf;
> > -	vector[0].iov_len = state->pktlen;
> > +	if (state->min_recv_size &&
> > +			state->pktlen > SMBD_SMB2_SHORT_RECEIVEFILE_WRITE_LEN +
> > +					state->min_recv_size) {
> 
> Can you add a helper variable to get rid of the strange formatting?
> Something like:
> 
> min_recvfile_size = SMBD_SMB2_SHORT_RECEIVEFILE_WRITE_LEN;
> min_recvfile_size += state->min_recv_size;
> if (state->min_recv_size == 0) {
> 	min_recvfile_size = UINT32_MAX;
> }
> if (state->pktlen > min_recvfile_size) {

Will do, no problem.

> > +	if (IVAL(state->pktbuf, 0) == SMB2_TF_MAGIC) {
> > +		/* Transform header. Cannot recvfile. */
> > +		return false;
> > +	}
> > +
> > +	if (IVAL(state->pktbuf, 0) != SMB2_MAGIC) {
> > +		/* Not SMB2. Normal error path will cope. */
> > +		return false;
> > +	}
> > +	if (SVAL(state->pktbuf, 4) != SMB2_HDR_BODY) {
> > +		/* Not SMB2. Normal error path will cope. */
> > +		return false;
> > +	}
> > +	if (IVAL(state->pktbuf, SMB2_HDR_NEXT_COMMAND) != 0) {
> > +		/* Chained. Cannot recvfile. */
> > +		return false;
> > +	}
> > +	if (IVAL(state->pktbuf, SMB2_HDR_FLAGS) &
> > +			(SMB2_HDR_FLAG_CHAINED|
> > +			 SMB2_HDR_FLAG_SIGNED)) {
> > +		/* Signed or chained. Cannot recvfile. */
> > +		return false;
> > +	}
> 
> Can you split this into 2 checks to avoid the strange formatting?

Yes, no problem.

> > +	if (SVAL(state->pktbuf, SMB2_HDR_OPCODE) != SMB2_OP_WRITE) {
> > +		/* Needs to be a WRITE. */
> > +		return false;
> > +	}
> 
> Shouldn't we do this check as the first check?

I'll move it up, but the transform header, SMB2_MAGIC
and SMB2_HDR_BODY length checks have to come first I think :-).

I'll make the changes and re-submit.

Thanks a *lot* for the review !

Jeremy.