net-ads-search crashes when tokengroups attribute is requested

[Jeremy Allison]

On Wed, Jan 24, 2018 at 08:41:30AM -0800, Jeremy Allison via samba-technical wrote:
> On Wed, Jan 24, 2018 at 09:08:28AM +0100, Volker Lendecke wrote:
> > On Tue, Jan 23, 2018 at 04:48:22PM -0800, Jeremy Allison via samba-technical wrote:
> > > On Tue, Jan 23, 2018 at 08:16:31PM +0200, Isaac Boukris wrote:
> > > > On Tue, Jan 23, 2018 at 9:51 AM, Isaac Boukris <iboukris at gmail.com> wrote:
> > > > > On Tue, Jan 23, 2018 at 3:10 AM, Jeremy Allison <jra at samba.org> wrote:
> > > > >> Oh, here is the real problem. ads_do_search_retry_internal()
> > > > >> is destroying the ADS_STRUCT *ads struct on reconnection
> > > > >> error when it didn't open it.
> > > > >>
> > > > >> Here is an attached (untested) patch you could try. I'll
> > > > >> have to go through all the code paths to ensure that this
> > > > >> doesn't cause leaks in other areas though.
> > > > >
> > > > >
> > > > > Yes, this patch works ok, solves the crash and invalid memory access
> > > > > (I had it initially, but wasn't sure).
> > > > 
> > > > 
> > > > Here is valgrind memory check before the patch:
> > > > https://pastebin.com/DbZPjKWv
> > > > 
> > > > And after:
> > > > https://pastebin.com/5G72dmLa
> > > 
> > > Yeah the patch is good. There's no case where
> > > ads_do_search_retry_internal() should destroy
> > > the passed in ADS_STRUCT *ads struct.
> > > 
> > > I'll log a bug and get this fixed.
> > 
> > Sorry, but this requires a much more thorough rewrite of libads and in
> > particular winbind. Are we sure that this does not create memleaks in
> > long-running winbind when connections are torn down by the server?
> 
> I think this is what the 'is_mine' structure element in
> ADS_STRUCT is for (winbindd is the only code that uses
> it). I couldn't see any possible leaks, but I'll take
> a closer look.

Another point, this particular crash is only one of a whole
host of crashes that can occur in all callers of ads_do_search_rety(),
all of which assume that ADS_STRUCT *ads is valid on exit
from the function (i.e. they all assume that ads_do_search_rety()
*doesn't* stamp on their pointer).

If there is an issue, the fix should be in winbindd and
not the other callers. It is reasonable to assume that
calling a search function won't deallocate one of the
parameters it is passed (and doesn't own :-).

Jeremy.