[PATCH] Fix bug 13629 plus a few small cleanups

Jeremy Allison jra at samba.org
Mon Oct 8 20:17:33 UTC 2018


On Sun, Oct 07, 2018 at 08:30:47PM +0200, Volker Lendecke via samba-technical wrote:
> Hi!
> 
> Review appreciated!

RB+. LGTM and pushed, thanks for helping out the Solaris/HPUX's of
this world :-).

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 f50a1e4743776fe17b25deecc7ee23aec0d201da Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Sun, 7 Oct 2018 14:47:26 +0200
> Subject: [PATCH 1/4] lib: Avoid the use of open_memstream in
>  tevent_req_profile_string
> 
> Solaris does not have it.
> 
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=13629
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
>  lib/tevent/test_req.c         | 15 +++++++-
>  lib/util/tevent_req_profile.c | 85 ++++++++++++++++++-------------------------
>  lib/util/tevent_req_profile.h |  8 +---
>  3 files changed, 51 insertions(+), 57 deletions(-)
> 
> diff --git a/lib/tevent/test_req.c b/lib/tevent/test_req.c
> index 565ef31024f..2274a8cadc8 100644
> --- a/lib/tevent/test_req.c
> +++ b/lib/tevent/test_req.c
> @@ -170,6 +170,7 @@ static bool test_tevent_req_profile2(struct torture_context *tctx,
>  	pid_t pid1, pid2;
>  	enum tevent_req_state state1, state2;
>  	uint64_t err1, err2;
> +	char *printstring;
>  	ssize_t pack_len;
>  	int err;
>  	bool ok;
> @@ -189,7 +190,12 @@ static bool test_tevent_req_profile2(struct torture_context *tctx,
>  	TALLOC_FREE(req);
>  	TALLOC_FREE(ev);
>  
> -	tevent_req_profile_print(p1, stdout, 0, UINT_MAX);
> +	printstring = tevent_req_profile_string(tctx, p1, 0, UINT_MAX);
> +	torture_assert_not_null(
> +		tctx,
> +		printstring,
> +		"tevent_req_profile_string failed\n");
> +	printf("%s\n", printstring);
>  
>  	pack_len = tevent_req_profile_pack(p1, NULL, 0);
>  	torture_assert(tctx, pack_len>0, "profile_pack failed\n");
> @@ -212,7 +218,12 @@ static bool test_tevent_req_profile2(struct torture_context *tctx,
>  					 "profile_unpack failed\n");
>  	}
>  
> -	tevent_req_profile_print(p2, stdout, 0, UINT_MAX);
> +	printstring = tevent_req_profile_string(tctx, p2, 0, UINT_MAX);
> +	torture_assert_not_null(
> +		tctx,
> +		printstring,
> +		"tevent_req_profile_string failed\n");
> +	printf("%s\n", printstring);
>  
>  	tevent_req_profile_get_name(p1, &str1);
>  	tevent_req_profile_get_name(p2, &str2);
> diff --git a/lib/util/tevent_req_profile.c b/lib/util/tevent_req_profile.c
> index 522741c5ede..2d280f78f32 100644
> --- a/lib/util/tevent_req_profile.c
> +++ b/lib/util/tevent_req_profile.c
> @@ -29,10 +29,11 @@
>  #include "lib/util/time_basic.h"
>  #include "lib/util/memory.h"
>  
> -int tevent_req_profile_print(const struct tevent_req_profile *profile,
> -			     FILE *fp,
> -			     unsigned indent,
> -			     unsigned max_indent)
> +static bool tevent_req_profile_string_internal(
> +	const struct tevent_req_profile *profile,
> +	unsigned indent,
> +	unsigned max_indent,
> +	char **string)
>  {
>  	struct timeval start, stop, diff;
>  	struct timeval_buf start_buf, stop_buf;
> @@ -44,7 +45,7 @@ int tevent_req_profile_print(const struct tevent_req_profile *profile,
>  	const char *state_buf = NULL;
>  	uint64_t user_error;
>  	const struct tevent_req_profile *sub = NULL;
> -	int ret;
> +	char *result;
>  
>  	tevent_req_profile_get_name(profile, &req_name);
>  
> @@ -85,8 +86,8 @@ int tevent_req_profile_print(const struct tevent_req_profile *profile,
>  		break;
>  	}
>  
> -	ret = fprintf(
> -		fp,
> +	result = talloc_asprintf_append_buffer(
> +		*string,
>  		"%*s[%s] %s [%s] %s [%s] [%ju.%.6ju] -> %s (%d %"PRIu64"))\n",
>  		indent,
>  		"",
> @@ -100,72 +101,58 @@ int tevent_req_profile_print(const struct tevent_req_profile *profile,
>  		state_buf,
>  		(int)state,
>  		user_error);
> -
> -	if (ret < 0) {
> -		return ret;
> +	if (result == NULL) {
> +		return false;
>  	}
> +	*string = result;
>  
>  	indent += 1;
>  
>  	if (indent >= max_indent) {
> -		return ret;
> +		return true;
>  	}
>  
>  	for (sub = tevent_req_profile_get_subprofiles(profile);
>  	     sub != NULL;
>  	     sub = tevent_req_profile_next(sub)) {
> -		int subret;
> -
> -		subret = tevent_req_profile_print(sub, fp, indent, max_indent);
> -		if (subret < 0) {
> -			return subret;
> -		}
> -
> -		ret += subret;
> -
> -		if (ret < subret) {
> -			/* overflow */
> -			return -1;
> +		bool ret;
> +
> +		ret = tevent_req_profile_string_internal(
> +			sub,
> +			indent,
> +			max_indent,
> +			string);
> +		if (!ret) {
> +			return false;
>  		}
>  	}
>  
> -	return ret;
> +	return true;
>  }
>  
> -char *tevent_req_profile_string(const struct tevent_req_profile *profile,
> -				TALLOC_CTX *mem_ctx,
> +char *tevent_req_profile_string(TALLOC_CTX *mem_ctx,
> +				const struct tevent_req_profile *profile,
>  				unsigned indent,
>  				unsigned max_indent)
>  {
> -	FILE *fp = NULL;
> -	char *buf = NULL;
> -	size_t buflen = 0;
> -	char *result = NULL;
> -	int ret;
> +	char *result;
> +	bool ret;
>  
> -	fp = open_memstream(&buf, &buflen);
> -	if (fp == NULL) {
> +	result = talloc_strdup(mem_ctx, "");
> +	if (result == NULL) {
>  		return NULL;
>  	}
>  
> -	ret = tevent_req_profile_print(profile, fp, 0, max_indent);
> -	if (ret < 0) {
> -		goto done;
> -	}
> -
> -	ret = fclose(fp);
> -	if (ret != 0) {
> -		goto done;
> +	ret = tevent_req_profile_string_internal(
> +		profile,
> +		indent,
> +		max_indent,
> +		&result);
> +	if (!ret) {
> +		TALLOC_FREE(result);
> +		return NULL;
>  	}
>  
> -	/*
> -	 * A FILE* from open_memstream maintains the 0-byte at the end
> -	 * beyond the reported length.
> -	 */
> -	result = talloc_memdup(mem_ctx, buf, buflen+1);
> -
> -done:
> -	SAFE_FREE(buf);
>  	return result;
>  }
>  
> diff --git a/lib/util/tevent_req_profile.h b/lib/util/tevent_req_profile.h
> index 00dbc5a9cc2..3bcdbc1a289 100644
> --- a/lib/util/tevent_req_profile.h
> +++ b/lib/util/tevent_req_profile.h
> @@ -29,12 +29,8 @@
>  #include "replace.h"
>  #include <tevent.h>
>  
> -int tevent_req_profile_print(const struct tevent_req_profile *profile,
> -			     FILE *fp,
> -			     unsigned indent,
> -			     unsigned max_indent);
> -char *tevent_req_profile_string(const struct tevent_req_profile *profile,
> -				TALLOC_CTX *mem_ctx,
> +char *tevent_req_profile_string(TALLOC_CTX *mem_ctx,
> +				const struct tevent_req_profile *profile,
>  				unsigned indent,
>  				unsigned max_indent);
>  ssize_t tevent_req_profile_pack(
> -- 
> 2.11.0
> 
> 
> From bd8d33097871fb512267053359d4bf9b2e36b346 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Fri, 5 Oct 2018 12:12:39 +0200
> Subject: [PATCH 2/4] pdb: Use "sid_compose" where appropriate
> 
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
>  source3/auth/token_util.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/source3/auth/token_util.c b/source3/auth/token_util.c
> index f5b0e694433..c95d54db671 100644
> --- a/source3/auth/token_util.c
> +++ b/source3/auth/token_util.c
> @@ -220,8 +220,7 @@ static NTSTATUS add_builtin_guests(struct security_token *token,
>  	/*
>  	 * First check the local GUEST account.
>  	 */
> -	sid_copy(&tmp_sid, get_global_sam_sid());
> -	sid_append_rid(&tmp_sid, DOMAIN_RID_GUEST);
> +	sid_compose(&tmp_sid, get_global_sam_sid(), DOMAIN_RID_GUEST);
>  
>  	if (nt_token_check_sid(&tmp_sid, token)) {
>  		status = add_sid_to_array_unique(token,
> @@ -237,8 +236,7 @@ static NTSTATUS add_builtin_guests(struct security_token *token,
>  	/*
>  	 * First check the local GUESTS group.
>  	 */
> -	sid_copy(&tmp_sid, get_global_sam_sid());
> -	sid_append_rid(&tmp_sid, DOMAIN_RID_GUESTS);
> +	sid_compose(&tmp_sid, get_global_sam_sid(), DOMAIN_RID_GUESTS);
>  
>  	if (nt_token_check_sid(&tmp_sid, token)) {
>  		status = add_sid_to_array_unique(token,
> -- 
> 2.11.0
> 
> 
> From f4c2b8336d2686050f9f1ccb614dcd667e6dd009 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Fri, 5 Oct 2018 11:34:41 +0200
> Subject: [PATCH 3/4] pdb: Fix some "(ret == true)" to just "(ret)"
> 
> "ret" is a boolean, so this should not change semantics
> 
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
>  source3/passdb/pdb_interface.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/source3/passdb/pdb_interface.c b/source3/passdb/pdb_interface.c
> index 8b23da6b979..315da52f01b 100644
> --- a/source3/passdb/pdb_interface.c
> +++ b/source3/passdb/pdb_interface.c
> @@ -1238,7 +1238,7 @@ bool pdb_sid_to_id(const struct dom_sid *sid, struct unixid *id)
>  
>  	ret = pdb->sid_to_id(pdb, sid, id);
>  
> -	if (ret == true) {
> +	if (ret) {
>  		idmap_cache_set_sid2unixid(sid, id);
>  	}
>  
> @@ -1540,7 +1540,7 @@ static bool pdb_default_sid_to_id(struct pdb_methods *methods,
>  	 * "Unix User" and "Unix Group"
>  	 */
>  	ret = pdb_sid_to_id_unix_users_and_groups(sid, id);
> -	if (ret == true) {
> +	if (ret) {
>  		goto done;
>  	}
>  
> -- 
> 2.11.0
> 
> 
> From 518060a12547b5c84b99d76470e93aff44262860 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Fri, 5 Oct 2018 14:49:17 +0200
> Subject: [PATCH 4/4] pdb: Reduce code duplication in make_user_info()
> 
> 10 lines less and a few hundred (-O0) bytes .text less
> 
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
>  source3/auth/user_info.c | 42 ++++++++++++++++--------------------------
>  1 file changed, 16 insertions(+), 26 deletions(-)
> 
> diff --git a/source3/auth/user_info.c b/source3/auth/user_info.c
> index c79cc0c4f35..81192a51c61 100644
> --- a/source3/auth/user_info.c
> +++ b/source3/auth/user_info.c
> @@ -73,53 +73,45 @@ NTSTATUS make_user_info(TALLOC_CTX *mem_ctx,
>  
>  	user_info->client.account_name = talloc_strdup(user_info, smb_name);
>  	if (user_info->client.account_name == NULL) {
> -		TALLOC_FREE(user_info);
> -		return NT_STATUS_NO_MEMORY;
> +		goto nomem;
>  	}
>  
>  	user_info->mapped.account_name = talloc_strdup(user_info, internal_username);
>  	if (user_info->mapped.account_name == NULL) {
> -		TALLOC_FREE(user_info);
> -		return NT_STATUS_NO_MEMORY;
> +		goto nomem;
>  	}
>  
>  	user_info->mapped.domain_name = talloc_strdup(user_info, domain);
>  	if (user_info->mapped.domain_name == NULL) {
> -		TALLOC_FREE(user_info);
> -		return NT_STATUS_NO_MEMORY;
> +		goto nomem;
>  	}
>  
>  	user_info->client.domain_name = talloc_strdup(user_info, client_domain);
>  	if (user_info->client.domain_name == NULL) {
> -		TALLOC_FREE(user_info);
> -		return NT_STATUS_NO_MEMORY;
> +		goto nomem;
>  	}
>  
>  	user_info->workstation_name = talloc_strdup(user_info, workstation_name);
>  	if (user_info->workstation_name == NULL) {
> -		TALLOC_FREE(user_info);
> -		return NT_STATUS_NO_MEMORY;
> +		goto nomem;
>  	}
>  
>  	user_info->remote_host = tsocket_address_copy(remote_address, user_info);
>  	if (user_info->remote_host == NULL) {
> -		TALLOC_FREE(user_info);
> -		return NT_STATUS_NO_MEMORY;
> +		goto nomem;
>  	}
>  
>  	if (local_address != NULL) {
>  		user_info->local_host = tsocket_address_copy(local_address,
>  							     user_info);
>  		if (user_info->local_host == NULL) {
> -			TALLOC_FREE(user_info);
> -			return NT_STATUS_NO_MEMORY;
> +			goto nomem;
>  		}
>  	}
>  
>  	user_info->service_description = talloc_strdup(user_info, service_description);
>  	if (user_info->service_description == NULL) {
> -		TALLOC_FREE(user_info);
> -		return NT_STATUS_NO_MEMORY;
> +		goto nomem;
>  	}
>  
>  	DEBUG(5,("making blobs for %s's user_info struct\n", internal_username));
> @@ -127,22 +119,19 @@ NTSTATUS make_user_info(TALLOC_CTX *mem_ctx,
>  	if (lm_pwd && lm_pwd->data) {
>  		user_info->password.response.lanman = data_blob_talloc(user_info, lm_pwd->data, lm_pwd->length);
>  		if (user_info->password.response.lanman.data == NULL) {
> -			TALLOC_FREE(user_info);
> -			return NT_STATUS_NO_MEMORY;
> +			goto nomem;
>  		}
>  	}
>  	if (nt_pwd && nt_pwd->data) {
>  		user_info->password.response.nt = data_blob_talloc(user_info, nt_pwd->data, nt_pwd->length);
>  		if (user_info->password.response.nt.data == NULL) {
> -			TALLOC_FREE(user_info);
> -			return NT_STATUS_NO_MEMORY;
> +			goto nomem;
>  		}
>  	}
>  	if (lm_interactive_pwd) {
>  		user_info->password.hash.lanman = talloc(user_info, struct samr_Password);
>  		if (user_info->password.hash.lanman == NULL) {
> -			TALLOC_FREE(user_info);
> -			return NT_STATUS_NO_MEMORY;
> +			goto nomem;
>  		}
>  		memcpy(user_info->password.hash.lanman->hash, lm_interactive_pwd->hash,
>  		       sizeof(user_info->password.hash.lanman->hash));
> @@ -152,8 +141,7 @@ NTSTATUS make_user_info(TALLOC_CTX *mem_ctx,
>  	if (nt_interactive_pwd) {
>  		user_info->password.hash.nt = talloc(user_info, struct samr_Password);
>  		if (user_info->password.hash.nt == NULL) {
> -			TALLOC_FREE(user_info);
> -			return NT_STATUS_NO_MEMORY;
> +			goto nomem;
>  		}
>  		memcpy(user_info->password.hash.nt->hash, nt_interactive_pwd->hash,
>  		       sizeof(user_info->password.hash.nt->hash));
> @@ -163,8 +151,7 @@ NTSTATUS make_user_info(TALLOC_CTX *mem_ctx,
>  	if (plaintext_password) {
>  		user_info->password.plaintext = talloc_strdup(user_info, plaintext_password);
>  		if (user_info->password.plaintext == NULL) {
> -			TALLOC_FREE(user_info);
> -			return NT_STATUS_NO_MEMORY;
> +			goto nomem;
>  		}
>  		talloc_set_destructor(user_info->password.plaintext, clear_string);
>  	}
> @@ -176,4 +163,7 @@ NTSTATUS make_user_info(TALLOC_CTX *mem_ctx,
>  	DEBUG(10,("made a user_info for %s (%s)\n", internal_username, smb_name));
>  	*ret_user_info = user_info;
>  	return NT_STATUS_OK;
> +nomem:
> +	TALLOC_FREE(user_info);
> +	return NT_STATUS_NO_MEMORY;
>  }
> -- 
> 2.11.0
> 




More information about the samba-technical mailing list