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>

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.

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 */

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++;
 

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

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);

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");