net-ads-search crashes when tokengroups attribute is requested

[Isaac Boukris]

On Thu, Jan 25, 2018 at 3:22 PM, Isaac Boukris <iboukris at gmail.com> 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.


Or yet another alternative, since we are not going to zero it with
SAFE_FREE we don't need its address.
-------------- next part --------------
From 7f1049c3f5107bb197832f2da2416f959be32de6 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Wed, 24 Jan 2018 14:09:43 -0800
Subject: [PATCH] s3: ldap: Ensure the ADS_STRUCT pointer doesn't get freed on
 error, we don't own it here.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=13244

Signed-off-by: Jeremy Allison <jra at samba.org>
Signed-off-by: Isaac Boukris <iboukris at gmail.com>
---
 source3/libads/ads_proto.h  |  1 +
 source3/libads/ads_struct.c | 13 +++++++++++++
 source3/libads/ldap_utils.c |  2 +-
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/source3/libads/ads_proto.h b/source3/libads/ads_proto.h
index b6d9d9b..4daeb97 100644
--- a/source3/libads/ads_proto.h
+++ b/source3/libads/ads_proto.h
@@ -42,6 +42,7 @@ ADS_STRUCT *ads_init(const char *realm,
 		     const char *ldap_server);
 bool ads_set_sasl_wrap_flags(ADS_STRUCT *ads, int flags);
 void ads_destroy(ADS_STRUCT **ads);
+void ads_cleanup(ADS_STRUCT *ads);
 
 /* The following definitions come from libads/disp_sec.c  */
 
diff --git a/source3/libads/ads_struct.c b/source3/libads/ads_struct.c
index 0dc7b11..10916c2 100644
--- a/source3/libads/ads_struct.c
+++ b/source3/libads/ads_struct.c
@@ -211,3 +211,16 @@ void ads_destroy(ADS_STRUCT **ads)
 			SAFE_FREE(*ads);
 	}
 }
+
+void ads_cleanup(ADS_STRUCT *ads)
+{
+	bool was_mine;
+
+	if (!ads)
+		return;
+
+	was_mine = ads->is_mine;
+	ads->is_mine = false;
+	ads_destroy(&ads);
+	ads->is_mine = was_mine;
+}
diff --git a/source3/libads/ldap_utils.c b/source3/libads/ldap_utils.c
index a4adbc0..30f1e9d 100644
--- a/source3/libads/ldap_utils.c
+++ b/source3/libads/ldap_utils.c
@@ -107,7 +107,7 @@ static ADS_STATUS ads_do_search_retry_internal(ADS_STRUCT *ads, const char *bind
 		if (!ADS_ERR_OK(status)) {
 			DEBUG(1,("ads_search_retry: failed to reconnect (%s)\n",
 				 ads_errstr(status)));
-			ads_destroy(&ads);
+			ads_cleanup(ads);
 			SAFE_FREE(bp);
 			return status;
 		}
-- 
2.1.0