[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