[PATCHSET] Several fixes found by our tools
Andreas Schneider
asn at samba.org
Wed May 16 17:20:41 UTC 2018
Hi,
I've packaged Samba for the next RHEL version and our internal tools found
several issues. Attached is a patchset which mostly fixes complaints by
Coverity.
Please review and push if OK.
Thanks,
Andreas
--
Andreas Schneider GPG-ID: CC014E3D
Samba Team asn at samba.org
www.samba.org
-------------- next part --------------
>From 3f321a30535d76ac7a3208c8fb62a505b9ae0402 Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Wed, 16 May 2018 11:53:05 +0200
Subject: [PATCH 01/10] s4:dsdb:tests: Add return code check
Found by Coverity.
Signed-off-by: Andreas Schneider <asn at samba.org>
---
source4/dsdb/samdb/ldb_modules/tests/test_unique_object_sids.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/source4/dsdb/samdb/ldb_modules/tests/test_unique_object_sids.c b/source4/dsdb/samdb/ldb_modules/tests/test_unique_object_sids.c
index dfc6d49cfa7..f9065e4e489 100644
--- a/source4/dsdb/samdb/ldb_modules/tests/test_unique_object_sids.c
+++ b/source4/dsdb/samdb/ldb_modules/tests/test_unique_object_sids.c
@@ -405,11 +405,12 @@ static void test_modify_of_objectSID_replicated(void **state)
assert_non_null(request);
original_request = request;
- ldb_request_add_control(
+ rc = ldb_request_add_control(
request,
DSDB_CONTROL_REPLICATED_UPDATE_OID,
false,
NULL);
+ assert_int_equal(rc, LDB_SUCCESS);
rc = unique_object_sids_modify(test_ctx->module, request);
--
2.16.3
>From faee5eede7be0bd9a26e721f90268618e68309f0 Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Wed, 16 May 2018 11:59:09 +0200
Subject: [PATCH 02/10] s3:winbind: Add sanity check when closing fd
Found by Coverity.
Signed-off-by: Andreas Schneider <asn at samba.org>
---
source3/winbindd/winbindd_cm.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/source3/winbindd/winbindd_cm.c b/source3/winbindd/winbindd_cm.c
index 9c2773d5d67..52b4c2c689f 100644
--- a/source3/winbindd/winbindd_cm.c
+++ b/source3/winbindd/winbindd_cm.c
@@ -1738,8 +1738,10 @@ static bool find_new_dc(TALLOC_CTX *mem_ctx,
TALLOC_FREE(addrs);
num_addrs = 0;
- close(*fd);
- *fd = -1;
+ if (*fd != -1) {
+ close(*fd);
+ *fd = -1;
+ }
goto again;
}
--
2.16.3
>From 4a702aa8148105a4bc9c54c98fee74c225d780aa Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Wed, 16 May 2018 12:05:40 +0200
Subject: [PATCH 03/10] ctdb: Check return values of tevent_req_set_endtime()
Found by Coverity.
Signed-off-by: Andreas Schneider <asn at samba.org>
---
ctdb/client/client_control.c | 4 +++-
ctdb/client/client_tunnel.c | 8 ++++++--
ctdb/common/sock_client.c | 4 +++-
3 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/ctdb/client/client_control.c b/ctdb/client/client_control.c
index 1755eccf65d..ab0aac8baf5 100644
--- a/ctdb/client/client_control.c
+++ b/ctdb/client/client_control.c
@@ -112,7 +112,9 @@ struct tevent_req *ctdb_client_control_send(TALLOC_CTX *mem_ctx,
}
if (!tevent_timeval_is_zero(&timeout)) {
- tevent_req_set_endtime(req, ev, timeout);
+ if (!tevent_req_set_endtime(req, ev, timeout)) {
+ return tevent_req_post(req, ev);
+ }
}
subreq = comm_write_send(state, ev, client->comm, buf, buflen);
diff --git a/ctdb/client/client_tunnel.c b/ctdb/client/client_tunnel.c
index 17b65469ae9..0bd7a3aff8c 100644
--- a/ctdb/client/client_tunnel.c
+++ b/ctdb/client/client_tunnel.c
@@ -457,7 +457,9 @@ struct tevent_req *ctdb_tunnel_request_send(TALLOC_CTX *mem_ctx,
}
if (!tevent_timeval_is_zero(&timeout)) {
- tevent_req_set_endtime(req, ev, timeout);
+ if (!tevent_req_set_endtime(req, ev, timeout)) {
+ return tevent_req_post(req, ev);
+ }
}
subreq = comm_write_send(state, ev, tctx->client->comm,
@@ -619,7 +621,9 @@ struct tevent_req *ctdb_tunnel_reply_send(TALLOC_CTX *mem_ctx,
}
if (!tevent_timeval_is_zero(&timeout)) {
- tevent_req_set_endtime(req, ev, timeout);
+ if (!tevent_req_set_endtime(req, ev, timeout)) {
+ return tevent_req_post(req, ev);
+ }
}
subreq = comm_write_send(state, ev, tctx->client->comm, pkt, pkt_len);
diff --git a/ctdb/common/sock_client.c b/ctdb/common/sock_client.c
index ced705042a4..75f471fc5de 100644
--- a/ctdb/common/sock_client.c
+++ b/ctdb/common/sock_client.c
@@ -247,7 +247,9 @@ struct tevent_req *sock_client_msg_send(TALLOC_CTX *mem_ctx,
tevent_req_set_callback(subreq, sock_client_msg_done, req);
if (! timeval_is_zero(&timeout)) {
- tevent_req_set_endtime(req, ev, timeout);
+ if (!tevent_req_set_endtime(req, ev, timeout)) {
+ return tevent_req_post(req, ev);
+ }
}
return req;
--
2.16.3
>From cb5fa4d9c13339e19007aaa1a9cde584331153d1 Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Wed, 16 May 2018 12:23:15 +0200
Subject: [PATCH 04/10] s3:modules: Improve log function in vfs_full_audit
Do not allocate empty strings which result in zero-length printf
warnings.
Found by: gcc -Wformat-zero-length
Signed-off-by: Andreas Schneider <asn at samba.org>
---
source3/modules/vfs_full_audit.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/source3/modules/vfs_full_audit.c b/source3/modules/vfs_full_audit.c
index ee8dbbcff2c..d6d03ae1eb9 100644
--- a/source3/modules/vfs_full_audit.c
+++ b/source3/modules/vfs_full_audit.c
@@ -530,7 +530,8 @@ static void do_log(vfs_op_type op, bool success, vfs_handle_struct *handle,
fstring err_msg;
char *audit_pre = NULL;
va_list ap;
- char *op_msg = NULL;
+ char *msg = NULL;
+ const char *op_msg = "";
SMB_VFS_HANDLE_GET_DATA(handle, pd,
struct vfs_full_audit_private_data,
@@ -547,12 +548,15 @@ static void do_log(vfs_op_type op, bool success, vfs_handle_struct *handle,
else
fstr_sprintf(err_msg, "fail (%s)", strerror(errno));
- va_start(ap, format);
- op_msg = talloc_vasprintf(talloc_tos(), format, ap);
- va_end(ap);
+ if (format == NULL || strlen(format) == 0) {
+ va_start(ap, format);
+ msg = talloc_vasprintf(talloc_tos(), format, ap);
+ va_end(ap);
- if (!op_msg) {
- goto out;
+ if (msg == NULL) {
+ goto out;
+ }
+ op_msg = msg;
}
audit_pre = audit_prefix(talloc_tos(), handle->conn);
@@ -576,7 +580,7 @@ static void do_log(vfs_op_type op, bool success, vfs_handle_struct *handle,
}
out:
TALLOC_FREE(audit_pre);
- TALLOC_FREE(op_msg);
+ TALLOC_FREE(msg);
TALLOC_FREE(tmp_do_log_ctx);
}
--
2.16.3
>From 49bf54f3cc51b6e5fa0c9a17f50efc434b112342 Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Wed, 16 May 2018 12:10:29 +0200
Subject: [PATCH 05/10] s3:winbind: Initialize validation_level in
winbind_dual_SamLogon()
Found by Covertiy.
Signed-off-by: Andreas Schneider <asn at samba.org>
---
source3/winbindd/winbindd_pam.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/source3/winbindd/winbindd_pam.c b/source3/winbindd/winbindd_pam.c
index a7e16815ec5..25564277f0a 100644
--- a/source3/winbindd/winbindd_pam.c
+++ b/source3/winbindd/winbindd_pam.c
@@ -2247,7 +2247,7 @@ NTSTATUS winbind_dual_SamLogon(struct winbindd_domain *domain,
uint16_t *_validation_level,
union netr_Validation **_validation)
{
- uint16_t validation_level;
+ uint16_t validation_level = 0;
union netr_Validation *validation = NULL;
NTSTATUS result;
--
2.16.3
>From 7d17b351cd5308a21b3fbad110374c310c8d7b70 Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Wed, 16 May 2018 12:11:30 +0200
Subject: [PATCH 06/10] s3:modules: Initialize pointers in vfs_virusfilter
Found by Coverity.
Signed-off-by: Andreas Schneider <asn at samba.org>
---
source3/modules/vfs_virusfilter_sophos.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/source3/modules/vfs_virusfilter_sophos.c b/source3/modules/vfs_virusfilter_sophos.c
index 72051cd64a2..82f9cbce749 100644
--- a/source3/modules/vfs_virusfilter_sophos.c
+++ b/source3/modules/vfs_virusfilter_sophos.c
@@ -234,7 +234,7 @@ static virusfilter_result virusfilter_sophos_scan(
virusfilter_result result = VIRUSFILTER_RESULT_ERROR;
char *report = NULL;
char *reply = NULL;
- char *reply_token, *reply_saveptr;
+ char *reply_token = NULL, *reply_saveptr = NULL;
int ret;
bool ok;
--
2.16.3
>From fe2a70a6f8420f6bb65df06551fec5764b47202d Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Wed, 16 May 2018 14:06:36 +0200
Subject: [PATCH 07/10] s3:winbind: Check if we have an open file descriptor
Found by Coverity.
Signed-off-by: Andreas Schneider <asn at samba.org>
---
source3/winbindd/winbindd_cm.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/source3/winbindd/winbindd_cm.c b/source3/winbindd/winbindd_cm.c
index 52b4c2c689f..95612034d2f 100644
--- a/source3/winbindd/winbindd_cm.c
+++ b/source3/winbindd/winbindd_cm.c
@@ -1983,7 +1983,10 @@ static NTSTATUS cm_open_connection(struct winbindd_domain *domain,
&new_conn->cli, &retry);
if (!NT_STATUS_IS_OK(result)) {
/* Don't leak the smb connection socket */
- close(fd);
+ if (fd != -1) {
+ close(fd);
+ fd = -1;
+ }
}
if (!retry)
--
2.16.3
>From a06a35afb3b8d9366e07ca11dc70164517345646 Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Wed, 16 May 2018 15:06:02 +0200
Subject: [PATCH 08/10] s4:torture: Make sure variable is initialized in oplock
test
Signed-off-by: Andreas Schneider <asn at samba.org>
---
source4/torture/smb2/oplock.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/source4/torture/smb2/oplock.c b/source4/torture/smb2/oplock.c
index cb1b5edddfe..6e53007bcfc 100644
--- a/source4/torture/smb2/oplock.c
+++ b/source4/torture/smb2/oplock.c
@@ -4976,7 +4976,8 @@ static void child_sig_term_handler(struct tevent_context *ev,
void *private_data)
{
int *pstatus = (int *)private_data;
- int status;
+ int status = 0;
+
wait(&status);
if (WIFEXITED(status)) {
*pstatus = WEXITSTATUS(status);
--
2.16.3
>From e3e03e1eb69c62605dc923170b7aa98a79906a07 Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Wed, 16 May 2018 16:54:47 +0200
Subject: [PATCH 09/10] libcli: Fix coverity warning in smb2cli_notify_send()
result_independent_of_operands: "(uint16_t)(recursive ? 1 : 0) >> 8" is
0 regardless of the values of its operands. This occurs as the operand
of assignment.
Found by Coverity.
Pair-Programmed-With: Ralph Boehme <slow at samba.org>
Signed-off-by: Andreas Schneider <asn at samba.org>
Signed-off-by: Ralph Boehme <slow at samba.org>
---
libcli/smb/smb2cli_notify.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/libcli/smb/smb2cli_notify.c b/libcli/smb/smb2cli_notify.c
index 34329ba16cc..1a2a2793a32 100644
--- a/libcli/smb/smb2cli_notify.c
+++ b/libcli/smb/smb2cli_notify.c
@@ -52,15 +52,18 @@ struct tevent_req *smb2cli_notify_send(TALLOC_CTX *mem_ctx,
struct tevent_req *req, *subreq;
struct smb2cli_notify_state *state;
uint8_t *fixed;
+ uint16_t watch_tree;
req = tevent_req_create(mem_ctx, &state,
struct smb2cli_notify_state);
if (req == NULL) {
return NULL;
}
+
+ watch_tree = recursive ? SMB2_WATCH_TREE : 0;
fixed = state->fixed;
SSVAL(fixed, 0, 32);
- SSVAL(fixed, 2, recursive ? SMB2_WATCH_TREE : 0);
+ SSVAL(fixed, 2, watch_tree);
SIVAL(fixed, 4, output_buffer_length);
SBVAL(fixed, 8, fid_persistent);
SBVAL(fixed, 16, fid_volatile);
--
2.16.3
>From 813226393c8aee098cfc2be21366da6973a82881 Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Wed, 16 May 2018 17:05:38 +0200
Subject: [PATCH 10/10] s3:smbd: Fix converity warning with _smb_setlen_large()
result_independent_of_operands: "(outsize - 4 & 0xffffff) >> 16 >> 8" is
0 regardless of the values of its operands. This occurs as the bitwise
first operand of "&".
So we should just pass a variable to silence the warning. However for
this, we should calculate it correctly and use size_t for it.
Found by Coverity.
Signed-off-by: Andreas Schneider <asn at samba.org>
---
source3/smbd/aio.c | 12 ++++++++++--
source3/smbd/error.c | 4 ++--
source3/smbd/process.c | 8 ++++----
source3/smbd/proto.h | 17 +++++++++++------
source3/smbd/reply.c | 4 ++--
5 files changed, 29 insertions(+), 16 deletions(-)
diff --git a/source3/smbd/aio.c b/source3/smbd/aio.c
index abf8858099b..b984036e9f8 100644
--- a/source3/smbd/aio.c
+++ b/source3/smbd/aio.c
@@ -239,7 +239,7 @@ static void aio_pread_smb1_done(struct tevent_req *req)
struct aio_extra *aio_ex = tevent_req_callback_data(
req, struct aio_extra);
files_struct *fsp = aio_ex->fsp;
- int outsize;
+ size_t outsize;
char *outbuf = (char *)aio_ex->outbuf.data;
ssize_t nread;
struct vfs_aio_state vfs_aio_state;
@@ -276,7 +276,15 @@ static void aio_pread_smb1_done(struct tevent_req *req)
(int)aio_ex->nbyte, (int)nread ) );
}
- _smb_setlen_large(outbuf, outsize - 4);
+
+ if (outsize <= 4) {
+ DBG_INFO("Invalid outsize (%zu)\n", outsize);
+ TALLOC_FREE(aio_ex);
+ return;
+ }
+ outsize -= 4;
+ _smb_setlen_large(outbuf, outsize);
+
show_msg(outbuf);
if (!srv_send_smb(aio_ex->smbreq->xconn, outbuf,
true, aio_ex->smbreq->seqnum+1,
diff --git a/source3/smbd/error.c b/source3/smbd/error.c
index c91f5b0daf4..3f9ecaa2c5c 100644
--- a/source3/smbd/error.c
+++ b/source3/smbd/error.c
@@ -105,9 +105,9 @@ void error_packet_set(char *outbuf, uint8_t eclass, uint32_t ecode, NTSTATUS nts
}
}
-int error_packet(char *outbuf, uint8_t eclass, uint32_t ecode, NTSTATUS ntstatus, int line, const char *file)
+size_t error_packet(char *outbuf, uint8_t eclass, uint32_t ecode, NTSTATUS ntstatus, int line, const char *file)
{
- int outsize = srv_set_message(outbuf,0,0,True);
+ size_t outsize = srv_set_message(outbuf,0,0,True);
error_packet_set(outbuf, eclass, ecode, ntstatus, line, file);
return outsize;
}
diff --git a/source3/smbd/process.c b/source3/smbd/process.c
index 6a3395ceabf..936b5351de7 100644
--- a/source3/smbd/process.c
+++ b/source3/smbd/process.c
@@ -274,10 +274,10 @@ out:
Setup the word count and byte count for a smb message.
********************************************************************/
-int srv_set_message(char *buf,
- int num_words,
- int num_bytes,
- bool zero)
+size_t srv_set_message(char *buf,
+ size_t num_words,
+ size_t num_bytes,
+ bool zero)
{
if (zero && (num_words || num_bytes)) {
memset(buf + smb_size,'\0',num_words*2 + num_bytes);
diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h
index 778561c241c..bee7acadeea 100644
--- a/source3/smbd/proto.h
+++ b/source3/smbd/proto.h
@@ -293,7 +293,12 @@ struct timespec get_change_timespec(connection_struct *conn,
bool use_nt_status(void);
void error_packet_set(char *outbuf, uint8_t eclass, uint32_t ecode, NTSTATUS ntstatus, int line, const char *file);
-int error_packet(char *outbuf, uint8_t eclass, uint32_t ecode, NTSTATUS ntstatus, int line, const char *file);
+size_t error_packet(char *outbuf,
+ uint8_t eclass,
+ uint32_t ecode,
+ NTSTATUS ntstatus,
+ int line,
+ const char *file);
void reply_nt_error(struct smb_request *req, NTSTATUS ntstatus,
int line, const char *file);
void reply_force_dos_error(struct smb_request *req, uint8_t eclass, uint32_t ecode,
@@ -825,10 +830,10 @@ bool srv_send_smb(struct smbXsrv_connection *xconn, char *buffer,
bool no_signing, uint32_t seqnum,
bool do_encrypt,
struct smb_perfcount_data *pcd);
-int srv_set_message(char *buf,
- int num_words,
- int num_bytes,
- bool zero);
+size_t srv_set_message(char *buf,
+ size_t num_words,
+ size_t num_bytes,
+ bool zero);
void remove_deferred_open_message_smb(struct smbXsrv_connection *xconn,
uint64_t mid);
bool schedule_deferred_open_message_smb(struct smbXsrv_connection *xconn,
@@ -955,7 +960,7 @@ ssize_t sendfile_short_send(struct smbXsrv_connection *xconn,
size_t smb_maxcnt);
void reply_readbraw(struct smb_request *req);
void reply_lockread(struct smb_request *req);
-int setup_readX_header(char *outbuf, size_t smb_maxcnt);
+size_t setup_readX_header(char *outbuf, size_t smb_maxcnt);
void reply_read(struct smb_request *req);
void reply_read_and_X(struct smb_request *req);
void error_to_writebrawerr(struct smb_request *req);
diff --git a/source3/smbd/reply.c b/source3/smbd/reply.c
index 92a65f5b90b..fc56e3234be 100644
--- a/source3/smbd/reply.c
+++ b/source3/smbd/reply.c
@@ -3926,9 +3926,9 @@ out:
Setup readX header.
****************************************************************************/
-int setup_readX_header(char *outbuf, size_t smb_maxcnt)
+size_t setup_readX_header(char *outbuf, size_t smb_maxcnt)
{
- int outsize;
+ size_t outsize;
outsize = srv_set_message(outbuf,12,smb_maxcnt + 1 /* padding byte */,
False);
--
2.16.3
More information about the samba-technical
mailing list