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

[Jeremy Allison]

On Fri, Dec 15, 2006 at 11:34:39PM -0500, simo wrote:
> 
> > 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 don't think so. I changed the call from print_sidlist from taking
a NULL context to taking the state->mem_ctx instead. I think it was
leaking memory before, but isn't now.

Are you reading the patch backwards ?

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

print_sidlist is ok, sprintf_append is bad. I've been removing the
idiom that mem_ctx == NULL means malloc, and mem_ctx != NUll means
talloc wherever I've found it. This is just another one.

I'll look at how hard it is to fix.

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

FYI: At least in SAMBA_3_0_24 there are no calls to print_sidlist
that take a null context anymore. Did you make changes to this in
your new idmap code ?

Jeremy.