small valgrind fix
Stefan Metzmacher
metze at samba.org
Fri Nov 27 17:11:42 UTC 2015
Hi Noel,
thanks for the report!
Can you try the attached patch?
David can you also have a look at the patch?
metze
Am 27.11.2015 um 16:38 schrieb Noel Power:
> Hi,
>
> valgrind was complaining [*] on winbindd startup (sometimes) maybe there
> is a better solution that avoids this kindof ugly patch (see attached)
>
> thanks
>
> Noel
>
> [*]
> ==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==
>
-------------- next part --------------
From 170093867c05451b02d3872420fb64a0e519217f 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] 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 | 81 +++++++++++++++++++++++++---------------------
1 file changed, 44 insertions(+), 37 deletions(-)
diff --git a/libcli/smb/smb2cli_ioctl.c b/libcli/smb/smb2cli_ioctl.c
index 42a424e..072a15f 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,32 @@ 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 (tevent_req_nterror(req, status)) {
+ return;
+ }
}
+ /*
+ * At this stage we're sure that got a body size of 0x31,
+ * either with NT_STATUS_OK 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 +252,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 +332,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 +351,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
-------------- 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/1b2db4a6/signature.sig>
More information about the samba-technical
mailing list