conflux: Avoid noting a cell was sent on a closed circuit

It turns out that circuit_package_relay_cell() returns 0 in order to drop a
cell but there is a code path, if the circuit queue is full, that also silently
closes the circuit and returns 0.

This lead to Conflux thinking a cell was sent but actually the cell was not and
the circuit was closed leading to the hard assert.

And so this function makes sure that circuit_package_relay_cell() and
append_cell_to_circuit_queue() returns a value that indicate what happened with
the cell and circuit so the caller can make an informed decision with it.

This change makes it that we do NOT enter the Conflux subsystem if the cell is
not queued on the circuit.

Fixes #40921

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

diff --git a/changes/ticket40921 b/changes/ticket40921
new file mode 100644 (file)
index 0000000..5818b91
--- /dev/null
+++ b/changes/ticket40921
@@ -0,0 +1,3 @@
+  o Minor bugfixes (conflux):
+    - Avoid a potential hard assert (crash) when sending a cell on a Conflux
+      set. Fixes bug 40921; bugfix on 0.4.8.1-alpha.

diff --git a/src/core/or/circuitbuild.c b/src/core/or/circuitbuild.c
index d6e022e7fa78df7e13b6d8c9fbf8098443c3dccb..dc1912294b27eba9b07fa91be7890eddb66a82d4 100644 (file)
--- a/src/core/or/circuitbuild.c
+++ b/src/core/or/circuitbuild.c
@@ -816,8 +816,10 @@ circuit_deliver_create_cell,(circuit_t *circ,
   circuit_set_n_circid_chan(circ, id, circ->n_chan);
   cell.circ_id = circ->n_circ_id;
 
-  append_cell_to_circuit_queue(circ, circ->n_chan, &cell,
-                               CELL_DIRECTION_OUT, 0);
+  if (append_cell_to_circuit_queue(circ, circ->n_chan, &cell,
+                                   CELL_DIRECTION_OUT, 0) < 0) {
+    return -1;
+  }
 
   if (CIRCUIT_IS_ORIGIN(circ)) {
     /* Update began timestamp for circuits starting their first hop */

diff --git a/src/core/or/conflux.c b/src/core/or/conflux.c
index 677df9506777bee4a973974d157eec946a75b3ec..9025dcffaf09cc5a5ed8ead58af270cb2a7bdde2 100644 (file)
--- a/src/core/or/conflux.c
+++ b/src/core/or/conflux.c
@@ -531,7 +531,10 @@ conflux_note_cell_sent(conflux_t *cfx, circuit_t *circ, uint8_t relay_command)
   }
 
   leg = conflux_get_leg(cfx, circ);
-  tor_assert(leg);
+  if (leg == NULL) {
+    log_fn(LOG_PROTOCOL_WARN, LD_BUG, "No Conflux leg after sending a cell");
+    return;
+  }
 
   leg->last_seq_sent++;
 

diff --git a/src/core/or/relay.c b/src/core/or/relay.c
index 6abe802355e0f5ace42a74bb573d0479d138a022..8e6fddf18b3b5f7f214a9fb8b6b20d635422db9c 100644 (file)
--- a/src/core/or/relay.c
+++ b/src/core/or/relay.c
@@ -365,14 +365,19 @@ circuit_receive_relay_cell(cell_t *cell, circuit_t *circ,
                                   * we might kill the circ before we relay
                                   * the cells. */
 
-  append_cell_to_circuit_queue(circ, chan, cell, cell_direction, 0);
+  if (append_cell_to_circuit_queue(circ, chan, cell, cell_direction, 0) < 0) {
+    return -END_CIRC_REASON_RESOURCELIMIT;
+  }
   return 0;
 }
 
 /** Package a relay cell from an edge:
  *  - Encrypt it to the right layer
  *  - Append it to the appropriate cell_queue on <b>circ</b>.
- */
+ *
+ * Return 1 if the cell was successfully sent as in queued on the circuit.
+ * Return 0 if the cell needs to be dropped as in ignored.
+ * Return -1 on error for which the circuit should be marked for close. */
 MOCK_IMPL(int,
 circuit_package_relay_cell, (cell_t *cell, circuit_t *circ,
                            cell_direction_t cell_direction,
@@ -430,8 +435,8 @@ circuit_package_relay_cell, (cell_t *cell, circuit_t *circ,
   }
   ++stats_n_relay_cells_relayed;
 
-  append_cell_to_circuit_queue(circ, chan, cell, cell_direction, on_stream);
-  return 0;
+  return append_cell_to_circuit_queue(circ, chan, cell,
+                                      cell_direction, on_stream);
 }
 
 /** If cell's stream_id matches the stream_id of any conn that's
@@ -742,13 +747,24 @@ relay_send_command_from_edge_,(streamid_t stream_id, circuit_t *orig_circ,
     circuit_sent_valid_data(origin_circ, rh.length);
   }
 
-  if (circuit_package_relay_cell(&cell, circ, cell_direction, cpath_layer,
-                                 stream_id, filename, lineno) < 0) {
+  int ret = circuit_package_relay_cell(&cell, circ, cell_direction,
+                                       cpath_layer, stream_id, filename,
+                                       lineno);
+  if (ret < 0) {
     log_warn(LD_BUG,"circuit_package_relay_cell failed. Closing.");
     circuit_mark_for_close(circ, END_CIRC_REASON_INTERNAL);
     return -1;
+  } else if (ret == 0) {
+    /* This means we should drop the cell or that the circuit was already
+     * marked for close. At this point in time, we do NOT close the circuit if
+     * the cell is dropped. It is not the case with arti where each circuit
+     * protocol violation will lead to closing the circuit. */
+    return 0;
   }
 
+  /* At this point, we are certain that the cell was queued on the circuit and
+   * thus will be sent on the wire. */
+
   if (circ->conflux) {
     conflux_note_cell_sent(circ->conflux, circ, relay_command);
   }
@@ -3381,8 +3397,13 @@ relay_consensus_has_changed(const networkstatus_t *ns)
  * The given <b>cell</b> is copied onto the circuit queue so the caller must
  * cleanup the memory.
  *
- * This function is part of the fast path. */
-void
+ * This function is part of the fast path.
+ *
+ * Return 1 if the cell was successfully sent.
+ * Return 0 if the cell can not be sent. The caller MUST NOT close the circuit.
+ * Return -1 indicating an error and that the caller should mark the circuit
+ * for close. */
+int
 append_cell_to_circuit_queue(circuit_t *circ, channel_t *chan,
                              cell_t *cell, cell_direction_t direction,
                              streamid_t fromstream)
@@ -3393,8 +3414,9 @@ append_cell_to_circuit_queue(circuit_t *circ, channel_t *chan,
   int32_t max_queue_size;
   int circ_blocked;
   int exitward;
-  if (circ->marked_for_close)
-    return;
+  if (circ->marked_for_close) {
+    return 0;
+  }
 
   exitward = (direction == CELL_DIRECTION_OUT);
   if (exitward) {
@@ -3424,9 +3446,8 @@ append_cell_to_circuit_queue(circuit_t *circ, channel_t *chan,
            "Closing circuit for safety reasons.",
            (exitward) ? "Outbound" : "Inbound", queue->n,
            max_queue_size);
-    circuit_mark_for_close(circ, END_CIRC_REASON_RESOURCELIMIT);
     stats_n_circ_max_cell_reached++;
-    return;
+    return -1;
   }
 
   /* Very important that we copy to the circuit queue because all calls to
@@ -3437,8 +3458,9 @@ append_cell_to_circuit_queue(circuit_t *circ, channel_t *chan,
   /* Check and run the OOM if needed. */
   if (PREDICT_UNLIKELY(cell_queues_check_size())) {
     /* We ran the OOM handler which might have closed this circuit. */
-    if (circ->marked_for_close)
-      return;
+    if (circ->marked_for_close) {
+      return 0;
+    }
   }
 
   /* If we have too many cells on the circuit, note that it should
@@ -3462,6 +3484,7 @@ append_cell_to_circuit_queue(circuit_t *circ, channel_t *chan,
 
   /* New way: mark this as having waiting cells for the scheduler */
   scheduler_channel_has_waiting_cells(chan);
+  return 1;
 }
 
 /** Append an encoded value of <b>addr</b> to <b>payload_out</b>, which must

diff --git a/src/core/or/relay.h b/src/core/or/relay.h
index 3a1f3b0ae51881696d8613ef2568307bea476a26..12198ae9827e77a70de7be74e206f03ad4fa3bd9 100644 (file)
--- a/src/core/or/relay.h
+++ b/src/core/or/relay.h
@@ -76,9 +76,9 @@ void cell_queue_append_packed_copy(circuit_t *circ, cell_queue_t *queue,
                                    int exitward, const cell_t *cell,
                                    int wide_circ_ids, int use_stats);
 
-void append_cell_to_circuit_queue(circuit_t *circ, channel_t *chan,
-                                  cell_t *cell, cell_direction_t direction,
-                                  streamid_t fromstream);
+int append_cell_to_circuit_queue(circuit_t *circ, channel_t *chan,
+                                 cell_t *cell, cell_direction_t direction,
+                                 streamid_t fromstream);
 
 void destroy_cell_queue_init(destroy_cell_queue_t *queue);
 void destroy_cell_queue_clear(destroy_cell_queue_t *queue);

diff --git a/src/feature/relay/circuitbuild_relay.c b/src/feature/relay/circuitbuild_relay.c
index 5b1609a1af7196e9638c6764879038f600545cea..ce6cbe6df45da5ae2cd901163ae86fa3e4026877 100644 (file)
--- a/src/feature/relay/circuitbuild_relay.c
+++ b/src/feature/relay/circuitbuild_relay.c
@@ -579,8 +579,10 @@ onionskin_answer(struct or_circuit_t *circ,
 
   int used_create_fast = (created_cell->cell_type == CELL_CREATED_FAST);
 
-  append_cell_to_circuit_queue(TO_CIRCUIT(circ),
-                               circ->p_chan, &cell, CELL_DIRECTION_IN, 0);
+  if (append_cell_to_circuit_queue(TO_CIRCUIT(circ), circ->p_chan,
+                                   &cell, CELL_DIRECTION_IN, 0) < 0) {
+    return -1;
+  }
   log_debug(LD_CIRC,"Finished sending '%s' cell.",
             used_create_fast ? "created_fast" : "created");