[PATCH] Remove fstring from wb_acct_info

[Jeremy Allison]

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

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

Jeremy.