[PATCH] Fix some Coverity findings
Jeremy Allison
jra at samba.org
Wed Mar 28 22:19:29 UTC 2018
On Wed, Mar 28, 2018 at 08:56:32PM +0200, Volker Lendecke via samba-technical wrote:
> Hi!
>
> Review appreciated!
LGTM. RB+ and pushed !
Jeremy.
> --
> 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 e9145c2648c13736887a4fb7ae5ede34bb35af13 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Tue, 27 Mar 2018 17:00:46 -0500
> Subject: [PATCH 1/8] libads: Fix CID 1433606 Dereference before null check
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/libads/kerberos_keytab.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/source3/libads/kerberos_keytab.c b/source3/libads/kerberos_keytab.c
> index 85f195abcae..8eb7b2a7c6b 100644
> --- a/source3/libads/kerberos_keytab.c
> +++ b/source3/libads/kerberos_keytab.c
> @@ -93,7 +93,7 @@ static bool fill_default_spns(TALLOC_CTX *ctx, const char *machine_name,
>
> if (*spns == NULL) {
> *spns = talloc_zero_array(ctx, const char*, 3);
> - if (spns == NULL) {
> + if (*spns == NULL) {
> return false;
> }
> }
> --
> 2.11.0
>
>
> From 424825c09ad8d4aa0309920daec8410642b3d3dd Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Tue, 27 Mar 2018 17:03:15 -0500
> Subject: [PATCH 2/8] lsa_server: Fix CID 1433608 Dereference after null check
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source4/rpc_server/lsa/lsa_lookup.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/source4/rpc_server/lsa/lsa_lookup.c b/source4/rpc_server/lsa/lsa_lookup.c
> index 3baff1ec11f..becbcfc8b64 100644
> --- a/source4/rpc_server/lsa/lsa_lookup.c
> +++ b/source4/rpc_server/lsa/lsa_lookup.c
> @@ -88,7 +88,7 @@ static NTSTATUS dcesrv_lsa_lookup_name(struct lsa_policy_state *state,
> bool match = false;
> int ret;
>
> - if (principal == NULL && principal[0] == '\0') {
> + if ((principal == NULL) || (principal[0] == '\0')) {
> return NT_STATUS_NONE_MAPPED;
> }
>
> --
> 2.11.0
>
>
> From 7d2f96d8c2e01452968411b17d6cfe627e7122fe Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Wed, 28 Mar 2018 07:00:34 -0500
> Subject: [PATCH 3/8] libads: Fix 1433611 Resource leak
>
> Not really a memleak due to the passed-in talloc ctx, but this way it's cleaner
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/libads/ldap.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/source3/libads/ldap.c b/source3/libads/ldap.c
> index 78b813c67d0..70166f2f594 100644
> --- a/source3/libads/ldap.c
> +++ b/source3/libads/ldap.c
> @@ -3471,6 +3471,7 @@ out:
> if (name != NULL) {
> ok = (strlen(name) > 0);
> }
> + TALLOC_FREE(name);
> return ok;
> }
>
> --
> 2.11.0
>
>
> From eba5e6b79a208cf2ffb48f9addc2b5ac1160c15a Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Wed, 28 Mar 2018 07:10:59 -0500
> Subject: [PATCH 4/8] dsdb: Fix CID 1433614 Dereference after null check
>
> This whole routine assumes module!=NULL, both in the successful as
> well as in error cases. So checking for module!=NULL is confusing both
> the reader as well as Coverity.
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source4/dsdb/samdb/ldb_modules/partition.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/source4/dsdb/samdb/ldb_modules/partition.c b/source4/dsdb/samdb/ldb_modules/partition.c
> index 422ed369ff5..37e714d6e1b 100644
> --- a/source4/dsdb/samdb/ldb_modules/partition.c
> +++ b/source4/dsdb/samdb/ldb_modules/partition.c
> @@ -1016,7 +1016,8 @@ int partition_del_trans(struct ldb_module *module)
> struct partition_private_data);
>
> for (i=0; data && data->partitions && data->partitions[i]; i++) {
> - if ((module && ldb_module_flags(ldb_module_get_ctx(module)) & LDB_FLG_ENABLE_TRACING)) {
> + if (ldb_module_flags(ldb_module_get_ctx(module)) &
> + LDB_FLG_ENABLE_TRACING) {
> ldb_debug(ldb_module_get_ctx(module), LDB_DEBUG_TRACE, "partition_del_trans() -> %s",
> ldb_dn_get_linearized(data->partitions[i]->ctrl->dn));
> }
> @@ -1035,7 +1036,8 @@ int partition_del_trans(struct ldb_module *module)
> }
> data->in_transaction--;
>
> - if ((module && ldb_module_flags(ldb_module_get_ctx(module)) & LDB_FLG_ENABLE_TRACING)) {
> + if (ldb_module_flags(ldb_module_get_ctx(module)) &
> + LDB_FLG_ENABLE_TRACING) {
> ldb_debug(ldb_module_get_ctx(module), LDB_DEBUG_TRACE, "partition_del_trans() -> (metadata partition)");
> }
> ret = ldb_next_del_trans(module);
> --
> 2.11.0
>
>
> From 0702e5798bfee6b6133a7eadb0d4f7e41e12dd02 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Wed, 28 Mar 2018 07:17:59 -0500
> Subject: [PATCH 5/8] vfs_fruit: Fix CID 1433613 Operands don't affect result
>
> Two changes: First, we can't check multiplication overflow after the
> operation. This has to be done before with a division. Second, there
> is no OFF_T_MAX, and both operands are size_t, so check for
> SIZE_MAX. The result is assigned to off_t, but I'm not sure where the
> automatic coercion from size_t to off_t would happen.
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/modules/vfs_fruit.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/source3/modules/vfs_fruit.c b/source3/modules/vfs_fruit.c
> index 1a05d0bae34..4299583b21b 100644
> --- a/source3/modules/vfs_fruit.c
> +++ b/source3/modules/vfs_fruit.c
> @@ -6515,12 +6515,12 @@ static bool fruit_tmsize_do_dirent(vfs_handle_struct *handle,
> return true;
> }
>
> - tm_size = bandsize * nbands;
> - if (tm_size > UINT64_MAX) {
> + if (bandsize > SIZE_MAX/nbands) {
> DBG_ERR("tmsize overflow: bandsize [%zu] nbands [%zu]\n",
> bandsize, nbands);
> return false;
> }
> + tm_size = bandsize * nbands;
>
> if (state->total_size + tm_size < state->total_size) {
> DBG_ERR("tmsize overflow: bandsize [%zu] nbands [%zu]\n",
> --
> 2.11.0
>
>
> From b16a84221266401d1b601339e24305fe791edfe6 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Wed, 28 Mar 2018 07:22:02 -0500
> Subject: [PATCH 6/8] smbstatus: Fix CID 1128560 Dereference null return value
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/utils/status.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/source3/utils/status.c b/source3/utils/status.c
> index beae85c4d3e..6370f7002e8 100644
> --- a/source3/utils/status.c
> +++ b/source3/utils/status.c
> @@ -528,6 +528,7 @@ int main(int argc, const char *argv[])
> };
> TALLOC_CTX *frame = talloc_stackframe();
> int ret = 0;
> + struct tevent_context *ev;
> struct messaging_context *msg_ctx = NULL;
> char *db_path;
> bool ok;
> @@ -618,7 +619,14 @@ int main(int argc, const char *argv[])
> * This implicitly initializes the global ctdbd connection,
> * usable by the db_open() calls further down.
> */
> - msg_ctx = messaging_init(NULL, samba_tevent_context_init(NULL));
> + ev = samba_tevent_context_init(NULL);
> + if (ev == NULL) {
> + fprintf(stderr, "samba_tevent_context_init failed\n");
> + ret = -1;
> + goto done;
> + }
> +
> + msg_ctx = messaging_init(NULL, ev);
> if (msg_ctx == NULL) {
> fprintf(stderr, "messaging_init failed\n");
> ret = -1;
> --
> 2.11.0
>
>
> From 5c5cd491e7ea5ea8522ce3578b84160ea24491b9 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Wed, 28 Mar 2018 07:24:01 -0500
> Subject: [PATCH 7/8] net: Fix CID 1128559 Dereference null return value
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/utils/net.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/source3/utils/net.c b/source3/utils/net.c
> index bde261670b5..44daa6088ca 100644
> --- a/source3/utils/net.c
> +++ b/source3/utils/net.c
> @@ -915,6 +915,7 @@ static struct functable net_func[] = {
> const char **argv_const = discard_const_p(const char *, argv);
> poptContext pc;
> TALLOC_CTX *frame = talloc_stackframe();
> + struct tevent_context *ev;
> struct net_context *c = talloc_zero(frame, struct net_context);
> NTSTATUS status;
>
> @@ -1036,9 +1037,12 @@ static struct functable net_func[] = {
> exit(1);
> }
>
> - status = messaging_init_client(c,
> - samba_tevent_context_init(c),
> - &c->msg_ctx);
> + ev = samba_tevent_context_init(c);
> + if (ev == NULL) {
> + d_fprintf(stderr, "samba_tevent_context_init failed\n");
> + exit(1);
> + }
> + status = messaging_init_client(c, ev, &c->msg_ctx);
> if (geteuid() != 0 &&
> NT_STATUS_EQUAL(status, NT_STATUS_ACCESS_DENIED)) {
> /*
> --
> 2.11.0
>
>
> From 1069b3f74bd84533ed7f7b9510305d22cba86a6b Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Wed, 28 Mar 2018 07:27:08 -0500
> Subject: [PATCH 8/8] libads: Fix CID 1349423 Resource leak
>
> get_sorted_dc_list should already take care, but this way it's safer
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/libads/ldap.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/source3/libads/ldap.c b/source3/libads/ldap.c
> index 70166f2f594..f9463043cc8 100644
> --- a/source3/libads/ldap.c
> +++ b/source3/libads/ldap.c
> @@ -412,7 +412,7 @@ static NTSTATUS resolve_and_ping_dns(ADS_STRUCT *ads, const char *sitename,
> const char *realm)
> {
> int count;
> - struct ip_service *ip_list;
> + struct ip_service *ip_list = NULL;
> NTSTATUS status = NT_STATUS_UNSUCCESSFUL;
>
> DEBUG(6, ("resolve_and_ping_dns: (cldap) looking for realm '%s'\n",
> @@ -421,6 +421,7 @@ static NTSTATUS resolve_and_ping_dns(ADS_STRUCT *ads, const char *sitename,
> status = get_sorted_dc_list(realm, sitename, &ip_list, &count,
> true);
> if (!NT_STATUS_IS_OK(status)) {
> + SAFE_FREE(ip_list);
> return status;
> }
>
> --
> 2.11.0
>
More information about the samba-technical
mailing list