[PATCH] Remove fstring from wb_acct_info

Andrew Bartlett abartlet at samba.org
Wed Oct 31 22:25:20 UTC 2018


On Wed, 2018-10-31 at 15:19 -0700, Jeremy Allison wrote:
> On Thu, Nov 01, 2018 at 11:15:12AM +1300, Andrew Bartlett via samba-technical wrote:
> > On Wed, 2018-10-31 at 17:45 +0100, Samuel Cabrero via samba-technical
> > wrote:
> > > Hi,
> > > 
> > > the attached patch removes two fstrings from wb_acct_info struct. The
> > > reason for this change is because the winbindd group enumeration
> > > backend functions (ADS in particular) try to allocate an array of
> > > wb_acct_info as long as the number of groups in the domain, which may
> > > result in a huge chunk of memory for domains with a large number of
> > > groups.
> > > 
> > > Branch:
> > > https://gitlab.com/samuelcabrero/samba/commits/winbind_enum_grp_nomem
> > > 
> > > CI:
> > > https://gitlab.com/samuelcabrero/samba/pipelines/34956873
> > > 
> > > 
> > > Please review and push if you agree.
> > 
> > I'm worried about memory lifetimes. 
> > 
> > Could you change the talloc_strdup() and normal assignments into
> > talloc_steal() onto the head of the array at the memory context?
> 
> Actually I think the proposed code in the patch is good (I'm also reviewing
> right now).
> 
> The talloc_strdup's are tallocing onto the (*info) struct,
> which itself is talloc'ed off the mem_ctx. So they stay
> around as long as the info struct does (they're owned by
> it).

Sure, these I'm just suggesting could be more efficient (which I wasn't
clear about). 

> > This would ensure the strings stay around as long as the structure
> > pointing to them.
> 
> I think his patch already does that.
> 
> My only concern is that it changes the semantics so that
> acct_desc can be NULL, whereas previously it was always
> "" if not present.
> 
> Having said that I can't see any place were acct_desc
> is used by the caller so I think it's good.

However, my question is more: what is the lifetime for the output of
centry_string() and ads_pull_username()?

I think the centry_string() should be duplicated with tallco_strdup()
(I dare not suggest talloc_reference) and ads_pull_username() brought
over with talloc_steal().

Thanks,

Andrew Bartlett
-- 
Andrew Bartlett
https://samba.org/~abartlet/
Authentication Developer, Samba Team         https://samba.org
Samba Development and Support, Catalyst IT   
https://catalyst.net.nz/services/samba







More information about the samba-technical mailing list