[PATCH] Use a stackframe for memory management in _wbint_QueryGroupList

[Andreas Schneider]

On Thursday, 7 December 2017 18:57:34 CET Jeremy Allison wrote:
> On Thu, Dec 07, 2017 at 08:29:03AM +0100, Andreas Schneider via samba-
technical wrote:
> > Hi,
> > 
> > I'm fixing that function right now for 3.6 and saw that some TALLOC_FREE
> > are missing. For master I decided to use a stackframe to make it simpler
> > to clean up.
> > 
> > Please review and push if OK.
> 
> Is the talloc_steal() really needed ?

Ups, during fixing that I forgot to change this back to frame. So 
talloc_array() should allocate on the frame that the array is freed on error 
and talloc_move should move it in the end ...

> We already have:
> 
>   	result = talloc_array(r->out.groups, struct wbint_Principal,
>   			      num_total);
> 
> (NB. result is allocated off r->out.groups) so doing:
> 
>  +	r->out.groups->principals = talloc_steal(r->out.groups, result);
> 
> is at best redundant. How does talloc_steal() react when
> the target is the same as the parent ?
> 
> Also, I prefer talloc_move() (yeah I know, I invented it) as it
> leaves the old pointer as NULL for safety :-).

Ok, I used talloc_move() now.
 
> Also, if you're fixing any real memory leaks here we probably need
> a bug report for back-ports.

Not really, we allocate on talloc_tos() it probably doesn't live long enough, 
but some error path directly free it and others don't. This just makes sure 
every error path directly cleans up. I don't think we need a bug.

Updated patch attached.


Thanks,


	Andreas

-- 
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             asn at samba.org
www.samba.org
-------------- next part --------------
>From d6bbf9ff01ff7a57935264e7676ce177f5937223 Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Wed, 6 Dec 2017 18:48:47 +0100
Subject: [PATCH] s3:winbindd: Use a stackframe for memory management in
 _wbint_QueryGroupList

Signed-off-by: Andreas Schneider <asn at samba.org>
---
 source3/winbindd/winbindd_dual_srv.c | 43 ++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 21 deletions(-)

diff --git a/source3/winbindd/winbindd_dual_srv.c b/source3/winbindd/winbindd_dual_srv.c
index 797a9d95493..81fcf855847 100644
--- a/source3/winbindd/winbindd_dual_srv.c
+++ b/source3/winbindd/winbindd_dual_srv.c
@@ -380,6 +380,7 @@ NTSTATUS _wbint_LookupGroupMembers(struct pipes_struct *p,
 NTSTATUS _wbint_QueryGroupList(struct pipes_struct *p,
 			       struct wbint_QueryGroupList *r)
 {
+	TALLOC_CTX *frame = NULL;
 	struct winbindd_domain *domain = wb_child_domain();
 	uint32_t i;
 	uint32_t num_local_groups = 0;
@@ -389,13 +390,15 @@ NTSTATUS _wbint_QueryGroupList(struct pipes_struct *p,
 	uint32_t ti = 0;
 	uint64_t num_total = 0;
 	struct wbint_Principal *result;
-	NTSTATUS status;
+	NTSTATUS status = NT_STATUS_UNSUCCESSFUL;
 	bool include_local_groups = false;
 
 	if (domain == NULL) {
 		return NT_STATUS_REQUEST_NOT_ACCEPTED;
 	}
 
+	frame = talloc_stackframe();
+
 	switch (lp_server_role()) {
 	case ROLE_ACTIVE_DIRECTORY_DC:
 		if (domain->internal) {
@@ -416,32 +419,33 @@ NTSTATUS _wbint_QueryGroupList(struct pipes_struct *p,
 	}
 
 	if (include_local_groups) {
-		status = wb_cache_enum_local_groups(domain, talloc_tos(),
+		status = wb_cache_enum_local_groups(domain, frame,
 						    &num_local_groups,
 						    &local_groups);
 		reset_cm_connection_on_error(domain, status);
 		if (!NT_STATUS_IS_OK(status)) {
-			return status;
+			goto out;
 		}
 	}
 
-	status = wb_cache_enum_dom_groups(domain, talloc_tos(),
+	status = wb_cache_enum_dom_groups(domain, frame,
 					  &num_dom_groups,
 					  &dom_groups);
 	reset_cm_connection_on_error(domain, status);
 	if (!NT_STATUS_IS_OK(status)) {
-		return status;
+		goto out;
 	}
 
 	num_total = num_local_groups + num_dom_groups;
 	if (num_total > UINT32_MAX) {
-		return NT_STATUS_INTERNAL_ERROR;
+		status = NT_STATUS_INTERNAL_ERROR;
+		goto out;
 	}
 
-	result = talloc_array(r->out.groups, struct wbint_Principal,
-			      num_total);
+	result = talloc_array(frame, struct wbint_Principal, num_total);
 	if (result == NULL) {
-		return NT_STATUS_NO_MEMORY;
+		status = NT_STATUS_NO_MEMORY;
+		goto out;
 	}
 
 	for (i = 0; i < num_local_groups; i++) {
@@ -452,14 +456,11 @@ NTSTATUS _wbint_QueryGroupList(struct pipes_struct *p,
 		rg->type = SID_NAME_ALIAS;
 		rg->name = talloc_strdup(result, lg->acct_name);
 		if (rg->name == NULL) {
-			TALLOC_FREE(result);
-			TALLOC_FREE(dom_groups);
-			TALLOC_FREE(local_groups);
-			return NT_STATUS_NO_MEMORY;
+			status = NT_STATUS_NO_MEMORY;
+			goto out;
 		}
 	}
 	num_local_groups = 0;
-	TALLOC_FREE(local_groups);
 
 	for (i = 0; i < num_dom_groups; i++) {
 		struct wb_acct_info *dg = &dom_groups[i];
@@ -469,19 +470,19 @@ NTSTATUS _wbint_QueryGroupList(struct pipes_struct *p,
 		rg->type = SID_NAME_DOM_GRP;
 		rg->name = talloc_strdup(result, dg->acct_name);
 		if (rg->name == NULL) {
-			TALLOC_FREE(result);
-			TALLOC_FREE(dom_groups);
-			TALLOC_FREE(local_groups);
-			return NT_STATUS_NO_MEMORY;
+			status = NT_STATUS_NO_MEMORY;
+			goto out;
 		}
 	}
 	num_dom_groups = 0;
-	TALLOC_FREE(dom_groups);
 
 	r->out.groups->num_principals = ti;
-	r->out.groups->principals = result;
+	r->out.groups->principals = talloc_move(r->out.groups, &result);
 
-	return NT_STATUS_OK;
+	status = NT_STATUS_OK;
+out:
+	TALLOC_FREE(frame);
+	return status;
 }
 
 NTSTATUS _wbint_QueryUserRidList(struct pipes_struct *p,
-- 
2.15.1