From 24ef73fe8bbea12ceedddad1953c7bd94ce36f31 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Sun, 11 Oct 2015 15:34:44 +0200 Subject: [PATCH 102/103] FO: Use refcount to keep track of servers returned to callers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves: https://fedorahosted.org/sssd/ticket/2829 Reviewed-by: Pavel Březina (cherry picked from commit 10c07e188323a2f9824b5e34379f3b1a9b37759e) --- src/providers/data_provider_fo.c | 7 ++- src/providers/dp_backend.h | 4 +- src/providers/fail_over.c | 95 ++++++++++++++++++++++++------ src/providers/fail_over.h | 10 +++- src/providers/krb5/krb5_auth.c | 4 +- src/providers/ldap/ldap_auth.c | 2 +- src/providers/ldap/sdap_async_connection.c | 4 +- src/tests/cmocka/test_fo_srv.c | 26 ++++---- src/tests/fail_over-tests.c | 2 +- 9 files changed, 115 insertions(+), 39 deletions(-) diff --git a/src/providers/data_provider_fo.c b/src/providers/data_provider_fo.c index cd57340a0ba0ac7e474dc502bf1f1b4de0e1f778..4c4d7b233bb4bd22aa7c7dcd1fec955c92fb08e4 100644 --- a/src/providers/data_provider_fo.c +++ b/src/providers/data_provider_fo.c @@ -606,7 +606,7 @@ errno_t be_resolve_server_process(struct tevent_req *subreq, time_t srv_status_change; struct be_svc_callback *callback; - ret = fo_resolve_service_recv(subreq, &state->srv); + ret = fo_resolve_service_recv(subreq, state, &state->srv); switch (ret) { case EOK: if (!state->srv) { @@ -699,7 +699,9 @@ errno_t be_resolve_server_process(struct tevent_req *subreq, return EOK; } -int be_resolve_server_recv(struct tevent_req *req, struct fo_server **srv) +int be_resolve_server_recv(struct tevent_req *req, + TALLOC_CTX *ref_ctx, + struct fo_server **srv) { struct be_resolve_server_state *state = tevent_req_data(req, struct be_resolve_server_state); @@ -707,6 +709,7 @@ int be_resolve_server_recv(struct tevent_req *req, struct fo_server **srv) TEVENT_REQ_RETURN_ON_ERROR(req); if (srv) { + fo_ref_server(ref_ctx, state->srv); *srv = state->srv; } diff --git a/src/providers/dp_backend.h b/src/providers/dp_backend.h index 0ced851be8468ce21a9d283e26461fc47194557e..f90d0b9c5fe69b1b14caa090bb515c60746de154 100644 --- a/src/providers/dp_backend.h +++ b/src/providers/dp_backend.h @@ -258,7 +258,9 @@ struct tevent_req *be_resolve_server_send(TALLOC_CTX *memctx, struct be_ctx *ctx, const char *service_name, bool first_try); -int be_resolve_server_recv(struct tevent_req *req, struct fo_server **srv); +int be_resolve_server_recv(struct tevent_req *req, + TALLOC_CTX *ref_ctx, + struct fo_server **srv); #define be_fo_set_port_status(ctx, service_name, server, status) \ _be_fo_set_port_status(ctx, service_name, server, status, \ diff --git a/src/providers/fail_over.c b/src/providers/fail_over.c index b309f1c68d0f4219d4b97eb0c01416e53ea856d0..24aed9dfa469ad730d176244eb329522a43c6fd8 100644 --- a/src/providers/fail_over.c +++ b/src/providers/fail_over.c @@ -79,6 +79,8 @@ struct fo_service { }; struct fo_server { + REFCOUNT_COMMON; + struct fo_server *prev; struct fo_server *next; @@ -90,6 +92,8 @@ struct fo_server { struct fo_service *service; struct timeval last_status_change; struct server_common *common; + + TALLOC_CTX *fo_internal_owner; }; struct server_common { @@ -217,6 +221,15 @@ int fo_is_srv_lookup(struct fo_server *s) return s && s->srv_data; } +static void fo_server_free(struct fo_server *server) +{ + if (server == NULL) { + return; + } + + talloc_free(server->fo_internal_owner); +} + static struct fo_server * collapse_srv_lookup(struct fo_server **_server) { @@ -231,12 +244,12 @@ collapse_srv_lookup(struct fo_server **_server) while (server->prev && server->prev->srv_data == meta->srv_data) { tmp = server->prev; DLIST_REMOVE(server->service->server_list, tmp); - talloc_zfree(tmp); + fo_server_free(tmp); } while (server->next && server->next->srv_data == meta->srv_data) { tmp = server->next; DLIST_REMOVE(server->service->server_list, tmp); - talloc_zfree(tmp); + fo_server_free(tmp); } if (server == server->service->active_server) { @@ -249,7 +262,7 @@ collapse_srv_lookup(struct fo_server **_server) /* add back the meta server to denote SRV lookup */ DLIST_ADD_AFTER(server->service->server_list, meta, server); DLIST_REMOVE(server->service->server_list, server); - talloc_zfree(server); + fo_server_free(server); } meta->srv_data->srv_lookup_status = SRV_NEUTRAL; @@ -502,8 +515,9 @@ create_server_common(TALLOC_CTX *mem_ctx, struct fo_ctx *ctx, const char *name) struct server_common *common; common = rc_alloc(mem_ctx, struct server_common); - if (common == NULL) + if (common == NULL) { return NULL; + } common->name = talloc_strdup(common, name); if (common->name == NULL) { @@ -524,6 +538,41 @@ create_server_common(TALLOC_CTX *mem_ctx, struct fo_ctx *ctx, const char *name) return common; } +static struct fo_server * +fo_server_alloc(struct fo_service *service, int port, + void *user_data, bool primary) +{ + static struct fo_server *server; + TALLOC_CTX *server_owner; + + server_owner = talloc_new(service); + if (server_owner == NULL) { + return NULL; + } + + server = rc_alloc(server_owner, struct fo_server); + if (server == NULL) { + return NULL; + } + + server->fo_internal_owner = server_owner; + + server->common = NULL; + server->next = NULL; + server->prev = NULL; + server->srv_data = NULL; + server->last_status_change.tv_sec = 0; + server->last_status_change.tv_usec = 0; + + server->port = port; + server->user_data = user_data; + server->service = service; + server->port_status = DEFAULT_PORT_STATUS; + server->primary = primary; + + return server; +} + int fo_add_srv_server(struct fo_service *service, const char *srv, const char *discovery_domain, const char *sssd_domain, @@ -557,14 +606,11 @@ fo_add_srv_server(struct fo_service *service, const char *srv, } } - server = talloc_zero(service, struct fo_server); - if (server == NULL) + /* SRV servers are always primary */ + server = fo_server_alloc(service, 0, user_data, true); + if (server == NULL) { return ENOMEM; - - server->user_data = user_data; - server->service = service; - server->port_status = DEFAULT_PORT_STATUS; - server->primary = true; /* SRV servers are never back up */ + } /* add the SRV-specific data */ server->srv_data = talloc_zero(service, struct srv_data); @@ -608,7 +654,7 @@ create_fo_server(struct fo_service *service, const char *name, struct fo_server *server; int ret; - server = talloc_zero(service, struct fo_server); + server = fo_server_alloc(service, port, user_data, primary); if (server == NULL) return NULL; @@ -623,11 +669,11 @@ create_fo_server(struct fo_service *service, const char *name, if (ret == ENOENT) { server->common = create_server_common(server, service->ctx, name); if (server->common == NULL) { - talloc_free(server); + fo_server_free(server); return NULL; } } else if (ret != EOK) { - talloc_free(server); + fo_server_free(server); return NULL; } } @@ -760,7 +806,6 @@ static errno_t fo_add_server_list(struct fo_service *service, server = create_fo_server(service, servers[i].host, servers[i].port, user_data, primary); if (server == NULL) { - talloc_free(srv_list); return ENOMEM; } @@ -769,7 +814,7 @@ static errno_t fo_add_server_list(struct fo_service *service, ret = fo_add_server_to_list(&srv_list, service->server_list, server, service->name); if (ret != EOK) { - talloc_zfree(server); + fo_server_free(server); continue; } @@ -803,12 +848,20 @@ fo_add_server(struct fo_service *service, const char *name, int port, ret = fo_add_server_to_list(&service->server_list, service->server_list, server, service->name); if (ret != EOK) { - talloc_free(server); + fo_server_free(server); } return ret; } +void fo_ref_server(TALLOC_CTX *ref_ctx, + struct fo_server *server) +{ + if (server) { + rc_reference(ref_ctx, struct fo_server, server); + } +} + static int get_first_server_entity(struct fo_service *service, struct fo_server **_server) { @@ -1150,7 +1203,9 @@ fo_resolve_service_done(struct tevent_req *subreq) } int -fo_resolve_service_recv(struct tevent_req *req, struct fo_server **server) +fo_resolve_service_recv(struct tevent_req *req, + TALLOC_CTX *ref_ctx, + struct fo_server **server) { struct resolve_service_state *state; @@ -1158,8 +1213,10 @@ fo_resolve_service_recv(struct tevent_req *req, struct fo_server **server) /* always return the server if asked for, otherwise the caller * cannot mark it as faulty in case we return an error */ - if (server) + if (server != NULL) { + fo_ref_server(ref_ctx, state->server); *server = state->server; + } TEVENT_REQ_RETURN_ON_ERROR(req); diff --git a/src/providers/fail_over.h b/src/providers/fail_over.h index e49c6414a14eb6ca2cad333f8efbb58576811345..75bff8da1cc29dd6184b6ec0d3fad545eb7204c2 100644 --- a/src/providers/fail_over.h +++ b/src/providers/fail_over.h @@ -128,7 +128,6 @@ int fo_add_server(struct fo_service *service, const char *name, int port, void *user_data, bool primary); - int fo_add_srv_server(struct fo_service *service, const char *srv, const char *discovery_domain, @@ -148,8 +147,17 @@ struct tevent_req *fo_resolve_service_send(TALLOC_CTX *mem_ctx, struct fo_service *service); int fo_resolve_service_recv(struct tevent_req *req, + TALLOC_CTX *ref_ctx, struct fo_server **server); + +/* To be used by async consumers of fo_resolve_service. If a server should be returned + * to an outer request, it should be referenced by a memory from that outer request, + * because the failover's server list might change with a subsequent call (see upstream + * bug #2829) + */ +void fo_ref_server(TALLOC_CTX *ref_ctx, struct fo_server *server); + /* * Set feedback about 'server'. Caller should use this to indicate a problem * with the server itself, not only with the service on that server. This diff --git a/src/providers/krb5/krb5_auth.c b/src/providers/krb5/krb5_auth.c index e3e9601b356efd72e50ab86e8b7cdd048e4e70d4..7b7a16a612332639aa474a7ebea6b966df18f08f 100644 --- a/src/providers/krb5/krb5_auth.c +++ b/src/providers/krb5/krb5_auth.c @@ -695,9 +695,9 @@ static void krb5_auth_resolve_done(struct tevent_req *subreq) int ret; if (!state->search_kpasswd) { - ret = be_resolve_server_recv(subreq, &kr->srv); + ret = be_resolve_server_recv(subreq, kr, &kr->srv); } else { - ret = be_resolve_server_recv(subreq, &kr->kpasswd_srv); + ret = be_resolve_server_recv(subreq, kr, &kr->kpasswd_srv); } talloc_zfree(subreq); diff --git a/src/providers/ldap/ldap_auth.c b/src/providers/ldap/ldap_auth.c index 217e80fd07abc41f2594d19397783683d44600cd..c94ba15bb17aa1641eb36781cc59ce158d48ca66 100644 --- a/src/providers/ldap/ldap_auth.c +++ b/src/providers/ldap/ldap_auth.c @@ -695,7 +695,7 @@ static void auth_resolve_done(struct tevent_req *subreq) int ret; bool use_tls; - ret = be_resolve_server_recv(subreq, &state->srv); + ret = be_resolve_server_recv(subreq, state, &state->srv); talloc_zfree(subreq); if (ret) { /* all servers have been tried and none diff --git a/src/providers/ldap/sdap_async_connection.c b/src/providers/ldap/sdap_async_connection.c index 8f5227d263f995693f6e65bd238171538aa52af7..ef7a1594954b4cb11f35d2cd56b0c4806ead797a 100644 --- a/src/providers/ldap/sdap_async_connection.c +++ b/src/providers/ldap/sdap_async_connection.c @@ -1148,7 +1148,7 @@ static void sdap_kinit_kdc_resolved(struct tevent_req *subreq) struct tevent_req *tgtreq; int ret; - ret = be_resolve_server_recv(subreq, &state->kdc_srv); + ret = be_resolve_server_recv(subreq, state, &state->kdc_srv); talloc_zfree(subreq); if (ret != EOK) { /* all servers have been tried and none @@ -1508,7 +1508,7 @@ static void sdap_cli_resolve_done(struct tevent_req *subreq) struct sdap_cli_connect_state); int ret; - ret = be_resolve_server_recv(subreq, &state->srv); + ret = be_resolve_server_recv(subreq, state, &state->srv); talloc_zfree(subreq); if (ret) { state->srv = NULL; diff --git a/src/tests/cmocka/test_fo_srv.c b/src/tests/cmocka/test_fo_srv.c index 109f664c84238cf9c1055a1cbc1a8c8870f2dc39..67f86fb17753bf90b88d007a6a1b309df830c152 100644 --- a/src/tests/cmocka/test_fo_srv.c +++ b/src/tests/cmocka/test_fo_srv.c @@ -201,6 +201,8 @@ struct test_fo_ctx { struct fo_service *fo_svc; struct sss_test_ctx *ctx; int ttl; + + struct fo_server *srv; }; int test_fo_srv_data_cmp(void *ud1, void *ud2) @@ -401,7 +403,7 @@ static void test_fo_srv_done1(struct tevent_req *req) struct fo_server *srv; errno_t ret; - ret = fo_resolve_service_recv(req, &srv); + ret = fo_resolve_service_recv(req, req, &srv); talloc_zfree(req); assert_int_equal(ret, ERR_OK); @@ -426,7 +428,7 @@ static void test_fo_srv_done2(struct tevent_req *req) struct fo_server *srv; errno_t ret; - ret = fo_resolve_service_recv(req, &srv); + ret = fo_resolve_service_recv(req, req, &srv); talloc_zfree(req); assert_int_equal(ret, ERR_OK); @@ -450,7 +452,7 @@ static void test_fo_srv_done3(struct tevent_req *req) struct fo_server *srv; errno_t ret; - ret = fo_resolve_service_recv(req, &srv); + ret = fo_resolve_service_recv(req, req, &srv); talloc_zfree(req); assert_int_equal(ret, ERR_OK); @@ -474,7 +476,7 @@ static void test_fo_srv_done4(struct tevent_req *req) struct fo_server *srv; errno_t ret; - ret = fo_resolve_service_recv(req, &srv); + ret = fo_resolve_service_recv(req, req, &srv); talloc_zfree(req); /* No servers are left..*/ assert_int_equal(ret, ENOENT); @@ -499,7 +501,7 @@ static void test_fo_srv_done5(struct tevent_req *req) struct fo_server *srv; errno_t ret; - ret = fo_resolve_service_recv(req, &srv); + ret = fo_resolve_service_recv(req, req, &srv); talloc_zfree(req); assert_int_equal(ret, ERR_OK); @@ -558,20 +560,19 @@ static void test_fo_srv_before(struct tevent_req *req) { struct test_fo_ctx *test_ctx = \ tevent_req_callback_data(req, struct test_fo_ctx); - struct fo_server *srv; struct ares_srv_reply *s1; struct ares_srv_reply *s2; char *dns_domain; errno_t ret; - ret = fo_resolve_service_recv(req, &srv); + ret = fo_resolve_service_recv(req, test_ctx, &test_ctx->srv); talloc_zfree(req); assert_int_equal(ret, ERR_OK); DEBUG(SSSDBG_TRACE_FUNC, "Before TTL change\n"); - check_server(test_ctx, srv, 389, "ldap1.sssd.com"); - fo_set_server_status(srv, SERVER_WORKING); + check_server(test_ctx, test_ctx->srv, 389, "ldap1.sssd.com"); + fo_set_server_status(test_ctx->srv, SERVER_WORKING); /* Simulate changing the DNS environment. Change the host names */ s1 = mock_ares_reply(test_ctx, "ldap2.sssd.com", 100, 2, 389); @@ -602,10 +603,15 @@ static void test_fo_srv_after(struct tevent_req *req) struct fo_server *srv; errno_t ret; - ret = fo_resolve_service_recv(req, &srv); + ret = fo_resolve_service_recv(req, req, &srv); talloc_zfree(req); assert_int_equal(ret, ERR_OK); + /* Try accessing server from a previous iteration. The + * server should be collapsed, but at least we shouldn't crash + */ + fo_set_server_status(test_ctx->srv, SERVER_WORKING); + /* Must be a different server now */ check_server(test_ctx, srv, 389, "ldap3.sssd.com"); diff --git a/src/tests/fail_over-tests.c b/src/tests/fail_over-tests.c index b21ead38229be5d55df2de10bec3dd00a8566d71..7c296d116968ae059c75920c91059f6e83ea0508 100644 --- a/src/tests/fail_over-tests.c +++ b/src/tests/fail_over-tests.c @@ -154,7 +154,7 @@ test_resolve_service_callback(struct tevent_req *req) task->test_ctx->tasks--; - recv_status = fo_resolve_service_recv(req, &server); + recv_status = fo_resolve_service_recv(req, req, &server); talloc_free(req); fail_if(recv_status != task->recv, "%s: Expected return of %d, got %d", task->location, task->recv, recv_status); -- 2.5.0