Padding byte in cifs readx response

Christof Schmitt cs at samba.org
Thu Aug 7 17:16:45 MDT 2014


On Thu, Aug 07, 2014 at 04:43:49PM +0200, Volker Lendecke wrote:
> On Wed, Aug 06, 2014 at 02:38:39PM -0700, Christof Schmitt wrote:
> > Here is some progress. I still get a selftest failure with a large read
> > request, so something must be wrong with the outbuf, but the tests i
> > added succeed.  I would like to get some feedback if my patches are the
> > right approach, or if adding the padding byte can be simplified.
> 
> Find some comments attached. My main comment is: I'd like to avoid passing
> down a boolean to create_outbuf and handle this by allocating another
> byte at a higher level. Apart from that, I think this will inevitably
> be dirty, our SMB1 read&x path just has to cover a lot of situations
> where the data comes from.
> 
> Volker
> 

Thanks. Attached is an updated patch series. I think the only remaining
issue is that 'make test' now fails against the source4 DC, the source3
file server should be good.

Christof

> -- 
> SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
> phone: +49-551-370000-0, fax: +49-551-370000-9
> AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
> http://www.sernet.de, mailto:kontakt at sernet.de

> From c2f64d945ad4f5f24baa6b9f31ebe8b0c9829dc1 Mon Sep 17 00:00:00 2001
> From: Christof Schmitt <cs at samba.org>
> Date: Tue, 29 Jul 2014 13:39:57 -0700
> Subject: [PATCH 1/6] VL: torture: Add test for 16 bit alignment of readx data
> 
> MS-CIFS requires a one byte pad to guarantee 16 bit alignment of the
> data:
> 
> Pad (1 byte): This field is optional. When using the NT LAN Manager
> dialect, this field can be used to align the Data field to a 16-bit
> boundary relative to the start of the SMB Header. If Unicode strings are
> being used, this field MUST be present. When used, this field MUST be
> one padding byte long.
> 
> VL: I'd split this up into two, but that's just cosmetics. One for the
> _recv function and one for the test itself.

done

> ---
>  source4/libcli/raw/interfaces.h   | 2 ++
>  source4/libcli/raw/rawreadwrite.c | 2 ++
>  source4/torture/raw/read.c        | 6 ++++++
>  3 files changed, 10 insertions(+)
> 
> diff --git a/source4/libcli/raw/interfaces.h b/source4/libcli/raw/interfaces.h
> index 12c0377..9003c12 100644
> --- a/source4/libcli/raw/interfaces.h
> +++ b/source4/libcli/raw/interfaces.h
> @@ -1809,6 +1809,8 @@ union smb_read {
>  			uint16_t remaining;
>  			uint16_t compaction_mode;
>  			uint32_t nread;
> +			uint16_t flags2;
> +			uint16_t data_offset;
>  		} out;
>  	} readx, generic;
>  
> diff --git a/source4/libcli/raw/rawreadwrite.c b/source4/libcli/raw/rawreadwrite.c
> index d3f5518..fb44ba4 100644
> --- a/source4/libcli/raw/rawreadwrite.c
> +++ b/source4/libcli/raw/rawreadwrite.c
> @@ -155,6 +155,8 @@ _PUBLIC_ NTSTATUS smb_raw_read_recv(struct smbcli_request *req, union smb_read *
>  		parms->readx.out.remaining       = SVAL(req->in.vwv, VWV(2));
>  		parms->readx.out.compaction_mode = SVAL(req->in.vwv, VWV(3));
>  		parms->readx.out.nread = SVAL(req->in.vwv, VWV(5));
> +		parms->readx.out.flags2 = req->flags2;
> +		parms->readx.out.data_offset = SVAL(req->in.vwv, VWV(6));
>  
>  		/* handle oversize replies for non-chained readx replies with
>  		   CAP_LARGE_READX. The snia spec has must to answer for. */
> diff --git a/source4/torture/raw/read.c b/source4/torture/raw/read.c
> index 59089bf..148df3b 100644
> --- a/source4/torture/raw/read.c
> +++ b/source4/torture/raw/read.c
> @@ -435,6 +435,12 @@ static bool test_readx(struct torture_context *tctx, struct smbcli_state *cli)
>  		goto done;
>  	}
>  
> +	if ((io.readx.out.flags2 & FLAGS2_UNICODE_STRINGS) &&
> +	    (io.readx.out.data_offset % 2 != 0)) {
> +		ret = false;
> +		torture_fail_goto(tctx, done, "data not 16 bit aligned\n");
> +	}
> +
>  	printf("Trying short read\n");
>  	io.readx.in.offset = 1;
>  	io.readx.in.mincnt = strlen(test_data);
> -- 
> 1.8.1.2
> 
> 
> From 35bbf9f33f8346262fd9dba2ea8327a2fc8a7d48 Mon Sep 17 00:00:00 2001
> From: Christof Schmitt <cs at samba.org>
> Date: Wed, 6 Aug 2014 11:20:57 -0700
> Subject: [PATCH 2/6] debug
> 
> ---
>  source3/smbd/reply.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/source3/smbd/reply.c b/source3/smbd/reply.c
> index 6c66414..e24fbd1 100644
> --- a/source3/smbd/reply.c
> +++ b/source3/smbd/reply.c
> @@ -3900,6 +3900,10 @@ nosendfile_read:
>  
>  	DEBUG(3, ("send_file_readX %s max=%d nread=%d\n",
>  		  fsp_fnum_dbg(fsp), (int)smb_maxcnt, (int)nread));
> +
> +	DEBUG(0, ("read and x outbuf: %s\n",
> +		  hex_encode_talloc(talloc_tos(), req->outbuf,
> +				    smb_len(req->outbuf))));
>  	return;
>  
>   strict_unlock:
> -- 
> 1.8.1.2
> 
> 
> From d6949baa742a310dbe7a82ab060cd6fbba8e4ef9 Mon Sep 17 00:00:00 2001
> From: Christof Schmitt <cs at samba.org>
> Date: Wed, 6 Aug 2014 13:29:24 -0700
> Subject: [PATCH 3/6] VL: incomplete: Add padding byte to non-sendfile readx
>  response
> 
> This still fails selftest:
> 
> [1/3 in 0s] samba3.raw.read(s3dc)
> Testing RAW_READ_READX
> Trying empty file read
> Trying zero file read
> Trying bad fnum
> Trying small read
> Trying short read
> Trying max offset
> Trying mincnt past EOF
> Trying page sized read
> Trying page + 1 sized read (check alignment)
> Buffer incorrect at line 512! ofs=4096 v1=0x20 v2=0x73
> UNEXPECTED(failure): samba3.raw.read.readx(s3dc)
> REASON: _StringException: _StringException:
> ../source4/torture/raw/read.c:512: buffer check failed
> envlog: SMBD LOG of: LOCALS3DC2
> 
> VL: Question: Is it possible to leave alone and in the higher levels
> calling this just let it allocate one more byte? This only affects read&x,
> all others are unchanged, right?

I changed this in the new patch series by creating a new function to add
the padding byte. This only affects two codepaths for the readx
response.

> ---
>  source3/smbd/process.c     | 28 ++++++++++++++++++++++++----
>  source3/smbd/proto.h       |  2 ++
>  source3/smbd/reply.c       | 11 ++++++-----
>  source4/torture/raw/read.c |  2 +-
>  4 files changed, 33 insertions(+), 10 deletions(-)
> 
> diff --git a/source3/smbd/process.c b/source3/smbd/process.c
> index 7148462..3a39429 100644
> --- a/source3/smbd/process.c
> +++ b/source3/smbd/process.c
> @@ -1344,10 +1344,14 @@ static const struct smb_message_struct {
>  
>  static bool create_outbuf(TALLOC_CTX *mem_ctx, struct smb_request *req,
>  			  const char *inbuf, char **outbuf, uint8_t num_words,
> -			  uint32_t num_bytes)
> +			  uint32_t num_bytes, bool pad)
>  {
>  	size_t smb_len = MIN_SMB_SIZE + VWV(num_words) + num_bytes;
>  
> +	if (pad) {
> +		smb_len += 1;
> +	}
> +
>  	/*
>  	 * Protect against integer wrap.
>  	 * The SMB layer reply can be up to 0xFFFFFF bytes.
> @@ -1371,7 +1375,13 @@ static bool create_outbuf(TALLOC_CTX *mem_ctx, struct smb_request *req,
>  	}
>  
>  	construct_reply_common(req, inbuf, *outbuf);
> +
>  	srv_set_message(*outbuf, num_words, num_bytes, false);
> +
> +	if (pad) {
> +		smb_setlen(*outbuf, smb_size + num_words*2 + num_bytes - 4 + 1);
> +	}
> +
>  	/*
>  	 * Zero out the word area, the caller has to take care of the bcc area
>  	 * himself
> @@ -1386,13 +1396,23 @@ static bool create_outbuf(TALLOC_CTX *mem_ctx, struct smb_request *req,
>  void reply_outbuf(struct smb_request *req, uint8 num_words, uint32 num_bytes)
>  {
>  	char *outbuf;
> -	if (!create_outbuf(req, req, (const char *)req->inbuf, &outbuf, num_words,
> -			   num_bytes)) {
> +	if (!create_outbuf(req, req, (const char *)req->inbuf, &outbuf,
> +			   num_words, num_bytes, false)) {
>  		smb_panic("could not allocate output buffer\n");
>  	}
>  	req->outbuf = (uint8_t *)outbuf;
>  }
>  
> +void reply_outbuf_pad(struct smb_request *req, uint8 num_words,
> +		      uint32 num_bytes)
> +{
> +	char *outbuf;
> +	if (!create_outbuf(req, req, (const char *)req->inbuf, &outbuf,
> +			   num_words, num_bytes, true)) {
> +		smb_panic("could not allocate output buffer\n");
> +	}
> +	req->outbuf = (uint8_t *)outbuf;
> +}
>  
>  /*******************************************************************
>   Dump a packet to a file.
> @@ -3058,7 +3078,7 @@ static bool smbd_echo_reply(struct smbd_echo_state *state,
>  	}
>  
>  	if (!create_outbuf(talloc_tos(), &req, (const char *)req.inbuf, &outbuf,
> -			   1, req.buflen)) {
> +			   1, req.buflen, false)) {
>  		DEBUG(10, ("create_outbuf failed\n"));
>  		return false;
>  	}
> diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h
> index 22cd921..1bb85a0 100644
> --- a/source3/smbd/proto.h
> +++ b/source3/smbd/proto.h
> @@ -771,6 +771,8 @@ bool push_deferred_open_message_smb(struct smb_request *req,
>  			       struct deferred_open_record *open_rec);
>  NTSTATUS allow_new_trans(struct trans_state *list, uint64_t mid);
>  void reply_outbuf(struct smb_request *req, uint8 num_words, uint32 num_bytes);
> +void reply_outbuf_pad(struct smb_request *req, uint8 num_words,
> +		      uint32 num_bytes);
>  void smb_request_done(struct smb_request *req);
>  const char *smb_fn_name(int type);
>  void add_to_common_flags2(uint32 v);
> diff --git a/source3/smbd/reply.c b/source3/smbd/reply.c
> index e24fbd1..d1afcf7 100644
> --- a/source3/smbd/reply.c
> +++ b/source3/smbd/reply.c
> @@ -3688,11 +3688,12 @@ static int setup_readX_header(struct smb_request *req, char *outbuf,
>  	      (smb_wct - 4)	/* offset from smb header to wct */
>  	      + 1 		/* the wct field */
>  	      + 12 * sizeof(uint16_t) /* vwv */
> -	      + 2);		/* the buflen field */
> +	      + 2		/* the buflen field */
> +	      + 1);		/* padding byte */
>  	SSVAL(outbuf,smb_vwv7,(smb_maxcnt >> 16));
>  	SSVAL(outbuf,smb_vwv11,smb_maxcnt);
>  	/* Reset the outgoing length, set_message truncates at 0x1FFFF. */
> -	_smb_setlen_large(outbuf,(smb_size + 12*2 + smb_maxcnt - 4));
> +	_smb_setlen_large(outbuf,(smb_size + 12*2 + smb_maxcnt - 4 + 1));
>  	return outsize;
>  }
>  
> @@ -3729,7 +3730,7 @@ static void send_file_readX(connection_struct *conn, struct smb_request *req,
>  	    (fsp->base_fsp == NULL) &&
>  	    (fsp->wcp == NULL) &&
>  	    lp_use_sendfile(SNUM(conn), xconn->smb1.signing_state) ) {
> -		uint8 headerbuf[smb_size + 12 * 2];
> +		uint8 headerbuf[smb_size + 12 * 2 + 1];
>  		DATA_BLOB header;
>  
>  		if(fsp_stat(fsp) == -1) {
> @@ -3843,7 +3844,7 @@ static void send_file_readX(connection_struct *conn, struct smb_request *req,
>  normal_read:
>  
>  	if ((smb_maxcnt & 0xFF0000) > 0x10000) {
> -		uint8 headerbuf[smb_size + 2*12];
> +		uint8 headerbuf[smb_size + 2*12 + 1];
>  		ssize_t ret;
>  
>  		construct_reply_common_req(req, (char *)headerbuf);
> @@ -3886,7 +3887,7 @@ nosendfile_read:
>  	SSVAL(req->outbuf, smb_vwv0, 0xff); /* andx chain ends */
>  	SSVAL(req->outbuf, smb_vwv1, 0);    /* no andx offset */
>  
> -	nread = read_file(fsp, smb_buf(req->outbuf), startpos, smb_maxcnt);
> +	nread = read_file(fsp, smb_buf(req->outbuf) + 1, startpos, smb_maxcnt);
>  	saved_errno = errno;
>  
>  	SMB_VFS_STRICT_UNLOCK(conn, fsp, &lock);
> diff --git a/source4/torture/raw/read.c b/source4/torture/raw/read.c
> index 148df3b..711feec 100644
> --- a/source4/torture/raw/read.c
> +++ b/source4/torture/raw/read.c
> @@ -44,7 +44,7 @@
>  #define CHECK_BUFFER(buf, seed, len) do { \
>  	if (!check_buffer(buf, seed, len, __LINE__)) { \
>  		ret = false; \
> -		goto done; \
> +		torture_fail_goto(tctx, done, "buffer check failed\n"); \
>  	}} while (0)
>  
>  #define BASEDIR "\\testread"
> -- 
> 1.8.1.2
> 
> 
> From 992322ff599b673fc51a3af53e25ca445e386c04 Mon Sep 17 00:00:00 2001
> From: Christof Schmitt <cs at samba.org>
> Date: Wed, 6 Aug 2014 13:53:10 -0700
> Subject: [PATCH 4/6] smbd: Add padding byte to readx response from rpc
>  requests
> 
> ---
>  source3/smbd/pipes.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/source3/smbd/pipes.c b/source3/smbd/pipes.c
> index f1f61bb..fa0c148 100644
> --- a/source3/smbd/pipes.c
> +++ b/source3/smbd/pipes.c
> @@ -422,7 +422,7 @@ void reply_pipe_read_and_X(struct smb_request *req)
>  	state->smb_maxcnt = SVAL(req->vwv+5, 0);
>  	state->smb_mincnt = SVAL(req->vwv+6, 0);
>  
> -	reply_outbuf(req, 12, state->smb_maxcnt);
> +	reply_outbuf_pad(req, 12, state->smb_maxcnt);
>  	SSVAL(req->outbuf, smb_vwv0, 0xff); /* andx chain ends */
>  	SSVAL(req->outbuf, smb_vwv1, 0);    /* no andx offset */
>  
> @@ -467,7 +467,7 @@ static void pipe_read_andx_done(struct tevent_req *subreq)
>  	req->outbuf = state->outbuf;
>  	state->outbuf = NULL;
>  
> -	srv_set_message((char *)req->outbuf, 12, nread, False);
> +	srv_set_message((char *)req->outbuf, 12, nread + 1, False);
>  
>  #if 0
>  	/*
> -- 
> 1.8.1.2
> 
> 
> From 952a6fb2e7399b82e3c926e01eea671b02182972 Mon Sep 17 00:00:00 2001
> From: Christof Schmitt <cs at samba.org>
> Date: Wed, 6 Aug 2014 13:55:50 -0700
> Subject: [PATCH 5/6] torture: Also run raw.read against the aio share
> 
> ---
>  source3/selftest/tests.py | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
> index e9b91d4..2c48b34 100755
> --- a/source3/selftest/tests.py
> +++ b/source3/selftest/tests.py
> @@ -355,6 +355,10 @@ for t in tests:
>          plansmbtorture4testsuite(t, "s3dc", '//$SERVER_IP/aio -U$USERNAME%$PASSWORD', 'aio')
>          plansmbtorture4testsuite(t, "s3dc", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD')
>          plansmbtorture4testsuite(t, "plugin_s4_dc", '//$SERVER/tmp -U$USERNAME%$PASSWORD')
> +    elif t == "raw.read":
> +        plansmbtorture4testsuite(t, "s3dc", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD')
> +        plansmbtorture4testsuite(t, "plugin_s4_dc", '//$SERVER/tmp -U$USERNAME%$PASSWORD')
> +        plansmbtorture4testsuite(t, "s3dc", '//$SERVER_IP/aio -U$USERNAME%$PASSWORD', 'aio')
>      elif t == "raw.search":
>          plansmbtorture4testsuite(t, "s3dc", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD')
>  # test the dirsort module.
> -- 
> 1.8.1.2
> 
> 
> From 069e303bb15ce2b72b41ac23d17ccddf9d4b0d5c Mon Sep 17 00:00:00 2001
> From: Christof Schmitt <cs at samba.org>
> Date: Wed, 6 Aug 2014 14:24:33 -0700
> Subject: [PATCH 6/6] smbd: Add padding byte to aio readx response
> 
> ---
>  source3/smbd/aio.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/source3/smbd/aio.c b/source3/smbd/aio.c
> index 2235c32..12a39f9 100644
> --- a/source3/smbd/aio.c
> +++ b/source3/smbd/aio.c
> @@ -196,7 +196,7 @@ NTSTATUS schedule_aio_read_and_X(connection_struct *conn,
>  	/* The following is safe from integer wrap as we've already checked
>  	   smb_maxcnt is 128k or less. Wct is 12 for read replies */
>  
> -	bufsize = smb_size + 12 * 2 + smb_maxcnt;
> +	bufsize = smb_size + 12 * 2 + smb_maxcnt + 1;
>  
>  	if ((aio_ex = create_aio_extra(NULL, fsp, bufsize)) == NULL) {
>  		DEBUG(10,("schedule_aio_read_and_X: malloc fail.\n"));
> @@ -221,7 +221,7 @@ NTSTATUS schedule_aio_read_and_X(connection_struct *conn,
>  	aio_ex->offset = startpos;
>  
>  	req = SMB_VFS_PREAD_SEND(aio_ex, fsp->conn->sconn->ev_ctx,
> -				 fsp, smb_buf(aio_ex->outbuf.data),
> +				 fsp, smb_buf(aio_ex->outbuf.data) + 1,
>  				 smb_maxcnt, startpos);
>  	if (req == NULL) {
>  		DEBUG(0,("schedule_aio_read_and_X: aio_read failed. "
> @@ -256,7 +256,7 @@ static void aio_pread_smb1_done(struct tevent_req *req)
>  	files_struct *fsp = aio_ex->fsp;
>  	int outsize;
>  	char *outbuf = (char *)aio_ex->outbuf.data;
> -	char *data = smb_buf(outbuf);
> +	char *data = smb_buf(outbuf) + 1;
>  	ssize_t nread;
>  	int err;
>  
> @@ -285,7 +285,7 @@ static void aio_pread_smb1_done(struct tevent_req *req)
>  		ERROR_NT(map_nt_error_from_unix(err));
>  		outsize = srv_set_message(outbuf,0,0,true);
>  	} else {
> -		outsize = srv_set_message(outbuf, 12, nread, False);
> +		outsize = srv_set_message(outbuf, 12, nread + 1, False);
>  		SSVAL(outbuf,smb_vwv2, 0xFFFF); /* Remaining - must be * -1. */
>  		SSVAL(outbuf,smb_vwv5, nread);
>  		SSVAL(outbuf,smb_vwv6, smb_offset(data,outbuf));
> -- 
> 1.8.1.2
> 

-------------- next part --------------
>From e546c3337cec3e398377e4922efcc14898c3529c Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Thu, 7 Aug 2014 15:20:23 -0700
Subject: [PATCH 1/8] smbd: Add function to extend outbuf with additional padding byte

Signed-off-by: Christof Schmitt <cs at samba.org>
---
 source3/smbd/process.c |   15 +++++++++++++++
 source3/smbd/proto.h   |    2 ++
 2 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/source3/smbd/process.c b/source3/smbd/process.c
index 7148462..929fb2f 100644
--- a/source3/smbd/process.c
+++ b/source3/smbd/process.c
@@ -1393,6 +1393,21 @@ void reply_outbuf(struct smb_request *req, uint8 num_words, uint32 num_bytes)
 	req->outbuf = (uint8_t *)outbuf;
 }
 
+void reply_outbuf_add_pad(struct smb_request *req, uint8 num_words,
+			  uint32 num_bytes)
+{
+	size_t smb_len = MIN_SMB_SIZE + VWV(num_words) + num_bytes;
+
+	req->outbuf = (uint8_t *)talloc_realloc(req, req->outbuf, char,
+						NBT_HDR_SIZE + smb_len
+						+ 1 /* padding byte */);
+	if (req->outbuf == NULL) {
+		smb_panic("could not reallocate output buffer\n");
+	}
+
+	smb_setlen(req->outbuf, smb_size + num_words*2 + num_bytes - 4
+		   + 1 /* padding byte */);
+}
 
 /*******************************************************************
  Dump a packet to a file.
diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h
index 22cd921..a97b47e 100644
--- a/source3/smbd/proto.h
+++ b/source3/smbd/proto.h
@@ -771,6 +771,8 @@ bool push_deferred_open_message_smb(struct smb_request *req,
 			       struct deferred_open_record *open_rec);
 NTSTATUS allow_new_trans(struct trans_state *list, uint64_t mid);
 void reply_outbuf(struct smb_request *req, uint8 num_words, uint32 num_bytes);
+void reply_outbuf_add_pad(struct smb_request *req, uint8 num_words,
+			  uint32 num_bytes);
 void smb_request_done(struct smb_request *req);
 const char *smb_fn_name(int type);
 void add_to_common_flags2(uint32 v);
-- 
1.7.1


>From a7391ffde7dfb3a89448daf9a45956926a062b7b Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Thu, 7 Aug 2014 15:26:54 -0700
Subject: [PATCH 2/8] smbd: Add padding byte to readx response

MS-CIFS 2.2.4.42.2 states:
"Pad (1 byte): This field is optional. When using the NT LAN Manager
dialect, this field can be used to align the Data field to a 16-bit
boundary relative to the start of the SMB Header. If Unicode strings are
being used, this field MUST be present. When used, this field MUST be
one padding byte long."

Always add the padding byte to all readx responses to avoid additional
complexity.

Signed-off-by: Christof Schmitt <cs at samba.org>
---
 source3/smbd/aio.c   |   10 ++++++----
 source3/smbd/pipes.c |    4 +++-
 source3/smbd/reply.c |   14 +++++++++-----
 3 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/source3/smbd/aio.c b/source3/smbd/aio.c
index 2235c32..8d32d44 100644
--- a/source3/smbd/aio.c
+++ b/source3/smbd/aio.c
@@ -196,7 +196,7 @@ NTSTATUS schedule_aio_read_and_X(connection_struct *conn,
 	/* The following is safe from integer wrap as we've already checked
 	   smb_maxcnt is 128k or less. Wct is 12 for read replies */
 
-	bufsize = smb_size + 12 * 2 + smb_maxcnt;
+	bufsize = smb_size + 12 * 2 + smb_maxcnt + 1 /* padding byte */;
 
 	if ((aio_ex = create_aio_extra(NULL, fsp, bufsize)) == NULL) {
 		DEBUG(10,("schedule_aio_read_and_X: malloc fail.\n"));
@@ -221,7 +221,8 @@ NTSTATUS schedule_aio_read_and_X(connection_struct *conn,
 	aio_ex->offset = startpos;
 
 	req = SMB_VFS_PREAD_SEND(aio_ex, fsp->conn->sconn->ev_ctx,
-				 fsp, smb_buf(aio_ex->outbuf.data),
+				 fsp,
+				 smb_buf(aio_ex->outbuf.data) + 1 /* pad */,
 				 smb_maxcnt, startpos);
 	if (req == NULL) {
 		DEBUG(0,("schedule_aio_read_and_X: aio_read failed. "
@@ -256,7 +257,7 @@ static void aio_pread_smb1_done(struct tevent_req *req)
 	files_struct *fsp = aio_ex->fsp;
 	int outsize;
 	char *outbuf = (char *)aio_ex->outbuf.data;
-	char *data = smb_buf(outbuf);
+	char *data = smb_buf(outbuf) + 1 /* padding byte */;
 	ssize_t nread;
 	int err;
 
@@ -285,7 +286,8 @@ static void aio_pread_smb1_done(struct tevent_req *req)
 		ERROR_NT(map_nt_error_from_unix(err));
 		outsize = srv_set_message(outbuf,0,0,true);
 	} else {
-		outsize = srv_set_message(outbuf, 12, nread, False);
+		outsize = srv_set_message(outbuf, 12,
+					  nread + 1 /* padding byte */, false);
 		SSVAL(outbuf,smb_vwv2, 0xFFFF); /* Remaining - must be * -1. */
 		SSVAL(outbuf,smb_vwv5, nread);
 		SSVAL(outbuf,smb_vwv6, smb_offset(data,outbuf));
diff --git a/source3/smbd/pipes.c b/source3/smbd/pipes.c
index f1f61bb..24dcb11 100644
--- a/source3/smbd/pipes.c
+++ b/source3/smbd/pipes.c
@@ -423,6 +423,7 @@ void reply_pipe_read_and_X(struct smb_request *req)
 	state->smb_mincnt = SVAL(req->vwv+6, 0);
 
 	reply_outbuf(req, 12, state->smb_maxcnt);
+	reply_outbuf_add_pad(req, 12, state->smb_maxcnt);
 	SSVAL(req->outbuf, smb_vwv0, 0xff); /* andx chain ends */
 	SSVAL(req->outbuf, smb_vwv1, 0);    /* no andx offset */
 
@@ -467,7 +468,8 @@ static void pipe_read_andx_done(struct tevent_req *subreq)
 	req->outbuf = state->outbuf;
 	state->outbuf = NULL;
 
-	srv_set_message((char *)req->outbuf, 12, nread, False);
+	srv_set_message((char *)req->outbuf, 12, nread + 1 /* padding byte */,
+			false);
 
 #if 0
 	/*
diff --git a/source3/smbd/reply.c b/source3/smbd/reply.c
index 6c66414..0377c35 100644
--- a/source3/smbd/reply.c
+++ b/source3/smbd/reply.c
@@ -3688,11 +3688,13 @@ static int setup_readX_header(struct smb_request *req, char *outbuf,
 	      (smb_wct - 4)	/* offset from smb header to wct */
 	      + 1 		/* the wct field */
 	      + 12 * sizeof(uint16_t) /* vwv */
-	      + 2);		/* the buflen field */
+	      + 2		/* the buflen field */
+	      + 1);		/* padding byte */
 	SSVAL(outbuf,smb_vwv7,(smb_maxcnt >> 16));
 	SSVAL(outbuf,smb_vwv11,smb_maxcnt);
 	/* Reset the outgoing length, set_message truncates at 0x1FFFF. */
-	_smb_setlen_large(outbuf,(smb_size + 12*2 + smb_maxcnt - 4));
+	_smb_setlen_large(outbuf,
+			  smb_size + 12*2 + smb_maxcnt - 4 + 1 /* pad */);
 	return outsize;
 }
 
@@ -3729,7 +3731,7 @@ static void send_file_readX(connection_struct *conn, struct smb_request *req,
 	    (fsp->base_fsp == NULL) &&
 	    (fsp->wcp == NULL) &&
 	    lp_use_sendfile(SNUM(conn), xconn->smb1.signing_state) ) {
-		uint8 headerbuf[smb_size + 12 * 2];
+		uint8 headerbuf[smb_size + 12 * 2 + 1 /* padding byte */];
 		DATA_BLOB header;
 
 		if(fsp_stat(fsp) == -1) {
@@ -3843,7 +3845,7 @@ static void send_file_readX(connection_struct *conn, struct smb_request *req,
 normal_read:
 
 	if ((smb_maxcnt & 0xFF0000) > 0x10000) {
-		uint8 headerbuf[smb_size + 2*12];
+		uint8 headerbuf[smb_size + 2*12 + 1 /* padding byte */];
 		ssize_t ret;
 
 		construct_reply_common_req(req, (char *)headerbuf);
@@ -3883,10 +3885,12 @@ normal_read:
 nosendfile_read:
 
 	reply_outbuf(req, 12, smb_maxcnt);
+	reply_outbuf_add_pad(req, 12, smb_maxcnt);
 	SSVAL(req->outbuf, smb_vwv0, 0xff); /* andx chain ends */
 	SSVAL(req->outbuf, smb_vwv1, 0);    /* no andx offset */
 
-	nread = read_file(fsp, smb_buf(req->outbuf), startpos, smb_maxcnt);
+	nread = read_file(fsp, smb_buf(req->outbuf) + 1 /* padding byte */,
+			  startpos, smb_maxcnt);
 	saved_errno = errno;
 
 	SMB_VFS_STRICT_UNLOCK(conn, fsp, &lock);
-- 
1.7.1


>From 4a01495c7911e30c2196933608a12f9ccf3dc1d2 Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Thu, 7 Aug 2014 14:19:57 -0700
Subject: [PATCH 3/8] s4:libcli/raw: Make flags2 and offset available to callers of readx

This will be used by smbtorture.

Signed-off-by: Christof Schmitt <cs at samba.org>
---
 source4/libcli/raw/interfaces.h   |    2 ++
 source4/libcli/raw/rawreadwrite.c |    2 ++
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/source4/libcli/raw/interfaces.h b/source4/libcli/raw/interfaces.h
index 12c0377..9003c12 100644
--- a/source4/libcli/raw/interfaces.h
+++ b/source4/libcli/raw/interfaces.h
@@ -1809,6 +1809,8 @@ union smb_read {
 			uint16_t remaining;
 			uint16_t compaction_mode;
 			uint32_t nread;
+			uint16_t flags2;
+			uint16_t data_offset;
 		} out;
 	} readx, generic;
 
diff --git a/source4/libcli/raw/rawreadwrite.c b/source4/libcli/raw/rawreadwrite.c
index d3f5518..fb44ba4 100644
--- a/source4/libcli/raw/rawreadwrite.c
+++ b/source4/libcli/raw/rawreadwrite.c
@@ -155,6 +155,8 @@ _PUBLIC_ NTSTATUS smb_raw_read_recv(struct smbcli_request *req, union smb_read *
 		parms->readx.out.remaining       = SVAL(req->in.vwv, VWV(2));
 		parms->readx.out.compaction_mode = SVAL(req->in.vwv, VWV(3));
 		parms->readx.out.nread = SVAL(req->in.vwv, VWV(5));
+		parms->readx.out.flags2 = req->flags2;
+		parms->readx.out.data_offset = SVAL(req->in.vwv, VWV(6));
 
 		/* handle oversize replies for non-chained readx replies with
 		   CAP_LARGE_READX. The snia spec has must to answer for. */
-- 
1.7.1


>From d98ba5f4140bae1f5e3e888a6e8730d12b26fd10 Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Thu, 7 Aug 2014 14:25:13 -0700
Subject: [PATCH 4/8] torture: Add test for 16 bit alignment of readx data

MS-CIFS requires a one byte pad to guarantee 16 bit alignment of the
data:

Pad (1 byte): This field is optional. When using the NT LAN Manager
dialect, this field can be used to align the Data field to a 16-bit
boundary relative to the start of the SMB Header. If Unicode strings are
being used, this field MUST be present. When used, this field MUST be
one padding byte long.

Signed-off-by: Christof Schmitt <cs at samba.org>
---
 source4/torture/raw/read.c |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/source4/torture/raw/read.c b/source4/torture/raw/read.c
index 59089bf..f176e7f 100644
--- a/source4/torture/raw/read.c
+++ b/source4/torture/raw/read.c
@@ -47,6 +47,13 @@
 		goto done; \
 	}} while (0)
 
+#define CHECK_READX_ALIGN(io) do {			      \
+	if ((io.readx.out.flags2 & FLAGS2_UNICODE_STRINGS) && \
+	    (io.readx.out.data_offset % 2 != 0)) { \
+		ret = false; \
+		torture_fail_goto(tctx, done, "data not 16 bit aligned\n"); \
+	}} while (0)
+
 #define BASEDIR "\\testread"
 
 
@@ -399,6 +406,7 @@ static bool test_readx(struct torture_context *tctx, struct smbcli_state *cli)
 	CHECK_VALUE(io.readx.out.nread, 0);
 	CHECK_VALUE(io.readx.out.remaining, 0xFFFF);
 	CHECK_VALUE(io.readx.out.compaction_mode, 0);
+	CHECK_READX_ALIGN(io);
 
 	printf("Trying zero file read\n");
 	io.readx.in.mincnt = 0;
@@ -408,6 +416,7 @@ static bool test_readx(struct torture_context *tctx, struct smbcli_state *cli)
 	CHECK_VALUE(io.readx.out.nread, 0);
 	CHECK_VALUE(io.readx.out.remaining, 0xFFFF);
 	CHECK_VALUE(io.readx.out.compaction_mode, 0);
+	CHECK_READX_ALIGN(io);
 
 	printf("Trying bad fnum\n");
 	io.readx.in.file.fnum = fnum+1;
@@ -429,6 +438,7 @@ static bool test_readx(struct torture_context *tctx, struct smbcli_state *cli)
 	CHECK_VALUE(io.readx.out.nread, strlen(test_data));
 	CHECK_VALUE(io.readx.out.remaining, 0xFFFF);
 	CHECK_VALUE(io.readx.out.compaction_mode, 0);
+	CHECK_READX_ALIGN(io);
 	if (memcmp(buf, test_data, strlen(test_data)) != 0) {
 		ret = false;
 		printf("incorrect data at %d!? (%s:%s)\n", __LINE__, test_data, buf);
@@ -444,6 +454,7 @@ static bool test_readx(struct torture_context *tctx, struct smbcli_state *cli)
 	CHECK_VALUE(io.readx.out.nread, strlen(test_data)-1);
 	CHECK_VALUE(io.readx.out.remaining, 0xFFFF);
 	CHECK_VALUE(io.readx.out.compaction_mode, 0);
+	CHECK_READX_ALIGN(io);
 	if (memcmp(buf, test_data+1, strlen(test_data)-1) != 0) {
 		ret = false;
 		printf("incorrect data at %d!? (%s:%s)\n", __LINE__, test_data+1, buf);
@@ -460,6 +471,7 @@ static bool test_readx(struct torture_context *tctx, struct smbcli_state *cli)
 		CHECK_VALUE(io.readx.out.nread, 0);
 		CHECK_VALUE(io.readx.out.remaining, 0xFFFF);
 		CHECK_VALUE(io.readx.out.compaction_mode, 0);
+		CHECK_READX_ALIGN(io);
 	}
 
 	printf("Trying mincnt past EOF\n");
@@ -472,6 +484,7 @@ static bool test_readx(struct torture_context *tctx, struct smbcli_state *cli)
 	CHECK_VALUE(io.readx.out.remaining, 0xFFFF);
 	CHECK_VALUE(io.readx.out.compaction_mode, 0);
 	CHECK_VALUE(io.readx.out.nread, strlen(test_data));
+	CHECK_READX_ALIGN(io);
 	if (memcmp(buf, test_data, strlen(test_data)) != 0) {
 		ret = false;
 		printf("incorrect data at %d!? (%s:%s)\n", __LINE__, test_data, buf);
@@ -493,6 +506,7 @@ static bool test_readx(struct torture_context *tctx, struct smbcli_state *cli)
 	CHECK_VALUE(io.readx.out.compaction_mode, 0);
 	CHECK_VALUE(io.readx.out.nread, io.readx.in.maxcnt);
 	CHECK_BUFFER(buf, seed, io.readx.out.nread);
+	CHECK_READX_ALIGN(io);
 
 	printf("Trying page + 1 sized read (check alignment)\n");
 	io.readx.in.offset = 0;
@@ -504,6 +518,7 @@ static bool test_readx(struct torture_context *tctx, struct smbcli_state *cli)
 	CHECK_VALUE(io.readx.out.compaction_mode, 0);
 	CHECK_VALUE(io.readx.out.nread, io.readx.in.maxcnt);
 	CHECK_BUFFER(buf, seed, io.readx.out.nread);
+	CHECK_READX_ALIGN(io);
 
 	printf("Trying large read (UINT16_MAX)\n");
 	io.readx.in.offset = 0;
@@ -515,6 +530,7 @@ static bool test_readx(struct torture_context *tctx, struct smbcli_state *cli)
 	CHECK_VALUE(io.readx.out.compaction_mode, 0);
 	CHECK_VALUE(io.readx.out.nread, io.readx.in.maxcnt);
 	CHECK_BUFFER(buf, seed, io.readx.out.nread);
+	CHECK_READX_ALIGN(io);
 
 	printf("Trying extra large read\n");
 	io.readx.in.offset = 0;
@@ -531,6 +547,7 @@ static bool test_readx(struct torture_context *tctx, struct smbcli_state *cli)
 		CHECK_VALUE(io.readx.out.nread, 0x10000);
 	}
 	CHECK_BUFFER(buf, seed, io.readx.out.nread);
+	CHECK_READX_ALIGN(io);
 
 	printf("Trying mincnt > maxcnt\n");
 	memset(buf, 0, maxsize);
@@ -543,6 +560,7 @@ static bool test_readx(struct torture_context *tctx, struct smbcli_state *cli)
 	CHECK_VALUE(io.readx.out.compaction_mode, 0);
 	CHECK_VALUE(io.readx.out.nread, io.readx.in.maxcnt);
 	CHECK_BUFFER(buf, seed, io.readx.out.nread);
+	CHECK_READX_ALIGN(io);
 
 	printf("Trying mincnt < maxcnt\n");
 	memset(buf, 0, maxsize);
@@ -555,6 +573,7 @@ static bool test_readx(struct torture_context *tctx, struct smbcli_state *cli)
 	CHECK_VALUE(io.readx.out.compaction_mode, 0);
 	CHECK_VALUE(io.readx.out.nread, io.readx.in.maxcnt);
 	CHECK_BUFFER(buf, seed, io.readx.out.nread);
+	CHECK_READX_ALIGN(io);
 
 	if (cli->transport->negotiate.capabilities & CAP_LARGE_READX) {
 		printf("Trying large readx\n");
@@ -564,11 +583,13 @@ static bool test_readx(struct torture_context *tctx, struct smbcli_state *cli)
 		status = smb_raw_read(cli->tree, &io);
 		CHECK_STATUS(status, NT_STATUS_OK);
 		CHECK_VALUE(io.readx.out.nread, 0xFFFF);
+		CHECK_READX_ALIGN(io);
 
 		io.readx.in.maxcnt = 0x10000;
 		status = smb_raw_read(cli->tree, &io);
 		CHECK_STATUS(status, NT_STATUS_OK);
 		CHECK_VALUE(io.readx.out.nread, 0x10000);
+		CHECK_READX_ALIGN(io);
 
 		io.readx.in.maxcnt = 0x10001;
 		status = smb_raw_read(cli->tree, &io);
@@ -610,6 +631,7 @@ static bool test_readx(struct torture_context *tctx, struct smbcli_state *cli)
 	status = smb_raw_read(cli->tree, &io);
 	CHECK_STATUS(status, NT_STATUS_OK);
 	CHECK_VALUE(io.readx.out.nread, 0);
+	CHECK_READX_ALIGN(io);
 
 	if (NT_STATUS_IS_ERR(smbcli_lock64(cli->tree, fnum, io.readx.in.offset, 1, 0, WRITE_LOCK))) {
 		printf("Failed to lock file at %d\n", __LINE__);
@@ -620,6 +642,7 @@ static bool test_readx(struct torture_context *tctx, struct smbcli_state *cli)
 	status = smb_raw_read(cli->tree, &io);
 	CHECK_STATUS(status, NT_STATUS_OK);
 	CHECK_VALUE(io.readx.out.nread, 0);
+	CHECK_READX_ALIGN(io);
 
 done:
 	smbcli_close(cli->tree, fnum);
-- 
1.7.1


>From f4e30a85292f70a7af23eb1fff5736d2d1cad650 Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Thu, 7 Aug 2014 14:31:42 -0700
Subject: [PATCH 5/8] torture: Use torture_fail macro in check_buffer for read requests

Signed-off-by: Christof Schmitt <cs at samba.org>
---
 source4/torture/raw/read.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/source4/torture/raw/read.c b/source4/torture/raw/read.c
index f176e7f..8551c0b 100644
--- a/source4/torture/raw/read.c
+++ b/source4/torture/raw/read.c
@@ -42,9 +42,9 @@
 	}} while (0)
 
 #define CHECK_BUFFER(buf, seed, len) do { \
-	if (!check_buffer(buf, seed, len, __LINE__)) { \
+	if (!check_buffer(tctx, buf, seed, len, __LINE__)) {	\
 		ret = false; \
-		goto done; \
+		torture_fail_goto(tctx, done, "buffer check failed\n"); \
 	}} while (0)
 
 #define CHECK_READX_ALIGN(io) do {			      \
@@ -70,15 +70,17 @@ static void setup_buffer(uint8_t *buf, unsigned int seed, int len)
 /*
   check a random buffer based on a seed
 */
-static bool check_buffer(uint8_t *buf, unsigned int seed, int len, int line)
+static bool check_buffer(struct torture_context *tctx, uint8_t *buf,
+			 unsigned int seed, int len, int line)
 {
 	int i;
 	srandom(seed);
 	for (i=0;i<len;i++) {
 		uint8_t v = random();
 		if (buf[i] != v) {
-			printf("Buffer incorrect at line %d! ofs=%d v1=0x%x v2=0x%x\n", 
-			       line, i, buf[i], v);
+			torture_warning(tctx, "Buffer incorrect at line %d! "
+					"ofs=%d v1=0x%x v2=0x%x\n", line, i,
+					buf[i], v);
 			return false;
 		}
 	}
-- 
1.7.1


>From c1ae740528a209f9ee4e852e876aee8334e4a415 Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Thu, 7 Aug 2014 14:40:00 -0700
Subject: [PATCH 6/8] torture: Use torture_assert macro for status check in raw.read

Signed-off-by: Christof Schmitt <cs at samba.org>
---
 source4/torture/raw/read.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/source4/torture/raw/read.c b/source4/torture/raw/read.c
index 8551c0b..365b385 100644
--- a/source4/torture/raw/read.c
+++ b/source4/torture/raw/read.c
@@ -26,12 +26,9 @@
 #include "torture/raw/proto.h"
 
 #define CHECK_STATUS(status, correct) do { \
-	if (!NT_STATUS_EQUAL(status, correct)) { \
-		printf("(%s) Incorrect status %s - should be %s\n", \
-		       __location__, nt_errstr(status), nt_errstr(correct)); \
-		ret = false; \
-		goto done; \
-	}} while (0)
+		torture_assert_ntstatus_equal_goto(tctx, status, correct, ret, \
+						   done, "incorrect status"); \
+	} while (0)
 
 #define CHECK_VALUE(v, correct) do { \
 	if ((v) != (correct)) { \
-- 
1.7.1


>From c7b2b97b828561895255a49c4653ccc0fda43878 Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Thu, 7 Aug 2014 14:44:23 -0700
Subject: [PATCH 7/8] torture: Use torture_assert macro for value check in raw.read

Signed-off-by: Christof Schmitt <cs at samba.org>
---
 source4/torture/raw/read.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/source4/torture/raw/read.c b/source4/torture/raw/read.c
index 365b385..6abf08b 100644
--- a/source4/torture/raw/read.c
+++ b/source4/torture/raw/read.c
@@ -31,12 +31,9 @@
 	} while (0)
 
 #define CHECK_VALUE(v, correct) do { \
-	if ((v) != (correct)) { \
-		printf("(%s) Incorrect value %s=%ld - should be %ld\n", \
-		       __location__, #v, (long)v, (long)correct); \
-		ret = false; \
-		goto done; \
-	}} while (0)
+		torture_assert_int_equal_goto(tctx, (v), (correct), ret, done, \
+					      "Incorrect value"); \
+	} while (0)
 
 #define CHECK_BUFFER(buf, seed, len) do { \
 	if (!check_buffer(tctx, buf, seed, len, __LINE__)) {	\
-- 
1.7.1


>From 8ecef96ed594ae3e753b3c1ef9329793d911cf48 Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Thu, 7 Aug 2014 15:42:05 -0700
Subject: [PATCH 8/8] torture: Also run raw.read against the aio share

This tests the changes in the aio code path.

Signed-off-by: Christof Schmitt <cs at samba.org>
---
 source3/selftest/tests.py |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
index e9b91d4..796865c 100755
--- a/source3/selftest/tests.py
+++ b/source3/selftest/tests.py
@@ -355,6 +355,10 @@ for t in tests:
         plansmbtorture4testsuite(t, "s3dc", '//$SERVER_IP/aio -U$USERNAME%$PASSWORD', 'aio')
         plansmbtorture4testsuite(t, "s3dc", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD')
         plansmbtorture4testsuite(t, "plugin_s4_dc", '//$SERVER/tmp -U$USERNAME%$PASSWORD')
+    elif t == "raw.read":
+        plansmbtorture4testsuite(t, "s3dc", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD')
+        plansmbtorture4testsuite(t, "s3dc", '//$SERVER_IP/aio -U$USERNAME%$PASSWORD', 'aio')
+        plansmbtorture4testsuite(t, "plugin_s4_dc", '//$SERVER/tmp -U$USERNAME%$PASSWORD')
     elif t == "raw.search":
         plansmbtorture4testsuite(t, "s3dc", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD')
 # test the dirsort module.
-- 
1.7.1



More information about the samba-technical mailing list