From bb102b392ff298ecd8a499f6ddc3904c59dcbba2 Mon Sep 17 00:00:00 2001

From: Jakub Hrozek <jhrozek@redhat.com>

Date: Wed, 18 Nov 2015 20:48:51 +0100

Subject: [PATCH 104/104] FO: Use tevent_req_defer_callback() when notifying

 callers

MIME-Version: 1.0

Content-Type: text/plain; charset=UTF-8

Content-Transfer-Encoding: 8bit

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.

Reviewed-by: Pavel Březina <pbrezina@redhat.com>

(cherry picked from commit a92f68763a57b211a1bf6b80b6dd80c4a1aa2738)

---

 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 c60310d..0b99098 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 67f86fb..a84ce43 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.5.0