From 4a84f8e18ea5604ac7e69849dee492718fd96296 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina@redhat.com>
Date: Wed, 26 Aug 2020 12:51:49 +0200
Subject: [PATCH] dp: fix potential race condition in provider's sbus server

We can hit a segfault if provider start is somehow delayed.

- dp_init_send
  - sbus_server_create_and_connect_send
    - sbus_server_create (*)
- dp_init_done (callback for sbus_server_create_and_connect_send)
  - sbus_server_create_and_connect_recv
  - sbus_server_set_on_connection (sets clients data and creates dp_cli)

At (*) sbus server is already created and accepts new connections once
we get into tevent loop. So it is possible that the client connects to
server before sbus_server_set_on_connection is called and thus the client
is not properly initialized. However it should not happen in normal start
because providers are started before responders and it can happen only if
data provider startup is somehow delay.

You can use this diff to reproduce the crash:
```diff

Reviewed-by: Alexey Tikhonov <atikhono@redhat.com>
---
 src/monitor/monitor.c                         |  3 ++-
 src/providers/data_provider/dp.c              |  9 +++-----
 src/providers/data_provider/dp_client.c       | 21 +++++++++++++++----
 src/providers/data_provider_be.c              |  2 ++
 src/sbus/connection/sbus_connection_connect.c |  7 +++++--
 src/sbus/sbus.h                               | 15 +++++++++++--
 src/sbus/sbus_private.h                       |  3 ---
 src/sbus/server/sbus_server.c                 |  9 +++++++-
 8 files changed, 50 insertions(+), 19 deletions(-)

diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
index 1e94a8f36..d9da05a51 100644
--- a/src/monitor/monitor.c
+++ b/src/monitor/monitor.c
@@ -2008,7 +2008,8 @@ static int monitor_process_init(struct mt_ctx *ctx,
 
     req = sbus_server_create_and_connect_send(ctx, ctx->ev, SSS_BUS_MONITOR,
                                               NULL, SSS_MONITOR_ADDRESS,
-                                              false, 100, ctx->uid, ctx->gid);
+                                              false, 100, ctx->uid, ctx->gid,
+                                              NULL, NULL);
     if (req == NULL) {
         ret = ENOMEM;
         goto done;
diff --git a/src/providers/data_provider/dp.c b/src/providers/data_provider/dp.c
index ba1cfec8a..0858c43d2 100644
--- a/src/providers/data_provider/dp.c
+++ b/src/providers/data_provider/dp.c
@@ -192,9 +192,9 @@ dp_init_send(TALLOC_CTX *mem_ctx,
     talloc_set_destructor(state->provider, dp_destructor);
 
     subreq = sbus_server_create_and_connect_send(state->provider, ev,
-                                                 state->sbus_name,
-                                                 NULL, sbus_address, true, 1000,
-                                                 uid, gid);
+        state->sbus_name, NULL, sbus_address, true, 1000, uid, gid,
+        (sbus_server_on_connection_cb)dp_client_init,
+        (sbus_server_on_connection_data)state->provider);
     if (subreq == NULL) {
         DEBUG(SSSDBG_CRIT_FAILURE, "Unable to create subrequest!\n");
         ret = ENOMEM;
@@ -235,9 +235,6 @@ static void dp_init_done(struct tevent_req *subreq)
         return;
     }
 
-    sbus_server_set_on_connection(state->provider->sbus_server,
-                                  dp_client_init, state->provider);
-
     /* be_ctx->provider must be accessible from modules and targets */
     state->be_ctx->provider = talloc_steal(state->be_ctx, state->provider);
 
diff --git a/src/providers/data_provider/dp_client.c b/src/providers/data_provider/dp_client.c
index 01baf01db..dcf939e57 100644
--- a/src/providers/data_provider/dp_client.c
+++ b/src/providers/data_provider/dp_client.c
@@ -140,15 +140,28 @@ dp_client_handshake_timeout(struct tevent_context *ev,
 {
     struct sbus_connection *conn;
     struct dp_client *dp_cli;
-
-    DEBUG(SSSDBG_OP_FAILURE,
-          "Client timed out before identification [%p]!\n", te);
+    const char *be_name;
+    const char *name;
 
     dp_cli = talloc_get_type(ptr, struct dp_client);
+    conn = dp_cli->conn;
+    be_name = dp_cli->provider->be_ctx->sbus_name;
 
     talloc_set_destructor(dp_cli, NULL);
 
-    conn = dp_cli->conn;
+    name = sbus_connection_get_name(dp_cli->conn);
+    if (name != NULL && strcmp(name, be_name) == 0) {
+        /* This is the data provider connection. Just free the client record
+         * but keep the connection opened. */
+        talloc_zfree(dp_cli);
+        return;
+    }
+
+    DEBUG(SSSDBG_OP_FAILURE,
+          "Client [%s] timed out before identification [%p]!\n",
+          name == NULL ? "unknown" : name, te);
+
+    /* Kill the connection. */
     talloc_zfree(dp_cli);
     talloc_zfree(conn);
 }
diff --git a/src/providers/data_provider_be.c b/src/providers/data_provider_be.c
index 95665332a..ca2d51650 100644
--- a/src/providers/data_provider_be.c
+++ b/src/providers/data_provider_be.c
@@ -665,6 +665,8 @@ int main(int argc, const char *argv[])
     uid_t uid;
     gid_t gid;
 
+    sleep(5);
+
     struct poptOption long_options[] = {
         POPT_AUTOHELP
         SSSD_MAIN_OPTS
diff --git a/src/sbus/connection/sbus_connection_connect.c b/src/sbus/connection/sbus_connection_connect.c
index 3f8702f0b..9cfe86206 100644
--- a/src/sbus/connection/sbus_connection_connect.c
+++ b/src/sbus/connection/sbus_connection_connect.c
@@ -344,7 +344,9 @@ sbus_server_create_and_connect_send(TALLOC_CTX *mem_ctx,
                                     bool use_symlink,
                                     uint32_t max_connections,
                                     uid_t uid,
-                                    gid_t gid)
+                                    gid_t gid,
+                                    sbus_server_on_connection_cb on_conn_cb,
+                                    sbus_server_on_connection_data on_conn_data)
 {
     struct sbus_server_create_and_connect_state *state;
     struct tevent_req *subreq;
@@ -358,7 +360,8 @@ sbus_server_create_and_connect_send(TALLOC_CTX *mem_ctx,
     }
 
     state->server = sbus_server_create(state, ev, address, use_symlink,
-                                       max_connections, uid, gid);
+                                       max_connections, uid, gid,
+                                       on_conn_cb, on_conn_data);
     if (state->server == NULL) {
         ret = ENOMEM;
         goto done;
diff --git a/src/sbus/sbus.h b/src/sbus/sbus.h
index 9136c4e4a..0983879f0 100644
--- a/src/sbus/sbus.h
+++ b/src/sbus/sbus.h
@@ -138,6 +138,8 @@ errno_t sbus_connect_private_recv(TALLOC_CTX *mem_ctx,
  * @param use_symlink            If a symlink to @address should be created.
  * @param uid                    Socket owner uid.
  * @param gid                    Socket owner gid.
+ * @param on_conn_cb             On new connection callback function.
+ * @param on_conn_data           Private data passed to the callback.
  *
  * @return New sbus server or NULL on error.
  */
@@ -148,7 +150,9 @@ sbus_server_create(TALLOC_CTX *mem_ctx,
                    bool use_symlink,
                    uint32_t max_connections,
                    uid_t uid,
-                   gid_t gid);
+                   gid_t gid,
+                   sbus_server_on_connection_cb on_conn_cb,
+                   sbus_server_on_connection_data on_conn_data);
 
 /**
  * Create a new sbus server at socket address @address and connect to it.
@@ -162,6 +166,8 @@ sbus_server_create(TALLOC_CTX *mem_ctx,
  * @param use_symlink            If a symlink to @address should be created.
  * @param uid                    Socket owner uid.
  * @param gid                    Socket owner gid.
+ * @param on_conn_cb             On new connection callback function.
+ * @param on_conn_data           Private data passed to the callback.
  *
  * @return Tevent request or NULL on error.
  */
@@ -174,7 +180,9 @@ sbus_server_create_and_connect_send(TALLOC_CTX *mem_ctx,
                                     bool use_symlink,
                                     uint32_t max_connections,
                                     uid_t uid,
-                                    gid_t gid);
+                                    gid_t gid,
+                                    sbus_server_on_connection_cb on_conn_cb,
+                                    sbus_server_on_connection_data on_conn_data);
 
 /**
  * Receive reply from @sbus_server_create_and_connect_send.
@@ -446,4 +454,7 @@ errno_t
 sbus_router_add_node_map(struct sbus_connection *conn,
                          struct sbus_node *map);
 
+/* Get connection name, well known name is preferred. */
+const char * sbus_connection_get_name(struct sbus_connection *conn);
+
 #endif /* _SBUS_H_ */
diff --git a/src/sbus/sbus_private.h b/src/sbus/sbus_private.h
index dbea7322d..eef397b86 100644
--- a/src/sbus/sbus_private.h
+++ b/src/sbus/sbus_private.h
@@ -190,9 +190,6 @@ void sbus_connection_tevent_disable(struct sbus_connection *conn);
 /* Mark that this connection is currently active (new method call arrived). */
 void sbus_connection_mark_active(struct sbus_connection *conn);
 
-/* Get connection name, well known name is preferred. */
-const char * sbus_connection_get_name(struct sbus_connection *conn);
-
 /* Set connection well known name. */
 errno_t sbus_connection_set_name(struct sbus_connection *conn,
                                  const char *name);
diff --git a/src/sbus/server/sbus_server.c b/src/sbus/server/sbus_server.c
index 2b9327051..69efd739b 100644
--- a/src/sbus/server/sbus_server.c
+++ b/src/sbus/server/sbus_server.c
@@ -635,7 +635,9 @@ sbus_server_create(TALLOC_CTX *mem_ctx,
                    bool use_symlink,
                    uint32_t max_connections,
                    uid_t uid,
-                   gid_t gid)
+                   gid_t gid,
+                   sbus_server_on_connection_cb on_conn_cb,
+                   sbus_server_on_connection_data on_conn_data)
 {
     DBusServer *dbus_server;
     struct sbus_server *sbus_server;
@@ -675,6 +677,11 @@ sbus_server_create(TALLOC_CTX *mem_ctx,
         goto done;
     }
 
+    if (on_conn_cb != NULL) {
+        _sbus_server_set_on_connection(sbus_server, "on-connection", on_conn_cb,
+                                       on_conn_data);
+    }
+
     sbus_server->names = sss_ptr_hash_create(sbus_server,
                              sbus_server_name_remove_from_table, sbus_server);
     if (sbus_server->names == NULL) {
-- 
2.32.0.windows.1

