Stop a stalled Call Home peer from blocking configuration applies

src/session.c

@@ -1171,17 +1171,24 @@ nc_session_free(struct nc_session *session, void (*data_free)(void *))
     session->status = NC_STATUS_CLOSING;
 
     if ((session->side == NC_SERVER) && (session->flags & NC_SESSION_CH_THREAD)) {
+        /* signaling a condition does not require its mutex to be held */
         pthread_cond_signal(&session->opts.server.ch_cond);
 
-        nc_timeouttime_get(&ts, NC_SESSION_FREE_LOCK_TIMEOUT);
+        if (ch_locked) {
+            nc_timeouttime_get(&ts, NC_SESSION_FREE_LOCK_TIMEOUT);
 
-        /* wait for CH thread to actually wake up and terminate */
-        r = 0;
-        while (!r && (session->flags & NC_SESSION_CH_THREAD)) {
-            r = pthread_cond_clockwait(&session->opts.server.ch_cond, &session->opts.server.ch_lock, COMPAT_CLOCK_ID, &ts);
-        }
-        if (r) {
-            ERR(session, "Waiting for Call Home thread failed (%s).", strerror(r));
+            /* wait for CH thread to actually wake up and terminate */
+            r = 0;
+            while (!r && (session->flags & NC_SESSION_CH_THREAD)) {
+                r = pthread_cond_clockwait(&session->opts.server.ch_cond, &session->opts.server.ch_lock, COMPAT_CLOCK_ID, &ts);
+            }
+            if (r) {
+                ERR(session, "Waiting for Call Home thread failed (%s).", strerror(r));
+            }
+        } else {
+            /* waiting on a condition requires its mutex to be held by the caller, so there is no
+             * way to wait for the Call Home thread without ch_lock */
+            ERR(session, "Freeing a Call Home session without its lock, not waiting for its thread.");
         }
     }
 
@@ -2108,6 +2115,12 @@ nc_session_curl_init(CURL **handle, struct nc_curl_data *data)
         return 1;
     }
 
+    /* limit the whole transfer, a host that connects and then stalls would block the TLS handshake */
+    if (curl_easy_setopt(*handle, CURLOPT_TIMEOUT_MS, NC_CURL_TIMEOUT_MS)) {
+        ERR(NULL, "Setting curl transfer timeout failed.");
+        return 1;
+    }
+
     /* do not use signals for timeouts, required for thread safety */
     if (curl_easy_setopt(*handle, CURLOPT_NOSIGNAL, 1L)) {
         ERR(NULL, "Setting CURLOPT_NOSIGNAL failed.");

src/session_client.c

@@ -1847,7 +1847,7 @@ nc_client_ch_del_bind(const char *address, uint16_t port, NC_TRANSPORT_IMPL ti)
 API int
 nc_accept_callhome(int timeout, struct ly_ctx *ctx, struct nc_session **session)
 {
-    int ret, sock;
+    int ret, sock = -1;
     char *host = NULL;
     uint16_t port, bind_idx = 0;
 

src/session_p.h

@@ -73,6 +73,11 @@ extern struct nc_server_opts server_opts;
  */
 #define NC_TRANSPORT_MSG_TIMEOUT 2000
 
+/**
+ * Maximum time in msec a transport handshake may block before it checks whether it was interrupted.
+ */
+#define NC_HANDSHAKE_INTERRUPT_STEP 100
+
 /**
  * Timeout in msec for acquiring a lock of a session (used with a condition, so higher numbers could be required
  * only in case of extreme concurrency).
@@ -129,6 +134,16 @@ extern struct nc_server_opts server_opts;
  */
 #define NC_CURL_CONNECT_TIMEOUT_MS 2000
 
+/**
+ * @brief Timeout in msec for a whole CRL download.
+ *
+ * The connection timeout alone does not bound a CRL distribution point that accepts the connection
+ * and then sends data very slowly or not at all. Since the download happens in the middle of a TLS
+ * handshake, and a Call Home handshake blocks a configuration apply, the whole transfer has to be
+ * bounded as well.
+ */
+#define NC_CURL_TIMEOUT_MS 10000
+
 /**
  * @brief Timeout in msec for acquiring the hello_lock
  * (iterating through all YANG modules + building capability strings)
@@ -1120,6 +1135,16 @@ struct nc_session {
             const struct nc_server_config *config;
 
 #ifdef NC_ENABLED_SSH_TLS
+            /**
+             * @brief Running flag of the Call Home thread performing the transport handshake.
+             *
+             * A borrowed pointer to ::nc_server_ch_thread_arg.thread_running, NOT owned by the
+             * session - the handshake is aborted as soon as it becomes 0. NULL for a handshake that
+             * cannot be interrupted, which is every handshake done by ::nc_accept(). Set and cleared
+             * by ::nc_connect_ch_endpt() exactly like ::nc_session.opts.server.config.
+             */
+            ATOMIC_T *ch_thread_running;
+
             uint16_t ssh_auth_attempts;    /**< number of failed SSH authentication attempts */
             void *client_cert;                /**< TLS client certificate if used for authentication */
 #endif /* NC_ENABLED_SSH_TLS */
@@ -1655,13 +1680,36 @@ int _nc_connect_ch_client_dispatch(const char *client_name, nc_server_ch_session
  */
 struct nc_session *nc_accept_callhome_ssh_sock(int sock, const char *host, uint16_t port, struct ly_ctx *ctx);
 
+/**
+ * @brief Check whether the transport handshake of a session should be aborted.
+ *
+ * Only a handshake performed by a Call Home thread can be interrupted, see
+ * ::nc_session.opts.server.ch_thread_running. Handshakes of accepted sessions are never interrupted.
+ *
+ * @param[in] session Session performing a transport handshake.
+ * @return 1 if the handshake should be aborted, 0 otherwise.
+ */
+int nc_session_handshake_interrupted(const struct nc_session *session);
+
+/**
+ * @brief Cap a transport handshake poll timeout so that an interrupt is noticed in time.
+ *
+ * A handshake that cannot be interrupted gets @p timeout unchanged, there is nothing it could
+ * notice by waking up before the data it is waiting for arrive.
+ *
+ * @param[in] session Session performing a transport handshake.
+ * @param[in] timeout Timeout in msec the handshake would like to wait for, negative means indefinitely.
+ * @return Timeout in msec to actually use, never negative for an interruptible handshake.
+ */
+int32_t nc_session_handshake_poll_timeout(const struct nc_session *session, int32_t timeout);
+
 /**
  * @brief Establish SSH transport on a socket.
  *
  * @param[in] session Session structure of the new connection.
  * @param[in] opts SSH server options to use.
  * @param[in] sock Socket of the new connection, closed if not set to the session.
- * @return 1 on success, 0 on timeout, -1 on error.
+ * @return 1 on success, 0 on timeout or interrupt, -1 on error.
  */
 int nc_accept_ssh_session(struct nc_session *session, struct nc_server_ssh_opts *opts, int sock);
 
@@ -1677,7 +1725,7 @@ struct nc_session *nc_accept_callhome_tls_sock(int sock, const char *host, uint1
  * @param[in] session Session structure of the new connection.
  * @param[in] sock Socket of the new connection.
  * @param[in] timeout Transport operations timeout in msec.
- * @return 1 on success, 0 on timeout, -1 on error.
+ * @return 1 on success, 0 on timeout or interrupt, -1 on error.
  */
 int nc_accept_tls_session(struct nc_session *session, struct nc_server_tls_opts *opts, int sock);
 

src/session_server.c

@@ -1087,6 +1087,7 @@ nc_sock_accept_pollfds(struct pollfd *pollfds, uint16_t pollfd_count, const char
     /* make the socket non-blocking */
     if (((flags = fcntl(client_sock, F_GETFL)) == -1) || (fcntl(client_sock, F_SETFL, flags | O_NONBLOCK) == -1)) {
         ERR(NULL, "Fcntl failed (%s).", strerror(errno));
+        ret = -1;
         goto cleanup;
     }
 
@@ -3744,6 +3745,35 @@ nc_accept(int timeout, const struct ly_ctx *ctx, struct nc_session **session)
 
 #ifdef NC_ENABLED_SSH_TLS
 
+int
+nc_session_handshake_interrupted(const struct nc_session *session)
+{
+    ATOMIC_T *ch_thread_running = session->opts.server.ch_thread_running;
+
+    if (!ch_thread_running) {
+        /* not a Call Home handshake, there is nobody to interrupt it */
+        return 0;
+    }
+
+    return !ATOMIC_LOAD_RELAXED(*ch_thread_running);
+}
+
+int32_t
+nc_session_handshake_poll_timeout(const struct nc_session *session, int32_t timeout)
+{
+    if (!session->opts.server.ch_thread_running) {
+        /* nothing can interrupt the handshake, there is no reason to wake up early */
+        return timeout;
+    }
+
+    /* a negative timeout means waiting indefinitely, which must not happen if we have to notice an interrupt */
+    if ((timeout < 0) || (timeout > NC_HANDSHAKE_INTERRUPT_STEP)) {
+        return NC_HANDSHAKE_INTERRUPT_STEP;
+    }
+
+    return timeout;
+}
+
 API int
 nc_server_ch_is_client(const char *name)
 {
@@ -3808,6 +3838,8 @@ nc_server_ch_client_is_endpt(const char *client_name, const char *endpt_name)
  * @param[in] config Pinned server configuration @p endpt belongs to, pinned into the created session
  * for the duration of the transport handshake.
  * @param[in] endpt Endpoint to use.
+ * @param[in] ch_thread_running Running flag of the calling Call Home thread, the transport handshake
+ * is aborted as soon as it becomes 0.
  * @param[in,out] cur_sock_pending Current pending socket for the connection.
  * @param[in] acquire_ctx_cb Callback for acquiring the libyang context.
  * @param[in] release_ctx_cb Callback for releasing the libyang context.
@@ -3816,9 +3848,9 @@ nc_server_ch_client_is_endpt(const char *client_name, const char *endpt_name)
  * @return NC_MSG values.
  */
 static NC_MSG_TYPE
-nc_connect_ch_endpt(const struct nc_server_config *config, const struct nc_ch_endpt *endpt, int *cur_sock_pending,
-        nc_server_ch_session_acquire_ctx_cb acquire_ctx_cb, nc_server_ch_session_release_ctx_cb release_ctx_cb,
-        void *ctx_cb_data, struct nc_session **session)
+nc_connect_ch_endpt(const struct nc_server_config *config, const struct nc_ch_endpt *endpt,
+        ATOMIC_T *ch_thread_running, int *cur_sock_pending, nc_server_ch_session_acquire_ctx_cb acquire_ctx_cb,
+        nc_server_ch_session_release_ctx_cb release_ctx_cb, void *ctx_cb_data, struct nc_session **session)
 {
     NC_MSG_TYPE msgtype;
     const struct ly_ctx *ctx = NULL;
@@ -3856,6 +3888,9 @@ nc_connect_ch_endpt(const struct nc_server_config *config, const struct nc_ch_en
     /* pin the configuration for the duration of the transport handshake, it is a borrowed pointer */
     (*session)->opts.server.config = config;
 
+    /* let the handshake be aborted as soon as this thread is told to stop, also a borrowed pointer */
+    (*session)->opts.server.ch_thread_running = ch_thread_running;
+
     /* sock gets assigned to session or closed */
     if (endpt->ti == NC_TI_SSH) {
         ret = nc_accept_ssh_session(*session, endpt->opts.ssh, sock);
@@ -3887,8 +3922,10 @@ nc_connect_ch_endpt(const struct nc_server_config *config, const struct nc_ch_en
         goto fail;
     }
 
-    /* the transport handshake is over, the configuration must not be reached through the session anymore */
+    /* the transport handshake is over, neither the configuration nor the running flag must be
+     * reached through the session anymore */
     (*session)->opts.server.config = NULL;
+    (*session)->opts.server.ch_thread_running = NULL;
 
     /* assign new SID atomically */
     (*session)->id = ATOMIC_INC_RELAXED(server_opts.new_session_id);
@@ -3910,6 +3947,7 @@ nc_connect_ch_endpt(const struct nc_server_config *config, const struct nc_ch_en
 fail:
     if (*session) {
         (*session)->opts.server.config = NULL;
+        (*session)->opts.server.ch_thread_running = NULL;
     }
     nc_session_free(*session, NULL);
     *session = NULL;
@@ -3954,6 +3992,10 @@ nc_server_ch_client_get_idle_timeout(const char *client_name, uint32_t *idle_tim
 /**
  * @brief Wait for any event after a NC session was established on a CH client.
  *
+ * The session is given to the user by ::nc_server_ch_thread_arg.new_session_cb. Until that
+ * succeeds the session still belongs to the Call Home thread, so it is freed here on any error.
+ * Afterwards it belongs to the user and is never freed here.
+ *
  * @param[in] data CH client thread argument.
  * @param[in] session New NC session. The session is invalid upon being freed (= function exit).
  * @return 0 if session was terminated normally,
@@ -3969,6 +4011,9 @@ nc_server_ch_client_thread_session_cond_wait(struct nc_server_ch_thread_arg *dat
 
     /* CH LOCK */
     if (nc_mutex_lock(&session->opts.server.ch_lock, NC_SESSION_CH_LOCK_TIMEOUT, __func__) != 1) {
+        /* the session has not been given to the user yet, so it is still ours to free */
+        nc_session_free(session, NULL);
+        data->release_ctx_cb(data->ctx_cb_data);
         return -1;
     }
 
@@ -4245,8 +4290,8 @@ nc_ch_client_thread(void *arg)
         }
 
         /* try to connect to the endpoint, the configuration stays pinned for the whole handshake */
-        msgtype = nc_connect_ch_endpt(config, cur_endpt, &cur_sock_pending, data->acquire_ctx_cb,
-                data->release_ctx_cb, data->ctx_cb_data, &session);
+        msgtype = nc_connect_ch_endpt(config, cur_endpt, &data->thread_running, &cur_sock_pending,
+                data->acquire_ctx_cb, data->release_ctx_cb, data->ctx_cb_data, &session);
         if (msgtype == NC_MSG_HELLO) {
             /* session established, the configuration is not needed anymore */
             nc_server_config_release(config);
@@ -4255,7 +4300,11 @@ nc_ch_client_thread(void *arg)
             cur_endpt = NULL;
 
             if (!ATOMIC_LOAD_RELAXED(data->thread_running)) {
-                /* thread should stop running */
+                /* thread should stop running, the session has not been given to the user yet,
+                 * so it is still ours to free */
+                nc_session_free(session, NULL);
+                session = NULL;
+                data->release_ctx_cb(data->ctx_cb_data);
                 goto cleanup;
             }
 
@@ -4331,6 +4380,12 @@ nc_ch_client_thread(void *arg)
             }
             cur_attempts = 0;
         } else {
+            if (!ATOMIC_LOAD_RELAXED(data->thread_running)) {
+                /* the handshake was interrupted because this thread should stop, do not count it as
+                 * a failed attempt and do not bother the user with it */
+                goto cleanup;
+            }
+
             /* session was not created, wait a little bit and try again */
             ++cur_attempts;
 

src/session_server_ssh.c

@@ -1625,6 +1625,11 @@ nc_accept_ssh_session_open_netconf_channel(struct nc_session *session, struct nc
             return -1;
         }
 
+        if (nc_session_handshake_interrupted(session)) {
+            VRB(session, "Waiting for the \"netconf\" SSH subsystem interrupted, the Call Home thread is terminating.");
+            return 0;
+        }
+
         time_diff = nc_timeouttime_cur_diff(&ts_timeout);
         if (time_diff < 1) {
             /* timeout */
@@ -1633,14 +1638,13 @@ nc_accept_ssh_session_open_netconf_channel(struct nc_session *session, struct nc
         }
 
         /* This functions listens to the network and automatically calls callback funcitons. */
-        ret = ssh_event_dopoll(session->ti.libssh.event, time_diff);
+        ret = ssh_event_dopoll(session->ti.libssh.event, nc_session_handshake_poll_timeout(session, time_diff));
         if (ret == SSH_ERROR) {
             ERR(session, "Failed to poll SSH event (%s).", ssh_get_error(session->ti.libssh.session));
             return -1;
-        } else if (ret == SSH_AGAIN) {
-            /* Timeout reached */
-            break;
         }
+        /* SSH_AGAIN only means the poll timeout elapsed, which may have been shortened to notice an
+         * interrupt, so ts_timeout checked at the top of the loop is the only authority */
     }
 
     if (session->flags & NC_SESSION_SSH_SUBSYS_NETCONF) {
@@ -1666,6 +1670,11 @@ nc_accept_ssh_session_open_netconf_channel(struct nc_session *session, struct nc
             return 1;
         }
 
+        if (nc_session_handshake_interrupted(session)) {
+            VRB(session, "Waiting for the \"netconf\" SSH subsystem interrupted, the Call Home thread is terminating.");
+            return 0;
+        }
+
         usleep(NC_TIMEOUT_STEP);
         if (nc_timeouttime_cur_diff(&ts_timeout) < 1) {
             /* timeout */
@@ -1761,6 +1770,11 @@ nc_accept_ssh_session_auth(struct nc_session *session, struct nc_server_ssh_opts
             return -1;
         }
 
+        if (nc_session_handshake_interrupted(session)) {
+            VRB(session, "SSH authentication interrupted, the Call Home thread is terminating.");
+            return 0;
+        }
+
         if (opts->auth_timeout) {
             time_diff = nc_timeouttime_cur_diff(&ts_timeout);
             if (time_diff < 1) {
@@ -1773,14 +1787,13 @@ nc_accept_ssh_session_auth(struct nc_session *session, struct nc_server_ssh_opts
         }
 
         /* This functions listens to the network and automatically calls callback funcitons. */
-        ret = ssh_event_dopoll(event, time_diff);
+        ret = ssh_event_dopoll(event, nc_session_handshake_poll_timeout(session, time_diff));
         if (ret == SSH_ERROR) {
             ERR(session, "Failed to poll SSH event (%s).", ssh_get_error(session->ti.libssh.session));
             return -1;
-        } else if (ret == SSH_AGAIN) {
-            /* Timeout reached */
-            break;
         }
+        /* SSH_AGAIN only means the poll timeout elapsed, which may have been shortened to notice an
+         * interrupt, so ts_timeout checked at the top of the loop is the only authority */
     }
 #else
     while (1) {
@@ -1801,6 +1814,11 @@ nc_accept_ssh_session_auth(struct nc_session *session, struct nc_server_ssh_opts
             break;
         }
 
+        if (nc_session_handshake_interrupted(session)) {
+            VRB(session, "SSH authentication interrupted, the Call Home thread is terminating.");
+            return 0;
+        }
+
         usleep(NC_TIMEOUT_STEP);
         if (opts->auth_timeout && (nc_timeouttime_cur_diff(&ts_timeout) < 1)) {
             /* timeout */
@@ -1964,6 +1982,12 @@ nc_accept_ssh_session(struct nc_session *session, struct nc_server_ssh_opts *opt
     DBG(session, "Performing SSH key exchange...");
     nc_timeouttime_get(&ts_timeout, NC_TRANSPORT_HANDSHAKE_TIMEOUT);
     while ((r = ssh_handle_key_exchange(session->ti.libssh.session)) == SSH_AGAIN) {
+        if (nc_session_handshake_interrupted(session)) {
+            VRB(session, "SSH key exchange interrupted, the Call Home thread is terminating.");
+            rc = 0;
+            goto cleanup;
+        }
+
         /* this tends to take longer */
         usleep(NC_TIMEOUT_STEP * 20);
         if (nc_timeouttime_cur_diff(&ts_timeout) < 1) {