[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