small valgrind fix
Stefan Metzmacher
metze at samba.org
Fri Nov 27 18:32:25 UTC 2015
Am 27.11.2015 um 18:11 schrieb Stefan Metzmacher:
> Hi Noel,
>
> thanks for the report!
>
> Can you try the attached patch?
>
> David can you also have a look at the patch?
Here're better fixes which also fix the STATUS_BUFFER_OVERFLOW
cases.
metze
-------------- next part --------------
From 0259796887405e1c256b4a509b9919936c4e82c0 Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Fri, 27 Nov 2015 17:31:04 +0100
Subject: [PATCH 1/5] libcli/smb: make sure we have a body size of 0x31 before
dereferencing an ioctl response
Found by valgrind, reported by Noel Power <nopower at suse.com>:
==7913== Invalid read of size 1
==7913== at 0xC4F23EE: smb2cli_ioctl_done (smb2cli_ioctl.c:245)
==7913== by 0x747A744: _tevent_req_notify_callback (tevent_req.c:112)
==7913== by 0x747A817: tevent_req_finish (tevent_req.c:149)
==7913== by 0x747A93C: tevent_req_trigger (tevent_req.c:206)
==7913== by 0x7479B2B: tevent_common_loop_immediate
(tevent_immediate.c:135)
==7913== by 0xA9CB4BE: run_events_poll (events.c:192)
==7913== by 0xA9CBB32: s3_event_loop_once (events.c:303)
==7913== by 0x7478C72: _tevent_loop_once (tevent.c:533)
==7913== by 0x747AACD: tevent_req_poll (tevent_req.c:256)
==7913== by 0x505315D: tevent_req_poll_ntstatus (tevent_ntstatus.c:109)
==7913== by 0xA7201F2: cli_tree_connect (cliconnect.c:2764)
==7913== by 0x165FF7: cm_prepare_connection (winbindd_cm.c:1276)
==7913== Address 0x16ce24ec is 764 bytes inside a block of size 813 alloc'd
==7913== at 0x4C29110: malloc (in
/usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==7913== by 0x768A0C1: __talloc_with_prefix (talloc.c:668)
==7913== by 0x768A27E: _talloc_pool (talloc.c:721)
==7913== by 0x768A41E: _talloc_pooled_object (talloc.c:790)
==7913== by 0x747A594: _tevent_req_create (tevent_req.c:66)
==7913== by 0xCF6E2FA: read_packet_send (async_sock.c:414)
==7913== by 0xCF6EB54: read_smb_send (read_smb.c:54)
==7913== by 0xC4DA146: smbXcli_conn_receive_next (smbXcli_base.c:1027)
==7913== by 0xC4DA02D: smbXcli_req_set_pending (smbXcli_base.c:978)
==7913== by 0xC4DF776: smb2cli_req_compound_submit (smbXcli_base.c:3166)
==7913== by 0xC4DFC1D: smb2cli_req_send (smbXcli_base.c:3268)
==7913== by 0xC4F2210: smb2cli_ioctl_send (smb2cli_ioctl.c:149)
==7913==
BUG: https://bugzilla.samba.org/show_bug.cgi?id=11622
Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
libcli/smb/smb2cli_ioctl.c | 84 ++++++++++++++++++++++++++--------------------
1 file changed, 47 insertions(+), 37 deletions(-)
diff --git a/libcli/smb/smb2cli_ioctl.c b/libcli/smb/smb2cli_ioctl.c
index 42a424e..2b572ba 100644
--- a/libcli/smb/smb2cli_ioctl.c
+++ b/libcli/smb/smb2cli_ioctl.c
@@ -22,8 +22,6 @@
#include "lib/util/tevent_ntstatus.h"
#include "smb_common.h"
#include "smbXcli_base.h"
-#include "librpc/ndr/libndr.h"
-#include "librpc/gen_ndr/ioctl.h"
struct smb2cli_ioctl_state {
uint8_t fixed[0x38];
@@ -31,6 +29,7 @@ struct smb2cli_ioctl_state {
uint32_t max_input_length;
uint32_t max_output_length;
struct iovec *recv_iov;
+ bool out_valid;
DATA_BLOB out_input_buffer;
DATA_BLOB out_output_buffer;
uint32_t ctl_code;
@@ -161,32 +160,6 @@ struct tevent_req *smb2cli_ioctl_send(TALLOC_CTX *mem_ctx,
return req;
}
-/*
- * 3.3.4.4 Sending an Error Response
- * An error code other than one of the following indicates a failure:
- */
-static bool smb2cli_ioctl_is_failure(uint32_t ctl_code, NTSTATUS status,
- size_t data_size)
-{
- if (NT_STATUS_IS_OK(status)) {
- return false;
- }
-
- /*
- * STATUS_INVALID_PARAMETER in a FSCTL_SRV_COPYCHUNK or
- * FSCTL_SRV_COPYCHUNK_WRITE Response, when returning an
- * SRV_COPYCHUNK_RESPONSE as described in section 2.2.32.1.
- */
- if (NT_STATUS_EQUAL(status, NT_STATUS_INVALID_PARAMETER) &&
- (ctl_code == FSCTL_SRV_COPYCHUNK ||
- ctl_code == FSCTL_SRV_COPYCHUNK_WRITE) &&
- data_size == sizeof(struct srv_copychunk_rsp)) {
- return false;
- }
-
- return true;
-}
-
static void smb2cli_ioctl_done(struct tevent_req *subreq)
{
struct tevent_req *req =
@@ -225,6 +198,16 @@ static void smb2cli_ioctl_done(struct tevent_req *subreq)
.body_size = 0x09,
},
{
+ /*
+ * a normal error
+ */
+ .status = NT_STATUS_INVALID_PARAMETER,
+ .body_size = 0x09
+ },
+ {
+ /*
+ * a special case for FSCTL_SRV_COPYCHUNK_*
+ */
.status = NT_STATUS_INVALID_PARAMETER,
.body_size = 0x31
},
@@ -233,10 +216,35 @@ static void smb2cli_ioctl_done(struct tevent_req *subreq)
status = smb2cli_req_recv(subreq, state, &iov,
expected, ARRAY_SIZE(expected));
TALLOC_FREE(subreq);
- if (iov == NULL && tevent_req_nterror(req, status)) {
- return;
+ if (NT_STATUS_EQUAL(status, NT_STATUS_INVALID_PARAMETER)) {
+ switch (state->ctl_code) {
+ case FSCTL_SRV_COPYCHUNK:
+ case FSCTL_SRV_COPYCHUNK_WRITE:
+ break;
+ default:
+ tevent_req_nterror(req, status);
+ return;
+ }
+
+ if (iov[1].iov_len != 0x30) {
+ tevent_req_nterror(req,
+ NT_STATUS_INVALID_NETWORK_RESPONSE);
+ return;
+ }
+ } else if (NT_STATUS_EQUAL(status, STATUS_BUFFER_OVERFLOW)) {
+ /* no error */
+ } else {
+ if (tevent_req_nterror(req, status)) {
+ return;
+ }
}
+ /*
+ * At this stage we're sure that got a body size of 0x31,
+ * either with NT_STATUS_OK, STATUS_BUFFER_OVERFLOW or
+ * NT_STATUS_INVALID_PARAMETER.
+ */
+
state->recv_iov = iov;
fixed = (uint8_t *)iov[1].iov_base;
dyn = (uint8_t *)iov[2].iov_base;
@@ -247,11 +255,6 @@ static void smb2cli_ioctl_done(struct tevent_req *subreq)
output_buffer_offset = IVAL(fixed, 0x20);
output_buffer_length = IVAL(fixed, 0x24);
- if (smb2cli_ioctl_is_failure(state->ctl_code, status, output_buffer_length) &&
- tevent_req_nterror(req, status)) {
- return;
- }
-
if ((input_buffer_offset > 0) && (input_buffer_length > 0)) {
uint32_t ofs;
@@ -332,6 +335,8 @@ static void smb2cli_ioctl_done(struct tevent_req *subreq)
state->out_output_buffer.length = output_buffer_length;
}
+ state->out_valid = true;
+
if (tevent_req_nterror(req, status)) {
return;
}
@@ -349,8 +354,13 @@ NTSTATUS smb2cli_ioctl_recv(struct tevent_req *req,
struct smb2cli_ioctl_state);
NTSTATUS status = NT_STATUS_OK;
- if (tevent_req_is_nterror(req, &status) &&
- smb2cli_ioctl_is_failure(state->ctl_code, status, state->out_output_buffer.length)) {
+ if (tevent_req_is_nterror(req, &status) && !state->out_valid) {
+ if (out_input_buffer) {
+ *out_input_buffer = data_blob_null;
+ }
+ if (out_output_buffer) {
+ *out_output_buffer = data_blob_null;
+ }
tevent_req_received(req);
return status;
}
--
1.9.1
From 67d7f9b0d2d4f4fcbb74ee7211456ce4f1ef6228 Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Fri, 27 Nov 2015 19:10:01 +0100
Subject: [PATCH 2/5] libcli/smb: correctly handle STATUS_BUFFER_OVERFLOW in
smb2cli_read*
BUG: https://bugzilla.samba.org/show_bug.cgi?id=11623
Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
libcli/smb/smb2cli_read.c | 26 +++++++++++++++++++++-----
1 file changed, 21 insertions(+), 5 deletions(-)
diff --git a/libcli/smb/smb2cli_read.c b/libcli/smb/smb2cli_read.c
index 4a31622..8110b65 100644
--- a/libcli/smb/smb2cli_read.c
+++ b/libcli/smb/smb2cli_read.c
@@ -29,6 +29,7 @@ struct smb2cli_read_state {
struct iovec *recv_iov;
uint8_t *data;
uint32_t data_length;
+ bool out_valid;
};
static void smb2cli_read_done(struct tevent_req *subreq);
@@ -105,8 +106,12 @@ static void smb2cli_read_done(struct tevent_req *subreq)
status = smb2cli_req_recv(subreq, state, &iov,
expected, ARRAY_SIZE(expected));
TALLOC_FREE(subreq);
- if (tevent_req_nterror(req, status)) {
- return;
+ if (NT_STATUS_EQUAL(status, STATUS_BUFFER_OVERFLOW)) {
+ /* no error */
+ } else {
+ if (tevent_req_nterror(req, status)) {
+ return;
+ }
}
data_offset = CVAL(iov[1].iov_base, 2);
@@ -120,6 +125,13 @@ static void smb2cli_read_done(struct tevent_req *subreq)
state->recv_iov = iov;
state->data = (uint8_t *)iov[2].iov_base;
+
+ state->out_valid = true;
+
+ if (tevent_req_nterror(req, status)) {
+ return;
+ }
+
tevent_req_done(req);
}
@@ -129,15 +141,19 @@ NTSTATUS smb2cli_read_recv(struct tevent_req *req, TALLOC_CTX *mem_ctx,
struct smb2cli_read_state *state =
tevent_req_data(req,
struct smb2cli_read_state);
- NTSTATUS status;
+ NTSTATUS status = NT_STATUS_OK;
- if (tevent_req_is_nterror(req, &status)) {
+ if (tevent_req_is_nterror(req, &status) && !state->out_valid) {
+ *data_length = 0;
+ *data = NULL;
+ tevent_req_received(req);
return status;
}
talloc_steal(mem_ctx, state->recv_iov);
*data_length = state->data_length;
*data = state->data;
- return NT_STATUS_OK;
+ tevent_req_received(req);
+ return status;
}
NTSTATUS smb2cli_read(struct smbXcli_conn *conn,
--
1.9.1
From ea7cd0253801950412a8364a8e3d5f0dbb0dac4b Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Fri, 27 Nov 2015 19:10:01 +0100
Subject: [PATCH 3/5] libcli/smb: correctly handle STATUS_BUFFER_OVERFLOW in
smb2cli_query_info*
BUG: https://bugzilla.samba.org/show_bug.cgi?id=11623
Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
libcli/smb/smb2cli_query_info.c | 24 +++++++++++++++++++-----
1 file changed, 19 insertions(+), 5 deletions(-)
diff --git a/libcli/smb/smb2cli_query_info.c b/libcli/smb/smb2cli_query_info.c
index a24844b..d499611 100644
--- a/libcli/smb/smb2cli_query_info.c
+++ b/libcli/smb/smb2cli_query_info.c
@@ -29,6 +29,7 @@ struct smb2cli_query_info_state {
uint32_t max_output_length;
struct iovec *recv_iov;
DATA_BLOB out_output_buffer;
+ bool out_valid;
};
static void smb2cli_query_info_done(struct tevent_req *subreq);
@@ -135,8 +136,12 @@ static void smb2cli_query_info_done(struct tevent_req *subreq)
status = smb2cli_req_recv(subreq, state, &iov,
expected, ARRAY_SIZE(expected));
TALLOC_FREE(subreq);
- if (tevent_req_nterror(req, status)) {
- return;
+ if (NT_STATUS_EQUAL(status, STATUS_BUFFER_OVERFLOW)) {
+ /* no error */
+ } else {
+ if (tevent_req_nterror(req, status)) {
+ return;
+ }
}
state->recv_iov = iov;
@@ -170,6 +175,12 @@ static void smb2cli_query_info_done(struct tevent_req *subreq)
state->out_output_buffer.length = output_buffer_length;
}
+ state->out_valid = true;
+
+ if (tevent_req_nterror(req, status)) {
+ return;
+ }
+
tevent_req_done(req);
}
@@ -180,9 +191,12 @@ NTSTATUS smb2cli_query_info_recv(struct tevent_req *req,
struct smb2cli_query_info_state *state =
tevent_req_data(req,
struct smb2cli_query_info_state);
- NTSTATUS status;
+ NTSTATUS status = NT_STATUS_OK;
- if (tevent_req_is_nterror(req, &status)) {
+ if (tevent_req_is_nterror(req, &status) && !state->out_valid) {
+ if (out_output_buffer) {
+ *out_output_buffer = data_blob_null;
+ }
tevent_req_received(req);
return status;
}
@@ -193,7 +207,7 @@ NTSTATUS smb2cli_query_info_recv(struct tevent_req *req,
}
tevent_req_received(req);
- return NT_STATUS_OK;
+ return status;
}
NTSTATUS smb2cli_query_info(struct smbXcli_conn *conn,
--
1.9.1
From 2b78aea564279af450aff19df7e19fc89486e743 Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Fri, 27 Nov 2015 19:10:01 +0100
Subject: [PATCH 4/5] libcli/smb: correctly handle STATUS_BUFFER_OVERFLOW in
smb1cli_readx*
BUG: https://bugzilla.samba.org/show_bug.cgi?id=11623
Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
libcli/smb/smb1cli_read.c | 53 +++++++++++++++++++++++++++++++----------------
1 file changed, 35 insertions(+), 18 deletions(-)
diff --git a/libcli/smb/smb1cli_read.c b/libcli/smb/smb1cli_read.c
index ab250ab..d7a7f43 100644
--- a/libcli/smb/smb1cli_read.c
+++ b/libcli/smb/smb1cli_read.c
@@ -26,9 +26,9 @@
struct smb1cli_readx_state {
uint32_t size;
uint16_t vwv[12];
- NTSTATUS status;
uint32_t received;
uint8_t *buf;
+ bool out_valid;
};
static void smb1cli_readx_done(struct tevent_req *subreq);
@@ -131,27 +131,36 @@ static void smb1cli_readx_done(struct tevent_req *subreq)
uint8_t *bytes;
uint16_t data_offset;
uint32_t bytes_offset;
+ NTSTATUS status;
static const struct smb1cli_req_expected_response expected[] = {
{
.status = NT_STATUS_OK,
.wct = 0x0C
},
+ {
+ .status = STATUS_BUFFER_OVERFLOW,
+ .wct = 0x0C
+ },
};
- state->status = smb1cli_req_recv(subreq, state,
- &recv_iov,
- NULL, /* phdr */
- &wct,
- &vwv,
- NULL, /* pvwv_offset */
- &num_bytes,
- &bytes,
- &bytes_offset,
- NULL, /* inbuf */
- expected, ARRAY_SIZE(expected));
+ status = smb1cli_req_recv(subreq, state,
+ &recv_iov,
+ NULL, /* phdr */
+ &wct,
+ &vwv,
+ NULL, /* pvwv_offset */
+ &num_bytes,
+ &bytes,
+ &bytes_offset,
+ NULL, /* inbuf */
+ expected, ARRAY_SIZE(expected));
TALLOC_FREE(subreq);
- if (tevent_req_nterror(req, state->status)) {
- return;
+ if (NT_STATUS_EQUAL(status, STATUS_BUFFER_OVERFLOW)) {
+ /* no error */
+ } else {
+ if (tevent_req_nterror(req, status)) {
+ return;
+ }
}
/* size is the number of bytes the server returned.
@@ -189,6 +198,12 @@ static void smb1cli_readx_done(struct tevent_req *subreq)
state->buf = bytes + (data_offset - bytes_offset);
+ state->out_valid = true;
+
+ if (tevent_req_nterror(req, status)) {
+ return;
+ }
+
tevent_req_done(req);
}
@@ -205,7 +220,7 @@ static void smb1cli_readx_done(struct tevent_req *subreq)
* @param[out] received The number of bytes received.
* @param[out] rcvbuf Pointer to the bytes received.
*
- * @return NT_STATUS_OK on succsess.
+ * @return NT_STATUS_OK or STATUS_BUFFER_OVERFLOW on succsess.
*/
NTSTATUS smb1cli_readx_recv(struct tevent_req *req,
uint32_t *received,
@@ -213,12 +228,14 @@ NTSTATUS smb1cli_readx_recv(struct tevent_req *req,
{
struct smb1cli_readx_state *state = tevent_req_data(
req, struct smb1cli_readx_state);
- NTSTATUS status;
+ NTSTATUS status = NT_STATUS_OK;
- if (tevent_req_is_nterror(req, &status)) {
+ if (tevent_req_is_nterror(req, &status) && !state->out_valid) {
+ *received = 0;
+ *rcvbuf = NULL;
return status;
}
*received = state->received;
*rcvbuf = state->buf;
- return NT_STATUS_OK;
+ return status;
}
--
1.9.1
From ae7be877ec0799913be4e097847d06011dc67a95 Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Fri, 27 Nov 2015 18:19:38 +0100
Subject: [PATCH 5/5] libcli/smb: fix BUFFER_OVERFLOW handling in
tstream_smbXcli_np
The special error is not NT_STATUS_BUFFER_TOO_SMALL, but STATUS_BUFFER_OVERFLOW.
Tested using TSTREAM_SMBXCLI_NP_MAX_BUF_SIZE == 20 and running
the following commands against a Windows 2012R2 server:
bin/smbtorture ncacn_np:SERVER[] rpc.lsa-getuser
bin/smbtorture ncacn_np:SERVER[smb2] rpc.lsa-getuser
BUG: https://bugzilla.samba.org/show_bug.cgi?id=11623
Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
libcli/smb/tstream_smbXcli_np.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/libcli/smb/tstream_smbXcli_np.c b/libcli/smb/tstream_smbXcli_np.c
index 9cd6302..af0863e 100644
--- a/libcli/smb/tstream_smbXcli_np.c
+++ b/libcli/smb/tstream_smbXcli_np.c
@@ -976,7 +976,14 @@ static void tstream_smbXcli_np_readv_trans_done(struct tevent_req *subreq)
received = out_output_buffer.length;
}
TALLOC_FREE(subreq);
- if (NT_STATUS_EQUAL(status, NT_STATUS_BUFFER_TOO_SMALL)) {
+ if (NT_STATUS_EQUAL(status, STATUS_BUFFER_OVERFLOW)) {
+ /*
+ * STATUS_BUFFER_OVERFLOW means that there's
+ * more data to read when the named pipe is used
+ * in message mode (which is the case here).
+ *
+ * But we hide this from the caller.
+ */
status = NT_STATUS_OK;
}
if (!NT_STATUS_IS_OK(status)) {
@@ -1052,9 +1059,9 @@ static void tstream_smbXcli_np_readv_read_done(struct tevent_req *subreq)
* We can't TALLOC_FREE(subreq) as usual here, as rcvbuf still is a
* child of that.
*/
- if (NT_STATUS_EQUAL(status, NT_STATUS_BUFFER_TOO_SMALL)) {
+ if (NT_STATUS_EQUAL(status, STATUS_BUFFER_OVERFLOW)) {
/*
- * NT_STATUS_BUFFER_TOO_SMALL means that there's
+ * STATUS_BUFFER_OVERFLOW means that there's
* more data to read when the named pipe is used
* in message mode (which is the case here).
*
--
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/20151127/61fe60ea/signature.sig>
More information about the samba-technical
mailing list