small valgrind fix
Jeremy Allison
jra at samba.org
Mon Nov 30 23:39:11 UTC 2015
On Fri, Nov 27, 2015 at 07:32:25PM +0100, Stefan Metzmacher wrote:
> 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.
LGTM. Reviewed-by: Jeremy Allison <jra at samba.org>
> 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
>
More information about the samba-technical
mailing list