net-ads-search crashes when tokengroups attribute is requested

[Isaac Boukris]

On Wed, Jan 24, 2018 at 10:08 AM, Volker Lendecke
<Volker.Lendecke at sernet.de> 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 winbind mostly use ads_cached_connection()  over ads_init(),
so is_mine would be false and ads_destroy won't free ads.
Also note that we do not free ads when returning other errors in this
function (as we didn't allocate it), like here:
https://buildfarm.opencsw.org/source/xref/samba/source3/libads/ldap_utils.c#60