FO: Check server validity before setting status

The list of resolved servers is allocated on the back end context and
kept in the fo_service structure. However, a single request often
resolves a server and keeps a pointer until the end of a request and
only then gives feedback about the server based on the request result.

This presents a big race condition in case the SRV resolution is used.
When there are requests coming in in parallel, it is possible that an
incoming request will invalidate a server until another request that
holds a pointer to the original server is able to give a feedback.

This patch simply checks if a server is in the list of servers
maintained by a service before reading its status.

https://fedorahosted.org/sssd/ticket/1364

7 files changed, 49 insertions, 33 deletions

diff --git a/src/providers/data_provider_fo.c b/src/providers/data_provider_fo.c
index 54c0841f9..c66912d56 100644
--- a/src/providers/data_provider_fo.c
+++ b/src/providers/data_provider_fo.c
@@ -707,32 +707,31 @@ void reset_fo(struct be_ctx *be_ctx)
 }
 
 void be_fo_set_port_status(struct be_ctx *ctx,
+                           const char *service_name,
                            struct fo_server *server,
                            enum port_status status)
 {
-    struct be_svc_data *svc;
-    struct fo_service *fsvc;
-
-    fo_set_port_status(server, status);
+    struct be_svc_data *be_svc;
 
-    fsvc = fo_get_server_service(server);
-    if (!fsvc) {
-        DEBUG(SSSDBG_OP_FAILURE, ("BUG: No service associated with server\n"));
+    be_svc = be_fo_find_svc_data(ctx, service_name);
+    if (be_svc == NULL) {
+        DEBUG(SSSDBG_OP_FAILURE,
+              ("No service associated with name %s\n", service_name));
         return;
     }
 
-    DLIST_FOR_EACH(svc, ctx->be_fo->svcs) {
-        if (svc->fo_service == fsvc) break;
-    }
-
-    if (!svc) {
-        DEBUG(SSSDBG_OP_FAILURE, ("BUG: Unknown service\n"));
+    if (!fo_svc_has_server(be_svc->fo_service, server)) {
+        DEBUG(SSSDBG_OP_FAILURE,
+              ("The server %p is not valid anymore, cannot set its status\n"));
         return;
     }
 
+    /* Now we know that the server is valid */
+    fo_set_port_status(server, status);
+
     if (status == PORT_WORKING) {
         /* We were successful in connecting to the server. Cycle through all
          * available servers next time */
-        svc->first_resolved = NULL;
+        be_svc->first_resolved = NULL;
     }
 }
diff --git a/src/providers/dp_backend.h b/src/providers/dp_backend.h
index b98d15078..357d58910 100644
--- a/src/providers/dp_backend.h
+++ b/src/providers/dp_backend.h
@@ -242,6 +242,7 @@ struct tevent_req *be_resolve_server_send(TALLOC_CTX *memctx,
 int be_resolve_server_recv(struct tevent_req *req, struct fo_server **srv);
 
 void be_fo_set_port_status(struct be_ctx *ctx,
+                           const char *service_name,
                            struct fo_server *server,
                            enum port_status status);
 
diff --git a/src/providers/fail_over.c b/src/providers/fail_over.c
index 422cd675b..d776037ac 100644
--- a/src/providers/fail_over.c
+++ b/src/providers/fail_over.c
@@ -1564,10 +1564,13 @@ void fo_reset_services(struct fo_ctx *fo_ctx)
     }
 }
 
-struct fo_service *
-fo_get_server_service(struct fo_server *server)
+bool fo_svc_has_server(struct fo_service *service, struct fo_server *server)
 {
-    if (!server) return NULL;
-    return server->service;
-}
+    struct fo_server *srv;
+
+    DLIST_FOR_EACH(srv, service->server_list) {
+        if (srv == server) return true;
+    }
 
+    return false;
+}
diff --git a/src/providers/fail_over.h b/src/providers/fail_over.h
index 36437ed56..0eb212584 100644
--- a/src/providers/fail_over.h
+++ b/src/providers/fail_over.h
@@ -194,6 +194,6 @@ int fo_is_srv_lookup(struct fo_server *s);
 
 void fo_reset_services(struct fo_ctx *fo_ctx);
 
-struct fo_service *fo_get_server_service(struct fo_server *server);
+bool fo_svc_has_server(struct fo_service *service, struct fo_server *server);
 
 #endif /* !__FAIL_OVER_H__ */
diff --git a/src/providers/krb5/krb5_auth.c b/src/providers/krb5/krb5_auth.c
index ec488e7dd..a305bb69c 100644
--- a/src/providers/krb5/krb5_auth.c
+++ b/src/providers/krb5/krb5_auth.c
@@ -819,6 +819,7 @@ static void krb5_child_done(struct tevent_req *subreq)
         /* ..which is unreachable by now.. */
         if (res->msg_status == PAM_AUTHTOK_LOCK_BUSY) {
             be_fo_set_port_status(state->be_ctx,
+                                  state->krb5_ctx->service->name,
                                   kr->kpasswd_srv, PORT_NOT_WORKING);
             /* ..try to resolve next kpasswd server */
             if (krb5_next_kpasswd(req) == NULL) {
@@ -827,6 +828,7 @@ static void krb5_child_done(struct tevent_req *subreq)
             return;
         } else {
             be_fo_set_port_status(state->be_ctx,
+                                  state->krb5_ctx->service->name,
                                   kr->kpasswd_srv, PORT_WORKING);
         }
     }
@@ -837,7 +839,8 @@ static void krb5_child_done(struct tevent_req *subreq)
     if (res->msg_status == PAM_AUTHINFO_UNAVAIL ||
         (kr->kpasswd_srv == NULL && res->msg_status == PAM_AUTHTOK_LOCK_BUSY)) {
         if (kr->srv != NULL) {
-            be_fo_set_port_status(state->be_ctx, kr->srv, PORT_NOT_WORKING);
+            be_fo_set_port_status(state->be_ctx, state->krb5_ctx->service->name,
+                                  kr->srv, PORT_NOT_WORKING);
             /* ..try to resolve next KDC */
             if (krb5_next_kdc(req) == NULL) {
                 tevent_req_error(req, ENOMEM);
@@ -845,7 +848,8 @@ static void krb5_child_done(struct tevent_req *subreq)
             return;
         }
     } else if (kr->srv != NULL) {
-        be_fo_set_port_status(state->be_ctx, kr->srv, PORT_WORKING);
+        be_fo_set_port_status(state->be_ctx, state->krb5_ctx->service->name,
+                              kr->srv, PORT_WORKING);
     }
 
     /* Now only a successful authentication or password change is left.
@@ -917,19 +921,19 @@ static struct tevent_req *krb5_next_server(struct tevent_req *req)
     switch (pd->cmd) {
         case SSS_PAM_AUTHENTICATE:
         case SSS_CMD_RENEW:
-            be_fo_set_port_status(state->be_ctx,
+            be_fo_set_port_status(state->be_ctx, state->krb5_ctx->service->name,
                                   state->kr->srv, PORT_NOT_WORKING);
             next_req = krb5_next_kdc(req);
             break;
         case SSS_PAM_CHAUTHTOK:
         case SSS_PAM_CHAUTHTOK_PRELIM:
             if (state->kr->kpasswd_srv) {
-                be_fo_set_port_status(state->be_ctx,
+                be_fo_set_port_status(state->be_ctx, state->krb5_ctx->service->name,
                                       state->kr->kpasswd_srv, PORT_NOT_WORKING);
                 next_req = krb5_next_kpasswd(req);
                 break;
             } else {
-                be_fo_set_port_status(state->be_ctx,
+                be_fo_set_port_status(state->be_ctx, state->krb5_ctx->service->name,
                                       state->kr->srv, PORT_NOT_WORKING);
                 next_req = krb5_next_kdc(req);
                 break;
diff --git a/src/providers/ldap/ldap_auth.c b/src/providers/ldap/ldap_auth.c
index cc5eff1b2..32a2e04ea 100644
--- a/src/providers/ldap/ldap_auth.c
+++ b/src/providers/ldap/ldap_auth.c
@@ -605,6 +605,7 @@ static void auth_connect_done(struct tevent_req *subreq)
         if (state->srv) {
             /* mark this server as bad if connection failed */
             be_fo_set_port_status(state->ctx->be,
+                                  state->sdap_service->name,
                                   state->srv, PORT_NOT_WORKING);
         }
         if (ret == ETIMEDOUT) {
@@ -617,7 +618,8 @@ static void auth_connect_done(struct tevent_req *subreq)
         tevent_req_error(req, ret);
         return;
     } else if (state->srv) {
-        be_fo_set_port_status(state->ctx->be, state->srv, PORT_WORKING);
+        be_fo_set_port_status(state->ctx->be, state->sdap_service->name,
+                              state->srv, PORT_WORKING);
     }
 
     ret = get_user_dn(state, state->ctx->be->sysdb, state->ctx->opts,
diff --git a/src/providers/ldap/sdap_async_connection.c b/src/providers/ldap/sdap_async_connection.c
index 9fee1a5d4..79ad3b8e4 100644
--- a/src/providers/ldap/sdap_async_connection.c
+++ b/src/providers/ldap/sdap_async_connection.c
@@ -1012,7 +1012,8 @@ static void sdap_kinit_done(struct tevent_req *subreq)
          * retry with another KDC */
         DEBUG(SSSDBG_MINOR_FAILURE,
               ("Communication with KDC timed out, trying the next one\n"));
-        be_fo_set_port_status(state->be, state->kdc_srv, PORT_NOT_WORKING);
+        be_fo_set_port_status(state->be, state->krb_service_name,
+                              state->kdc_srv, PORT_NOT_WORKING);
         nextreq = sdap_kinit_next_kdc(req);
         if (!nextreq) {
             tevent_req_error(req, ENOMEM);
@@ -1040,7 +1041,8 @@ static void sdap_kinit_done(struct tevent_req *subreq)
         return;
     } else {
         if (kerr == KRB5_KDC_UNREACH) {
-            be_fo_set_port_status(state->be, state->kdc_srv, PORT_NOT_WORKING);
+            be_fo_set_port_status(state->be, state->krb_service_name,
+                                  state->kdc_srv, PORT_NOT_WORKING);
             nextreq = sdap_kinit_next_kdc(req);
             if (!nextreq) {
                 tevent_req_error(req, ENOMEM);
@@ -1371,7 +1373,8 @@ static void sdap_cli_connect_done(struct tevent_req *subreq)
     talloc_zfree(subreq);
     if (ret) {
         /* retry another server */
-        be_fo_set_port_status(state->be, state->srv, PORT_NOT_WORKING);
+        be_fo_set_port_status(state->be, state->service->name,
+                              state->srv, PORT_NOT_WORKING);
         ret = sdap_cli_resolve_next(req);
         if (ret != EOK) {
             tevent_req_error(req, ret);
@@ -1444,7 +1447,8 @@ static void sdap_cli_rootdse_done(struct tevent_req *subreq)
     talloc_zfree(subreq);
     if (ret) {
         if (ret == ETIMEDOUT) { /* retry another server */
-            be_fo_set_port_status(state->be, state->srv, PORT_NOT_WORKING);
+            be_fo_set_port_status(state->be, state->service->name,
+                                  state->srv, PORT_NOT_WORKING);
             ret = sdap_cli_resolve_next(req);
             if (ret != EOK) {
                 tevent_req_error(req, ret);
@@ -1681,7 +1685,8 @@ static void sdap_cli_rootdse_auth_done(struct tevent_req *subreq)
         if (ret == ETIMEDOUT) {
             /* The server we authenticated against went down. Retry another
              * one */
-            be_fo_set_port_status(state->be, state->srv, PORT_NOT_WORKING);
+            be_fo_set_port_status(state->be, state->service->name,
+                                  state->srv, PORT_NOT_WORKING);
             ret = sdap_cli_resolve_next(req);
             if (ret != EOK) {
                 tevent_req_error(req, ret);
@@ -1729,7 +1734,8 @@ int sdap_cli_connect_recv(struct tevent_req *req,
     if (tevent_req_is_error(req, &tstate, &err)) {
         /* mark the server as bad if connection failed */
         if (state->srv) {
-            be_fo_set_port_status(state->be, state->srv, PORT_NOT_WORKING);
+            be_fo_set_port_status(state->be, state->service->name,
+                                  state->srv, PORT_NOT_WORKING);
         } else {
             if (can_retry) {
                 *can_retry = false;
@@ -1741,7 +1747,8 @@ int sdap_cli_connect_recv(struct tevent_req *req,
         }
         return EIO;
     } else if (state->srv) {
-        be_fo_set_port_status(state->be, state->srv, PORT_WORKING);
+        be_fo_set_port_status(state->be, state->service->name,
+                              state->srv, PORT_WORKING);
     }
 
     if (gsh) {