svn commit: samba r20207 - in branches: SAMBA_3_0/source/nsswitch SAMBA_3_0_24/source/nsswitch

[simo]

On Sat, 2006-12-16 at 01:52 +0000, jra at samba.org wrote:
> Author: jra
> Date: 2006-12-16 01:52:06 +0000 (Sat, 16 Dec 2006)
> New Revision: 20207
> 
> WebSVN: http://websvn.samba.org/cgi-bin/viewcvs.cgi?view=rev&root=samba&rev=20207
> 
> Log:
> Fix a couple more places where extra_data was
> being talloc'ed off the NULL context instead
> of being malloced.
> Jeremy.

[..]

> Modified: branches/SAMBA_3_0_24/source/nsswitch/winbindd_group.c
> ===================================================================
> --- branches/SAMBA_3_0_24/source/nsswitch/winbindd_group.c	2006-12-16 01:32:57 UTC (rev 20206)
> +++ branches/SAMBA_3_0_24/source/nsswitch/winbindd_group.c	2006-12-16 01:52:06 UTC (rev 20207)
> @@ -1398,12 +1398,15 @@
>  		return WINBINDD_OK;
>  	}
>  
> -	if (!print_sidlist(NULL, groups, num_groups, &sidstring, &len)) {
> -		DEBUG(0, ("malloc failed\n"));
> +	if (!print_sidlist(state->mem_ctx, groups, num_groups, &sidstring, &len)) {
> +		DEBUG(0, ("talloc failed\n"));
>  		return WINBINDD_ERROR;
>  	}
>  
> -	state->response.extra_data.data = sidstring;
> +	state->response.extra_data.data = SMB_STRDUP(sidstring);
> +	if (!state->response.extra_data.data) {
> +		return WINBINDD_ERROR;
> +	}
>  	state->response.length += len+1;
>  	state->response.data.num_entries = num_groups;

Jeremy,
If I correctly read this patch we are leaking memory here and you should
free sidstring.

I think the way print_sidlist() and sprintf_append() are coded up is
very bad and misleading, I am sure it will be very easy to get them
wrong again in future.

I had to read the code to find out with big surprise that passing NULL
as mem_ctx does NOT in fact allocate on the NULL context but does a
malloc, and so based on what you pass as mem_ctx you have to carefully
decide whether to use talloc_free() or just a free().

I would either separate these functions in a malloc and a talloc
version, or rather convert all paths to just use the talloc one and let
it alloc on the NULL context when you don't want/need to set up a
specific memory context. This way you can consistently always use
talloc_free() in functions calling print_sidlist() without risking
errors.

Simo.

-- 
Simo Sorce
Samba Team GPL Compliance Officer
email: idra at samba.org
http://samba.org