Browse Source

Fixed two memory / race condition bugs

(1) Was casting the event listener context to the wrong type
(2) Was using cells after we gave up ownership of them
Steven Engler 4 years ago
parent
commit
8ff491b734
1 changed files with 11 additions and 5 deletions
  1. 11 5
      src/core/or/safe_connection.c

+ 11 - 5
src/core/or/safe_connection.c

@@ -645,7 +645,7 @@ static void
 safe_or_conn_link_protocol_version_cb(event_label_t label, event_data_t data,
                                       void *context)
 {
-  safe_or_connection_t *safe_or_conn = context;
+  safe_or_connection_t *safe_or_conn = TO_SAFE_OR_CONN(context);
   tor_assert(label == or_conn_link_protocol_version_ev);
   tor_assert(safe_or_conn != NULL);
   tor_mutex_acquire(&TO_SAFE_CONN(safe_or_conn)->lock);
@@ -670,7 +670,7 @@ safe_or_conn_open_cb(event_label_t label, event_data_t data, void *context)
 {
   (void)data;
 
-  safe_or_connection_t *safe_or_conn = context;
+  safe_or_connection_t *safe_or_conn = TO_SAFE_OR_CONN(context);
   tor_assert(label == or_conn_open_ev);
   tor_assert(safe_or_conn != NULL);
   tor_mutex_acquire(&TO_SAFE_CONN(safe_or_conn)->lock);
@@ -1584,7 +1584,7 @@ static void
 safe_or_conn_outgoing_cell_cb(event_label_t label, event_data_t data,
                               void *context)
 {
-  safe_or_connection_t *safe_or_conn = context;
+  safe_or_connection_t *safe_or_conn = TO_SAFE_OR_CONN(context);
   tor_assert(safe_or_conn != NULL);
   tor_mutex_acquire(&TO_SAFE_CONN(safe_or_conn)->lock);
 
@@ -1691,6 +1691,7 @@ static void
 process_cells_from_inbuf(safe_or_connection_t *safe_or_conn)
 {
   tor_assert(safe_or_conn != NULL);
+  tor_assert(safe_or_conn->waiting_for_link_protocol == false);
 
   while (true) {
     var_cell_t *var_cell = NULL;
@@ -1702,13 +1703,16 @@ process_cells_from_inbuf(safe_or_connection_t *safe_or_conn)
         return;
       }
 
+      uint8_t command = var_cell->command;
+
       event_data_t event_data = { .ptr = var_cell };
       event_source_publish(TO_SAFE_CONN(safe_or_conn)->event_source,
                            safe_or_conn_var_cell_ev, event_data,
                            void_var_cell_free);
 
-      if (safe_or_conn->link_protocol == 0 &&
-          var_cell->command == CELL_VERSIONS) {
+      // we no longer own the var cell at this point, so don't access it again
+
+      if (safe_or_conn->link_protocol == 0 && command == CELL_VERSIONS) {
         // this is the first VERSIONS cell we've received;
         // in order to process future cells, we need to be told our
         // protocol version
@@ -1729,6 +1733,8 @@ process_cells_from_inbuf(safe_or_connection_t *safe_or_conn)
         event_source_publish(TO_SAFE_CONN(safe_or_conn)->event_source,
                              safe_or_conn_fixed_cell_ev, event_data,
                              tor_free_);
+
+        // we no longer own the cell at this point, so don't access it again
       } else {
         // there is not yet a complete cell
         return;