[PATCH] Minor cleanup to libnet_join_member

[Swen Schillig]

On Thu, 2018-01-25 at 10:34 -0800, Jeremy Allison wrote:
> On Thu, Jan 25, 2018 at 04:52:04PM +0100, Swen Schillig via samba-
> technical wrote:
> > On Thu, 2018-01-25 at 15:16 +0100, Volker Lendecke via samba-
> > technical
> > wrote:
> > > On Thu, Jan 25, 2018 at 03:03:56PM +0100, Swen Schillig via
> > > samba-
> > > technical wrote:
> > > > On Thu, 2018-01-25 at 14:49 +0100, Volker Lendecke via samba-
> > > > technical
> > > > wrote:
> > > > > On Thu, Jan 25, 2018 at 02:33:12PM +0100, Swen Schillig via
> > > > > samba-
> > > > > technical wrote:
> > > > > > 
> > > > > 
> > > > > Bike-shedding-alert: talloc_move() is higly preferred over
> > > > > talloc_steal() these days...
> > > > > 
> > > > > Volker
> > > > 
> > > > I was thinking about it as well, but I didn't see the point to
> > > > zero
> > > > the "old" variable just before destroying it anyway.
> > > 
> > > That's true. But TALLOC_FREE instead of talloc_free is overkill
> > > in
> > > many places too. It's just careful programming. We want to crash
> > > instead of use-after-free of pointers.
> > > 
> > > Volker
> > 
> > The data we're talking about is local, the processing is
> > synchronous
> > and the very next command is free'in the memory.
> > Since talloc_move is not atomic, it is not any bit more secure here
> > then talloc_steal.
> > 
> > Anyhow, if there's no other opinion 'til tomorrow, 
> > I'll update the code for you to talloc_move :-)
> 
> Yes, please use talloc_move. I sometime git grep for talloc_steal
> to look for memory errors (as the two are usually synonymous :-).
> 

Ok.

Here we go.


Cheers Swen
-------------- next part --------------
From b82aa466921c8d9c27e66d75db1806091996b0ca Mon Sep 17 00:00:00 2001
From: Swen Schillig <swen at vnet.ibm.com>
Date: Thu, 25 Jan 2018 11:18:50 +0100
Subject: [PATCH] Minor cleanup to libnet_join_member

Prevent code duplication by consolidating cleanup task
at the end of the function.

Signed-off-by: Swen Schillig <swen at vnet.ibm.com>
---
 source4/libnet/libnet_join.c | 46 ++++++++++++++++++--------------------------
 1 file changed, 19 insertions(+), 27 deletions(-)

diff --git a/source4/libnet/libnet_join.c b/source4/libnet/libnet_join.c
index 6cd18e02c9b..245485f69cd 100644
--- a/source4/libnet/libnet_join.c
+++ b/source4/libnet/libnet_join.c
@@ -940,11 +940,10 @@ NTSTATUS libnet_Join_member(struct libnet_context *ctx,
 
 	r2 = talloc_zero(tmp_mem, struct libnet_JoinDomain);
 	if (!r2) {
-		r->out.error_string = NULL;
-		talloc_free(tmp_mem);
-		return NT_STATUS_NO_MEMORY;
+		status = NT_STATUS_NO_MEMORY;
+		goto out;
 	}
-	
+
 	acct_type = ACB_WSTRUST;
 
 	if (r->in.netbios_name != NULL) {
@@ -952,19 +951,17 @@ NTSTATUS libnet_Join_member(struct libnet_context *ctx,
 	} else {
 		netbios_name = talloc_strdup(tmp_mem, lpcfg_netbios_name(ctx->lp_ctx));
 		if (!netbios_name) {
-			r->out.error_string = NULL;
-			talloc_free(tmp_mem);
-			return NT_STATUS_NO_MEMORY;
+			status = NT_STATUS_NO_MEMORY;
+			goto out;
 		}
 	}
 
 	account_name = talloc_asprintf(tmp_mem, "%s$", netbios_name);
 	if (!account_name) {
-		r->out.error_string = NULL;
-		talloc_free(tmp_mem);
-		return NT_STATUS_NO_MEMORY;
+		status = NT_STATUS_NO_MEMORY;
+		goto out;
 	}
-	
+
 	/*
 	 * join the domain
 	 */
@@ -978,16 +975,14 @@ NTSTATUS libnet_Join_member(struct libnet_context *ctx,
 	status = libnet_JoinDomain(ctx, r2, r2);
 	if (!NT_STATUS_IS_OK(status)) {
 		r->out.error_string = talloc_steal(mem_ctx, r2->out.error_string);
-		talloc_free(tmp_mem);
-		return status;
+		goto out;
 	}
 
 	set_secrets = talloc_zero(tmp_mem,
 				  struct provision_store_self_join_settings);
 	if (!set_secrets) {
-		r->out.error_string = NULL;
-		talloc_free(tmp_mem);
-		return NT_STATUS_NO_MEMORY;
+		status = NT_STATUS_NO_MEMORY;
+		goto out;
 	}
 
 	set_secrets->domain_name = r2->out.domain_name;
@@ -997,7 +992,7 @@ NTSTATUS libnet_Join_member(struct libnet_context *ctx,
 	set_secrets->machine_password = r2->out.join_password;
 	set_secrets->key_version_number = r2->out.kvno;
 	set_secrets->domain_sid = r2->out.domain_sid;
-	
+
 	status = provision_store_self_join(ctx, ctx->lp_ctx, ctx->event_ctx, set_secrets, &error_string);
 	if (!NT_STATUS_IS_OK(status)) {
 		if (error_string) {
@@ -1008,19 +1003,16 @@ NTSTATUS libnet_Join_member(struct libnet_context *ctx,
 						  "provision_store_self_join failed with %s",
 						  nt_errstr(status));
 		}
-		talloc_free(tmp_mem);
-		return status;
+		goto out;
 	}
 
 	/* move all out parameter to the callers TALLOC_CTX */
-	r->out.error_string	= NULL;
-	r->out.join_password	= r2->out.join_password;
-	talloc_reparent(r2, mem_ctx, r2->out.join_password);
-	r->out.domain_sid	= r2->out.domain_sid;
-	talloc_reparent(r2, mem_ctx, r2->out.domain_sid);
-	r->out.domain_name      = r2->out.domain_name;
-	talloc_reparent(r2, mem_ctx, r2->out.domain_name);
+	r->out.join_password	= talloc_move(mem_ctx, &r2->out.join_password);
+	r->out.domain_sid	= talloc_move(mem_ctx, &r2->out.domain_sid);
+	r->out.domain_name      = talloc_move(mem_ctx, &r2->out.domain_name);
+	status = NT_STATUS_OK;
+out:
 	talloc_free(tmp_mem);
-	return NT_STATUS_OK;
+	return status;
 }
 
-- 
2.14.3