net-ads-search crashes when tokengroups attribute is requested

[Jeremy Allison]

On Thu, Jan 25, 2018 at 03:22:02PM +0200, Isaac Boukris wrote:
> On Thu, Jan 25, 2018 at 12:19 PM, Volker Lendecke
> <Volker.Lendecke at sernet.de> wrote:
> > On Wed, Jan 24, 2018 at 02:16:53PM -0800, Jeremy Allison wrote:
> >> OK, how about this fix instead ? It keeps the ads_destroy() call
> >> after the reconnection failure, but uses the horrific ads->is_mine
> >> flag to ensure the ADS_STRUCT isn't thrown away causing the
> >> valgrind failures and crashes.
> >>
> >> If any of the callers were indirecting into any of the
> >> ADS_STRUCT members after a ads_do_search_retry_internal()
> >> reconnect failure, they were already crashing (their
> >> ads pointer had already been freed). And if they were
> >> not, but only calling ads_destroy() on error (which
> >> is the case for all of the code I've seen), then this
> >> fix makes it safe for them to do so whilst still doing
> >> the internal structure memory free that winbindd may
> >> be depending on.
> >>
> >> I safe off the original value of the horrific ads->is_mine
> >> flag and restore it afterwards, so this should have no
> >> effect on the winbindd code that actually uses this.
> >>
> >> Isaac, can you test this fix to see if it also fixes
> >> the valgrind errors ?
> >
> > RB+. Lets wait the confirmation before pushing though.
> 
> Yes, it is fine, it solved the valgrind errors (same as removing ads_destroy).
> 
> I think the attached patch is a bit cleaner though.

Not quite. The ads_cleanup() call should take a
ADS_STRUCT *, not ADS_STRUCT **. There's no need
for it to have access to the containing pointer
as it never frees it.

I'm going to push my existing patch, you can
propose yours (when fixed:-) as a cleanup
on top.

I also thought of adding a 'cleanup' function
(I even thought of the same name :-) but didn't
as currently we only make this call in one place.

Thanks for confirming it fixes the issues !

Jeremy.