From: Ralph Boehme <slow@samba.org>
Date: Fri, 19 Nov 2021 12:29:54 +0000 (+0100)
Subject: mdssvc: prevent a crash when pending search finishes after the client closed the... 
X-Git-Tag: samba-4.17.0rc1~120
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=9b56c7030f86f24a5b21f2a972a641afb556f7ab;p=thirdparty%2Fsamba.git

mdssvc: prevent a crash when pending search finishes after the client closed the search connection

When a search is in-flight and currently being processed against the
Elasticsearch server, we set s->pending. In the destructor of "s" we check "pending"
and reject deallocation of the object.

One instance where "s" is requested to be deallocated is when the client closes
the top-level per-share search connection. This will implicitly close all
searches associated with the mds_ctx from mds_ctx_destructor_cb():

	while (mds_ctx->query_list != NULL) {
		/*
		 * slq destructor removes element from list.
		 * Don't use TALLOC_FREE()!
		 */
		talloc_free(mds_ctx->query_list);
	}

So when this happens the Elasticsearch backend query object stays around,
alongside with any active tevent_req request and a tevent_req timer set with
tevent_req_set_endtime() in mds_es_search_send().

BUG: https://bugzilla.samba.org/show_bug.cgi?id=14915
RN: mdssvc crashes when searches are pending and the client closes the mdssvc IPC pipe

Signed-off-by: Ralph Boehme <slow@samba.org>
Reviewed-by: Noel Power <npower@samba.org>
---

diff --git a/source3/rpc_server/mdssvc/mdssvc_es.c b/source3/rpc_server/mdssvc/mdssvc_es.c
index 46f2917dbfe..b16426bc65a 100644
--- a/source3/rpc_server/mdssvc/mdssvc_es.c
+++ b/source3/rpc_server/mdssvc/mdssvc_es.c
@@ -115,6 +115,26 @@ static bool mds_es_next_search_trigger(struct mds_es_ctx *mds_es_ctx);
 static void mds_es_search_set_pending(struct sl_es_search *s);
 static void mds_es_search_unset_pending(struct sl_es_search *s);
 
+static int mds_es_ctx_destructor(struct mds_es_ctx *mds_es_ctx)
+{
+	struct sl_es_search *s = mds_es_ctx->searches;
+
+	/*
+	 * The per tree-connect state mds_es_ctx (a child of mds_ctx) is about
+	 * to go away and has already freed all waiting searches. If there's a
+	 * search remaining that's when the search is already active. Reset the
+	 * mds_es_ctx pointer, so we can detect this when the search completes.
+	 */
+
+	if (s == NULL) {
+		return 0;
+	}
+
+	s->mds_es_ctx = NULL;
+
+	return 0;
+}
+
 static bool mds_es_connect(struct mds_ctx *mds_ctx)
 {
 	struct mdssvc_es_ctx *mdssvc_es_ctx = talloc_get_type_abort(
@@ -132,6 +152,7 @@ static bool mds_es_connect(struct mds_ctx *mds_ctx)
 	};
 
 	mds_ctx->backend_private = mds_es_ctx;
+	talloc_set_destructor(mds_es_ctx, mds_es_ctx_destructor);
 
 	subreq = mds_es_connect_send(
 			mds_es_ctx,
@@ -349,6 +370,9 @@ static void mds_es_reconnect_on_error(struct sl_es_search *s)
 
 static int search_destructor(struct sl_es_search *s)
 {
+	if (s->mds_es_ctx == NULL) {
+		return 0;
+	}
 	DLIST_REMOVE(s->mds_es_ctx->searches, s);
 	return 0;
 }
@@ -444,6 +468,15 @@ static void mds_es_search_done(struct tevent_req *subreq)
 	DBG_DEBUG("Search done for search [%p]\n", s);
 
 	mds_es_search_unset_pending(s);
+
+	if (mds_es_ctx == NULL) {
+		/*
+		 * Search connection closed by the user while s was pending.
+		 */
+		TALLOC_FREE(s);
+		return;
+	}
+
 	DLIST_REMOVE(mds_es_ctx->searches, s);
 
 	ret = mds_es_search_recv(subreq);