URL: https://github.com/SSSD/sssd/pull/5299
Author: pbrezina
Title: #5299: dp: fix potential race condition in provider's sbus server
Action: synchronized
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/5299/head:pr5299
git checkout pr5299
From 8d460852c3e19790d6b75c2983868c842ec2d186 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@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
--- a/src/providers/data_provider_be.c
+++ b/src/providers/data_provider_be.c
@@ -702,6 +702,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
```
Resolves:
https://github.com/SSSD/sssd/issues/5298
---
src/monitor/monitor.c | 3 ++-
src/providers/data_provider/dp.c | 9 +++------
src/sbus/connection/sbus_connection_connect.c | 7 +++++--
src/sbus/sbus.h | 12 ++++++++++--
src/sbus/server/sbus_server.c | 9 ++++++++-
5 files changed, 28 insertions(+), 12 deletions(-)
diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
index 1e94a8f36e..d9da05a511 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 ba1cfec8a8..0858c43d2a 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/sbus/connection/sbus_connection_connect.c b/src/sbus/connection/sbus_connection_connect.c
index 3f8702f0b7..9cfe86206b 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 9136c4e4ad..bfa7ecd579 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.
diff --git a/src/sbus/server/sbus_server.c b/src/sbus/server/sbus_server.c
index 2b93270516..69efd739b4 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_cb, sbus_server);
if (sbus_server->names == NULL) {
_______________________________________________
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
Fedora Code of Conduct:
https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives:
https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.org
