URL: https://github.com/SSSD/sssd/pull/646 Author: pbrezina Title: #646: proxy: access provider directly not through be_ctx Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/646/head:pr646 git checkout pr646
From 132f86363083b30c3ff5c36d3891d85cfa89670c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com> Date: Wed, 5 Sep 2018 13:35:54 +0200 Subject: [PATCH 1/5] proxy: access provider directly not through be_ctx Modules are initialized as part of dp_init_send() but be_ctx->provider is set only after this request is finished therefore it is not available here. Resolves: https://pagure.io/SSSD/sssd/issue/3812 --- src/providers/proxy/proxy_init.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/providers/proxy/proxy_init.c b/src/providers/proxy/proxy_init.c index cf4f82e12..98c6dd179 100644 --- a/src/providers/proxy/proxy_init.c +++ b/src/providers/proxy/proxy_init.c @@ -192,6 +192,7 @@ static errno_t proxy_auth_conf(TALLOC_CTX *mem_ctx, static errno_t proxy_init_auth_ctx(TALLOC_CTX *mem_ctx, struct be_ctx *be_ctx, + struct data_provider *provider, struct proxy_auth_ctx **_auth_ctx) { struct proxy_auth_ctx *auth_ctx; @@ -213,7 +214,7 @@ static errno_t proxy_init_auth_ctx(TALLOC_CTX *mem_ctx, goto done; } - ret = proxy_client_init(dp_sbus_conn(be_ctx->provider), auth_ctx); + ret = proxy_client_init(dp_sbus_conn(provider), auth_ctx); if (ret != EOK) { goto done; } @@ -273,7 +274,7 @@ errno_t sssm_proxy_init(TALLOC_CTX *mem_ctx, /* Initialize auth_ctx since one of the access, auth or chpass is set. */ - ret = proxy_init_auth_ctx(mem_ctx, be_ctx, &auth_ctx); + ret = proxy_init_auth_ctx(mem_ctx, be_ctx, provider, &auth_ctx); if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, "Unable to create auth context [%d]: %s\n", ret, sss_strerror(ret)); From a37be1b91ef00662bbba27af36ec5de28588f3f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com> Date: Wed, 5 Sep 2018 13:51:55 +0200 Subject: [PATCH 2/5] dp: set be_ctx->provider as part of dp_init request Backend context is overused inside sssd code even during its initialization. Some parts of initialization code requires access to be_ctx->provider so we must make it available as soon as possible. Better solution would be to always use 'provider' directly in initialization but this makes it safer for any future changes as one does not have to keep in mind when it is safe to use be_ctx->provider and when not. Now it is always safe. Resolves: https://pagure.io/SSSD/sssd/issue/3812 --- src/providers/data_provider/dp.c | 21 +++++++++++++-------- src/providers/data_provider/dp.h | 1 - src/providers/data_provider_be.c | 2 +- src/providers/proxy/proxy_init.c | 2 +- 4 files changed, 15 insertions(+), 11 deletions(-) diff --git a/src/providers/data_provider/dp.c b/src/providers/data_provider/dp.c index fd19d2803..bd003c8b3 100644 --- a/src/providers/data_provider/dp.c +++ b/src/providers/data_provider/dp.c @@ -120,6 +120,7 @@ static int dp_destructor(struct data_provider *provider) } struct dp_init_state { + struct be_ctx *be_ctx; struct data_provider *provider; char *sbus_name; }; @@ -158,6 +159,7 @@ dp_init_send(TALLOC_CTX *mem_ctx, goto done; } + state->be_ctx = be_ctx; state->provider->ev = ev; state->provider->uid = uid; state->provider->gid = gid; @@ -224,12 +226,14 @@ static void dp_init_done(struct tevent_req *subreq) 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); + ret = dp_init_modules(state->provider, &state->provider->modules); if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, "Unable to initialize DP modules " "[%d]: %s\n", ret, sss_strerror(ret)); - tevent_req_error(req, ret); - return; + goto done; } ret = dp_init_targets(state->provider, state->provider->be_ctx, @@ -237,25 +241,27 @@ static void dp_init_done(struct tevent_req *subreq) if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, "Unable to initialize DP targets " "[%d]: %s\n", ret, sss_strerror(ret)); - tevent_req_error(req, ret); - return; + goto done; } ret = dp_init_interface(state->provider); if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, "Unable to initialize DP interface " "[%d]: %s\n", ret, sss_strerror(ret)); + goto done; + } + +done: + if (ret != EOK) { + talloc_zfree(state->be_ctx->provider); tevent_req_error(req, ret); - return; } tevent_req_done(req); - return; } errno_t dp_init_recv(TALLOC_CTX *mem_ctx, struct tevent_req *req, - struct data_provider **_provider, const char **_sbus_name) { struct dp_init_state *state; @@ -263,7 +269,6 @@ errno_t dp_init_recv(TALLOC_CTX *mem_ctx, TEVENT_REQ_RETURN_ON_ERROR(req); - *_provider = talloc_steal(mem_ctx, state->provider); *_sbus_name = talloc_steal(mem_ctx, state->sbus_name); return EOK; diff --git a/src/providers/data_provider/dp.h b/src/providers/data_provider/dp.h index 33e6e6567..0028eb1cb 100644 --- a/src/providers/data_provider/dp.h +++ b/src/providers/data_provider/dp.h @@ -117,7 +117,6 @@ dp_init_send(TALLOC_CTX *mem_ctx, errno_t dp_init_recv(TALLOC_CTX *mem_ctx, struct tevent_req *req, - struct data_provider **_provider, const char **_sbus_name); bool _dp_target_enabled(struct data_provider *provider, diff --git a/src/providers/data_provider_be.c b/src/providers/data_provider_be.c index 670ddb477..6d2477e34 100644 --- a/src/providers/data_provider_be.c +++ b/src/providers/data_provider_be.c @@ -541,7 +541,7 @@ static void dp_initialized(struct tevent_req *req) be_ctx = tevent_req_callback_data(req, struct be_ctx); - ret = dp_init_recv(be_ctx, req, &be_ctx->provider, &be_ctx->sbus_name); + ret = dp_init_recv(be_ctx, req, &be_ctx->sbus_name); talloc_zfree(req); if (ret != EOK) { goto done; diff --git a/src/providers/proxy/proxy_init.c b/src/providers/proxy/proxy_init.c index 98c6dd179..32343a3bf 100644 --- a/src/providers/proxy/proxy_init.c +++ b/src/providers/proxy/proxy_init.c @@ -214,7 +214,7 @@ static errno_t proxy_init_auth_ctx(TALLOC_CTX *mem_ctx, goto done; } - ret = proxy_client_init(dp_sbus_conn(provider), auth_ctx); + ret = proxy_client_init(dp_sbus_conn(be_ctx->provider), auth_ctx); if (ret != EOK) { goto done; } From e708913148a96a055e29ed12b33dbd4d50b8e76c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com> Date: Wed, 12 Sep 2018 13:21:11 +0200 Subject: [PATCH 3/5] sbus: read destination after sender is set dbus_message_set_sender may reallocate internal fields which will yield pointer obtained by dbus_message_get_* invalid. --- src/sbus/server/sbus_server_handler.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/sbus/server/sbus_server_handler.c b/src/sbus/server/sbus_server_handler.c index c300d81e1..d4e454780 100644 --- a/src/sbus/server/sbus_server_handler.c +++ b/src/sbus/server/sbus_server_handler.c @@ -148,9 +148,6 @@ sbus_server_filter(DBusConnection *dbus_conn, return DBUS_HANDLER_RESULT_HANDLED; } - destination = dbus_message_get_destination(message); - type = dbus_message_get_type(message); - conn = dbus_connection_get_data(dbus_conn, server->data_slot); if (conn == NULL) { DEBUG(SSSDBG_CRIT_FAILURE, "Unknown connection!\n"); @@ -173,6 +170,11 @@ sbus_server_filter(DBusConnection *dbus_conn, return DBUS_HANDLER_RESULT_HANDLED; } + /* Set sender may reallocate internal fields so this needs to be read + * after we call dbus_message_set_sender(). */ + destination = dbus_message_get_destination(message); + type = dbus_message_get_type(message); + if (type == DBUS_MESSAGE_TYPE_SIGNAL) { return sbus_server_route_signal(server, conn, message, destination); } From 5c7f60f2576b04f30071de9ed7caa5c2d5aa6a8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com> Date: Wed, 12 Sep 2018 13:22:34 +0200 Subject: [PATCH 4/5] sbus: do not try to remove signal listeners when disconnecting This may cause some troubles if the dbus connection was dropped as dbus will try to actually send the messages. Also when the connectin is being freed, tevent integration is already disabled so there is no point in doing this. --- src/sbus/router/sbus_router_hash.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/sbus/router/sbus_router_hash.c b/src/sbus/router/sbus_router_hash.c index 186dc613f..2d407b2fb 100644 --- a/src/sbus/router/sbus_router_hash.c +++ b/src/sbus/router/sbus_router_hash.c @@ -384,6 +384,10 @@ sbus_router_listeners_delete_cb(hash_entry_t *item, return; } + if (conn->disconnecting) { + return; + } + /* If we still have the D-Bus connection available, we try to unregister * the previously registered listener when its removed from table. */ From 999841d42733b53d0533dee70e8948fe4f53de1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com> Date: Wed, 12 Sep 2018 13:24:27 +0200 Subject: [PATCH 5/5] sbus: free watch_fd->fdevent explicitly We never reproduced this with gdb but valgrind shows invalid read in sbus_watch_handler after the watch_fd was freed. This should not be needed since watch_fd is memory parent of fdevent but it seems to help. --- src/sbus/connection/sbus_watch.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/sbus/connection/sbus_watch.c b/src/sbus/connection/sbus_watch.c index 3898311df..0e4bd01d1 100644 --- a/src/sbus/connection/sbus_watch.c +++ b/src/sbus/connection/sbus_watch.c @@ -280,6 +280,7 @@ sbus_watch_remove(DBusWatch *dbus_watch, void *data) if (watch_fd->dbus_watch.read == NULL && watch_fd->dbus_watch.write == NULL) { + talloc_free(watch_fd->fdevent); talloc_free(watch_fd); } }
_______________________________________________ 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://getfedora.org/code-of-conduct.html List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.org
