[PATCH] Use a stackframe for memory management in _wbint_QueryGroupList

[Jeremy Allison]

On Thu, Dec 07, 2017 at 07:04:42PM +0100, Andreas Schneider wrote:
> 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.

LGTM. Pushed !





> -- 
> Andreas Schneider                   GPG-ID: CC014E3D
> Samba Team                             asn at samba.org
> www.samba.org

> 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
>