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.

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

2 files changed, 59 insertions, 5 deletions

diff --git a/src/providers/fail_over.c b/src/providers/fail_over.c
index c60310d8f..0b99098ad 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 67f86fb17..a84ce4348 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;