net-ads-search crashes when tokengroups attribute is requested

[Jeremy Allison]

On Wed, Jan 24, 2018 at 08:34:51PM +0100, Volker Lendecke wrote:
> On Wed, Jan 24, 2018 at 08:44:39AM -0800, Jeremy Allison wrote:
> > 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 :-).
> 
> Right. The whole API around ads_struct is pretty much broken from my
> point of view. I've tried to tackle that in the past a few times, but
> we have enough code using that API that a one-shot approach just won't
> work. We need to replace that gradually.

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 ?

Thanks,

	Jeremy.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-s3-ldap-Ensure-the-ADS_STRUCT-pointer-doesn-t-get-fr.patch
Type: text/x-diff
Size: 1217 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20180124/b8f5adf7/0001-s3-ldap-Ensure-the-ADS_STRUCT-pointer-doesn-t-get-fr.diff>