conflux: Flag set as in full teardown in the free path

We suspect a shutdown race of some sort for which the full teardown is not
noticed during the close but should be during the free.

For that, we flag the conflux set as in full teardown (if so) in the free path
in case the close path didn't caught it.

Fixes #40870

Signed-off-by: David Goulet <dgoulet@torproject.org>

diff --git a/changes/ticket40870 b/changes/ticket40870
new file mode 100644 (file)
index 0000000..c33c83e
--- /dev/null
+++ b/changes/ticket40870
@@ -0,0 +1,4 @@
+  o Minor bugfixes (conflux, client):
+    - Avoid a non fatal assert caused by data coming in on a conflux set that is
+      being freed during shutdown. Fixes bug 40870; bugfix on 0.4.8.1-alpha.
+

diff --git a/src/app/main/shutdown.c b/src/app/main/shutdown.c
index b3f1c6d0583654b1928d72ddc22609e782e74d6d..7f0d112c90dbacd44403bdecfbcb0be5cdc755b2 100644 (file)
--- a/src/app/main/shutdown.c
+++ b/src/app/main/shutdown.c
@@ -120,6 +120,7 @@ tor_free_all(int postfork)
   dirserv_free_all();
   rep_hist_free_all();
   bwhist_free_all();
+  conflux_notify_shutdown();
   circuit_free_all();
   conflux_pool_free_all();
   circpad_machines_free();

diff --git a/src/core/or/conflux_pool.c b/src/core/or/conflux_pool.c
index 4a7e941372bdc5f8ed82d9b25ebab0c1113e4c0b..ec2938aefc7eca1faeeaeaa87f98076f1c097048 100644 (file)
--- a/src/core/or/conflux_pool.c
+++ b/src/core/or/conflux_pool.c
@@ -1607,7 +1607,22 @@ linked_circuit_free(circuit_t *circ, bool is_client)
 
   /* Circuit can be freed without being closed and so we try to delete this leg
    * so we can learn if this circuit is the last leg or not. */
-  cfx_del_leg(circ->conflux, circ);
+  if (cfx_del_leg(circ->conflux, circ)) {
+    /* Check for instances of bug #40870, which we suspect happen
+     * during exit. If any happen outside of exit, BUG and warn. */
+    if (!circ->conflux->in_full_teardown) {
+      /* We should bug and warn if we're not in a shutdown process; that
+       * means we got here somehow without a close. */
+      if (BUG(!shutting_down)) {
+        log_warn(LD_BUG,
+                 "Conflux circuit %p being freed without being marked for "
+                 "full teardown via close, with shutdown state %d. "
+                 "Please report this.", circ, shutting_down);
+        conflux_log_set(LOG_WARN, circ->conflux, is_client);
+      }
+      circ->conflux->in_full_teardown = true;
+    }
+  }
 
   if (CONFLUX_NUM_LEGS(circ->conflux) > 0) {
     /* The last leg will free the streams but until then, we nullify to avoid
@@ -2126,14 +2141,36 @@ conflux_log_set(int loglevel, const conflux_t *cfx, bool is_client)
   }
 }
 
+/**
+ * Conflux needs a notification when tor_shutdown() begins, so that
+ * when circuits are freed, new legs are not launched.
+ *
+ * This needs a separate notification from conflux_pool_free_all(),
+ * because circuits must be freed before that function.
+ */
+void
+conflux_notify_shutdown(void)
+{
+  shutting_down = true;
+}
+
+#ifdef TOR_UNIT_TESTS
+/**
+ * For unit tests: Clear the shutting down state so we resume building legs.
+ */
+void
+conflux_clear_shutdown(void)
+{
+  shutting_down = false;
+}
+#endif
+
 /** Free and clean up the conflux pool subsystem. This is called by the subsys
  * manager AFTER all circuits have been freed which implies that all objects in
  * the pools aren't referenced anymore. */
 void
 conflux_pool_free_all(void)
 {
-  shutting_down = true;
-
   digest256map_free(client_linked_pool, free_conflux_void_);
   digest256map_free(server_linked_pool, free_conflux_void_);
   digest256map_free(client_unlinked_pool, free_unlinked_void_);

diff --git a/src/core/or/conflux_pool.h b/src/core/or/conflux_pool.h
index afa4d9d0588a7180c83f6ea54e21eb072e585f53..eba726b03a1c14a818eb9f020813c2d4aea5bae5 100644 (file)
--- a/src/core/or/conflux_pool.h
+++ b/src/core/or/conflux_pool.h
@@ -12,6 +12,7 @@
 #include "core/or/or.h"
 
 void conflux_pool_init(void);
+void conflux_notify_shutdown(void);
 void conflux_pool_free_all(void);
 
 origin_circuit_t *conflux_get_circ_for_conn(const entry_connection_t *conn,
@@ -41,6 +42,7 @@ void conflux_log_set(int loglevel, const conflux_t *cfx, bool is_client);
 
 #ifdef TOR_UNIT_TESTS
 bool launch_new_set(int num_legs);
+void conflux_clear_shutdown(void);
 digest256map_t *get_linked_pool(bool is_client);
 digest256map_t *get_unlinked_pool(bool is_client);
 extern uint8_t DEFAULT_CLIENT_UX;

diff --git a/src/test/test_conflux_pool.c b/src/test/test_conflux_pool.c
index fc306773773aac8c0cf31cb1af4f07541bd01e31..05dc7b14ff98b3542f8cad085a42647e2251d7db 100644 (file)
--- a/src/test/test_conflux_pool.c
+++ b/src/test/test_conflux_pool.c
@@ -396,6 +396,7 @@ test_setup(void)
 static void
 test_clear_circs(void)
 {
+  conflux_notify_shutdown();
   SMARTLIST_FOREACH(circ_pairs, circ_pair_t *, circ_pair, {
     tor_free(circ_pair);
   });
@@ -430,6 +431,9 @@ test_clear_circs(void)
   tor_assert(smartlist_len(mock_cell_delivery) == 0);
 
   (void)free_fake_origin_circuit;
+
+  /* Clear shutdown flag so we can resume testing again. */
+  conflux_clear_shutdown();
 }
 
 static void