From c1f0d65e507d29de907ee5ba6b330a9e6b625c10 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Wed, 18 Nov 2015 20:48:51 +0100 Subject: [PATCH] FO: Use tevent_req_defer_callback() when notifying callers If a fo_resolve_service callback would modify the server->common member in any way, for example by dereferencing the server and lowering the refcount to 0, which would free the common structure, then the next iteration of fo_resolve_service_done would access memory that was already gone. Please see https://tevent.samba.org/group__tevent__request.html#ga09373077d0b39e321a196a86bfebf280 for more details. --- src/providers/fail_over.c | 15 +++++++++++-- src/tests/cmocka/test_fo_srv.c | 49 +++++++++++++++++++++++++++++++++++++++--- 2 files changed, 59 insertions(+), 5 deletions(-) diff --git a/src/providers/fail_over.c b/src/providers/fail_over.c index c60310d8f17b129be115a4d3724ac7e54549820e..0b99098ad9c8caeb6d8503066f3e99d78faeb001 100644 --- a/src/providers/fail_over.c +++ b/src/providers/fail_over.c @@ -131,6 +131,7 @@ struct resolve_service_request { struct server_common *server_common; struct tevent_req *req; + struct tevent_context *ev; }; struct status { @@ -940,7 +941,9 @@ resolve_service_request_destructor(struct resolve_service_request *request) } static int -set_lookup_hook(struct fo_server *server, struct tevent_req *req) +set_lookup_hook(struct tevent_context *ev, + struct fo_server *server, + struct tevent_req *req) { struct resolve_service_request *request; @@ -956,6 +959,7 @@ set_lookup_hook(struct fo_server *server, struct tevent_req *req) talloc_free(request); return ENOMEM; } + request->ev = ev; request->req = req; DLIST_ADD(server->common->request_list, request); talloc_set_destructor(request, resolve_service_request_destructor); @@ -1142,7 +1146,7 @@ fo_resolve_service_server(struct tevent_req *req) case SERVER_RESOLVING_NAME: /* Name resolution is already under way. Just add ourselves into the * waiting queue so we get notified after the operation is finished. */ - ret = set_lookup_hook(state->server, req); + ret = set_lookup_hook(state->ev, state->server, req); if (ret != EOK) { tevent_req_error(req, ret); return true; @@ -1194,6 +1198,13 @@ fo_resolve_service_done(struct tevent_req *subreq) /* Take care of all requests for this server. */ while ((request = common->request_list) != NULL) { DLIST_REMOVE(common->request_list, request); + + /* If the request callback decresed refcount on the returned + * server, we would have crashed as common would not be valid + * anymore. Rather schedule the notify for next tev iteration + */ + tevent_req_defer_callback(request->req, request->ev); + if (ret) { tevent_req_error(request->req, ret); } else { diff --git a/src/tests/cmocka/test_fo_srv.c b/src/tests/cmocka/test_fo_srv.c index 67f86fb17753bf90b88d007a6a1b309df830c152..a84ce4348d2e59aaab4fc9ac1bd4cfd853ff491d 100644 --- a/src/tests/cmocka/test_fo_srv.c +++ b/src/tests/cmocka/test_fo_srv.c @@ -575,10 +575,10 @@ static void test_fo_srv_before(struct tevent_req *req) 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); + s1 = mock_ares_reply(test_ctx, "ldap1.sssd.com", 100, 2, 389); assert_non_null(s1); - s2 = mock_ares_reply(test_ctx, "ldap3.sssd.com", 100, 1, 389); + s2 = mock_ares_reply(test_ctx, "ldap2.sssd.com", 100, 1, 389); assert_non_null(s2); s1->next = s2; @@ -596,12 +596,17 @@ static void test_fo_srv_before(struct tevent_req *req) tevent_req_set_callback(req, test_fo_srv_after, test_ctx); } +static void test_fo_srv_after2(struct tevent_req *req); + static void test_fo_srv_after(struct tevent_req *req) { struct test_fo_ctx *test_ctx = \ tevent_req_callback_data(req, struct test_fo_ctx); struct fo_server *srv; errno_t ret; + struct ares_srv_reply *s1; + struct ares_srv_reply *s2; + char *dns_domain; ret = fo_resolve_service_recv(req, req, &srv); talloc_zfree(req); @@ -612,8 +617,46 @@ static void test_fo_srv_after(struct tevent_req *req) */ fo_set_server_status(test_ctx->srv, SERVER_WORKING); + sleep(test_ctx->ttl + 1); + /* Must be a different server now */ - check_server(test_ctx, srv, 389, "ldap3.sssd.com"); + check_server(test_ctx, srv, 389, "ldap2.sssd.com"); + + /* Simulate changing the DNS environment. Change the host names */ + s1 = mock_ares_reply(test_ctx, "ldap1.sssd.com", 100, 1, 389); + assert_non_null(s1); + + s2 = mock_ares_reply(test_ctx, "ldap2.sssd.com", 100, 2, 389); + assert_non_null(s2); + + s1->next = s2; + + dns_domain = talloc_strdup(test_ctx, "sssd.com"); + assert_non_null(dns_domain); + + mock_srv_results(s1, test_ctx->ttl, dns_domain); + sleep(test_ctx->ttl + 1); + + req = fo_resolve_service_send(test_ctx, test_ctx->ctx->ev, + test_ctx->resolv, test_ctx->fo_ctx, + test_ctx->fo_svc); + assert_non_null(req); + tevent_req_set_callback(req, test_fo_srv_after2, test_ctx); +} + +static void test_fo_srv_after2(struct tevent_req *req) +{ + struct test_fo_ctx *test_ctx = \ + tevent_req_callback_data(req, struct test_fo_ctx); + struct fo_server *srv; + errno_t ret; + + ret = fo_resolve_service_recv(req, req, &srv); + talloc_zfree(req); + assert_int_equal(ret, ERR_OK); + + /* Must be a different server now */ + check_server(test_ctx, srv, 389, "ldap1.sssd.com"); test_ctx->ctx->error = ERR_OK; test_ctx->ctx->done = true; -- 2.4.3