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

[simo]

On Fri, 2006-12-15 at 20:51 -0800, Jeremy Allison wrote:
> 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 ?

opps sorry I read the NULL backwards indeed.

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

yeah sprintf_append is very bad.

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

No I don't need that function in IDMAP code.

Simo.

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