Merge pull request #2753 from qlyoung/fix-zebra-shutdown-crash-2

Fix zebra shutdown crash: Round 3

diff --git a/lib/frr_pthread.c b/lib/frr_pthread.c
index 00681b9db86167b85d17cd843f75c03812a48fbc..f5a6383f48d9ade8a100695dd14979cf307b40ed 100644 (file)
--- a/lib/frr_pthread.c
+++ b/lib/frr_pthread.c
@@ -273,7 +273,33 @@ static int fpt_halt(struct frr_pthread *fpt, void **res)
        return 0;
 }
 
-/* entry pthread function & main event loop */
+/*
+ * Entry pthread function & main event loop.
+ *
+ * Upon thread start the following actions occur:
+ *
+ * - frr_pthread's owner field is set to pthread ID.
+ * - All signals are blocked (except for unblockable signals).
+ * - Pthread's threadmaster is set to never handle pending signals
+ * - Poker pipe for poll() is created and queued as I/O source
+ * - The frr_pthread->running_cond condition variable is signalled to indicate
+ *   that the previous actions have completed. It is not safe to assume any of
+ *   the above have occurred before receiving this signal.
+ *
+ * After initialization is completed, the event loop begins running. Each tick,
+ * the following actions are performed before running the usual event system
+ * tick function:
+ *
+ * - Verify that the running boolean is set
+ * - Verify that there are no pending cancellation requests
+ * - Verify that there are tasks scheduled
+ *
+ * So long as the conditions are met, the event loop tick is run and the
+ * returned task is executed.
+ *
+ * If any of these conditions are not met, the event loop exits, closes the
+ * pipes and dies without running any cleanup functions.
+ */
 static void *fpt_run(void *arg)
 {
        struct frr_pthread *fpt = arg;
@@ -289,6 +315,7 @@ static void *fpt_run(void *arg)
 
        struct thread task;
        while (atomic_load_explicit(&fpt->running, memory_order_relaxed)) {
+               pthread_testcancel();
                if (thread_fetch(fpt->master, &task)) {
                        thread_call(&task);
                }

diff --git a/zebra/main.c b/zebra/main.c
index 0b8bc6a0a542266570c9e7da70729f6666f10cd7..4eeba8549aa13c8c67a69e8c3536a27fee9748f6 100644 (file)
--- a/zebra/main.c
+++ b/zebra/main.c
@@ -138,11 +138,16 @@ static void sigint(void)
 {
        struct vrf *vrf;
        struct zebra_vrf *zvrf;
+       struct listnode *ln, *nn;
+       struct zserv *client;
 
        zlog_notice("Terminating on signal");
 
        frr_early_fini();
 
+       for (ALL_LIST_ELEMENTS(zebrad.client_list, ln, nn, client))
+               zserv_close_client(client);
+
        list_delete_all_node(zebrad.client_list);
        zebra_ptm_finish();
 

diff --git a/zebra/zserv.c b/zebra/zserv.c
index 725cc9229c8e16e551b718870495742a56692841..fa501b187d3ac7e328715e9a6119e5a9672275ba 100644 (file)
--- a/zebra/zserv.c
+++ b/zebra/zserv.c
@@ -91,7 +91,7 @@ enum zserv_event {
        /* The calling client has packets on its input buffer */
        ZSERV_PROCESS_MESSAGES,
        /* The calling client wishes to be killed */
-       ZSERV_HANDLE_CLOSE,
+       ZSERV_HANDLE_CLIENT_FAIL,
 };
 
 /*
@@ -160,18 +160,25 @@ static void zserv_log_message(const char *errmsg, struct stream *msg,
 /*
  * Gracefully shut down a client connection.
  *
- * Cancel any pending tasks for the client's thread. Then schedule a task on the
- * main thread to shut down the calling thread.
+ * Cancel any pending tasks for the client's thread. Then schedule a task on
+ * the main thread to shut down the calling thread.
  *
  * Must be called from the client pthread, never the main thread.
  */
-static void zserv_client_close(struct zserv *client)
+static void zserv_client_fail(struct zserv *client)
 {
+       zlog_warn("Client '%s' encountered an error and is shutting down.",
+                 zebra_route_string(client->proto));
+
        atomic_store_explicit(&client->pthread->running, false,
-                             memory_order_seq_cst);
+                             memory_order_relaxed);
+       if (client->sock > 0) {
+               close(client->sock);
+               client->sock = -1;
+       }
        THREAD_OFF(client->t_read);
        THREAD_OFF(client->t_write);
-       zserv_event(client, ZSERV_HANDLE_CLOSE);
+       zserv_event(client, ZSERV_HANDLE_CLIENT_FAIL);
 }
 
 /*
@@ -264,7 +271,7 @@ static int zserv_write(struct thread *thread)
 zwrite_fail:
        zlog_warn("%s: could not write to %s [fd = %d], closing.", __func__,
                  zebra_route_string(client->proto), client->sock);
-       zserv_client_close(client);
+       zserv_client_fail(client);
        return 0;
 }
 
@@ -438,7 +445,7 @@ static int zserv_read(struct thread *thread)
 
 zread_fail:
        stream_fifo_free(cache);
-       zserv_client_close(client);
+       zserv_client_fail(client);
        return -1;
 }
 
@@ -605,28 +612,46 @@ static void zserv_client_free(struct zserv *client)
        XFREE(MTYPE_TMP, client);
 }
 
-/*
- * Finish closing a client.
- *
- * This task is scheduled by a ZAPI client pthread on the main pthread when it
- * wants to stop itself. When this executes, the client connection should
- * already have been closed. This task's responsibility is to gracefully
- * terminate the client thread, update relevant internal datastructures and
- * free any resources allocated by the main thread.
- */
-static int zserv_handle_client_close(struct thread *thread)
+void zserv_close_client(struct zserv *client)
 {
-       struct zserv *client = THREAD_ARG(thread);
-
-       /* synchronously stop thread */
+       /* synchronously stop and join pthread */
        frr_pthread_stop(client->pthread, NULL);
 
-       /* destroy frr_pthread */
+       if (IS_ZEBRA_DEBUG_EVENT)
+               zlog_debug("Closing client '%s'",
+                          zebra_route_string(client->proto));
+
+       /* if file descriptor is still open, close it */
+       if (client->sock > 0) {
+               close(client->sock);
+               client->sock = -1;
+       }
+
+       thread_cancel_event(zebrad.master, client);
+       THREAD_OFF(client->t_cleanup);
+
+       /* destroy pthread */
        frr_pthread_destroy(client->pthread);
        client->pthread = NULL;
 
+       /* remove from client list */
        listnode_delete(zebrad.client_list, client);
+
+       /* delete client */
        zserv_client_free(client);
+}
+
+/*
+ * This task is scheduled by a ZAPI client pthread on the main pthread when it
+ * wants to stop itself. When this executes, the client connection should
+ * already have been closed and the thread will most likely have died, but its
+ * resources still need to be cleaned up.
+ */
+static int zserv_handle_client_fail(struct thread *thread)
+{
+       struct zserv *client = THREAD_ARG(thread);
+
+       zserv_close_client(client);
        return 0;
 }
 
@@ -814,9 +839,9 @@ void zserv_event(struct zserv *client, enum zserv_event event)
                thread_add_event(zebrad.master, zserv_process_messages, client,
                                 0, NULL);
                break;
-       case ZSERV_HANDLE_CLOSE:
-               thread_add_event(zebrad.master, zserv_handle_client_close,
-                                client, 0, NULL);
+       case ZSERV_HANDLE_CLIENT_FAIL:
+               thread_add_event(zebrad.master, zserv_handle_client_fail,
+                                client, 0, &client->t_cleanup);
        }
 }
 
@@ -1037,7 +1062,6 @@ void zserv_init(void)
 {
        /* Client list init. */
        zebrad.client_list = list_new();
-       zebrad.client_list->del = (void (*)(void *)) zserv_client_free;
 
        /* Misc init. */
        zebrad.sock = -1;

diff --git a/zebra/zserv.h b/zebra/zserv.h
index 78cc200fa8c4576aff24a4669ebba1e1143e44ac..aaefd78eeab76bb3b1b897595c3cbeaf3edd0f9d 100644 (file)
--- a/zebra/zserv.h
+++ b/zebra/zserv.h
@@ -73,6 +73,9 @@ struct zserv {
        struct thread *t_read;
        struct thread *t_write;
 
+       /* Threads for the main pthread */
+       struct thread *t_cleanup;
+
        /* default routing table this client munges */
        int rtm_table;
 
@@ -232,6 +235,18 @@ extern int zserv_send_message(struct zserv *client, struct stream *msg);
  */
 extern struct zserv *zserv_find_client(uint8_t proto, unsigned short instance);
 
+
+/*
+ * Close a client.
+ *
+ * Kills a client's thread, removes the client from the client list and cleans
+ * up its resources.
+ *
+ * client
+ *    the client to close
+ */
+extern void zserv_close_client(struct zserv *client);
+
 #if defined(HANDLE_ZAPI_FUZZING)
 extern void zserv_read_file(char *input);
 #endif