chan: Do not re-queue after a fail cell write

Couple things happen in this commit. First, we do not re-queue a cell back in
the circuit queue if the write packed cell failed. Currently, it is close to
impossible to have it failed but just in case, the channel is mark as closed
and we move on.

The second thing is that the channel_write_packed_cell() always took ownership
of the cell whatever the outcome. This means, on success or failure, it needs
to free it.

It turns out that that we were using the wrong free function in one case and
not freeing it in an other possible code path. So, this commit makes sure we
only free it in one place that is at the very end of
channel_write_packed_cell() which is the top layer of the channel abstraction.
This makes also channel_tls_write_packed_cell_method() return a negative value
on error.

Two unit tests had to be fixed (quite trivial) due to a double free of the
packed cell in the test since now we do free it in all cases correctly.

Part of #23709

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

4 files changed, 32 insertions, 29 deletions

diff --git a/src/or/channel.c b/src/or/channel.c
index 94d7af47b5..9e79fc1a3f 100644
--- a/src/or/channel.c
+++ b/src/or/channel.c
@@ -1431,8 +1431,11 @@ channel_clear_remote_end(channel_t *chan)
 /**
  * Write to a channel the given packed cell.
  *
- * Return 0 on success and the cell is freed so the caller MUST discard any
- * reference to it. On error, -1 is returned and the cell is untouched.
+ * Return 0 on success or -1 on error.
+ *
+ * Two possible errors can happen. Either the channel is not opened or the
+ * lower layer (specialized channel) failed to write it. In both cases, it is
+ * the caller responsability to free the cell.
  */
 static int
 write_packed_cell(channel_t *chan, packed_cell_t *cell)
@@ -1483,10 +1486,15 @@ write_packed_cell(channel_t *chan, packed_cell_t *cell)
  * Write a packed cell to a channel using the write_cell() method.  This is
  * called by the transport-independent code to deliver a packed cell to a
  * channel for transmission.
+ *
+ * Return 0 on success else a negative value. In both cases, the caller should
+ * not access the cell anymore, it is freed both on success and error.
  */
 int
 channel_write_packed_cell(channel_t *chan, packed_cell_t *cell)
 {
+  int ret = -1;
+
   tor_assert(chan);
   tor_assert(cell);
 
@@ -1494,14 +1502,20 @@ channel_write_packed_cell(channel_t *chan, packed_cell_t *cell)
     log_debug(LD_CHANNEL, "Discarding %p on closing channel %p with "
               "global ID "U64_FORMAT, cell, chan,
               U64_PRINTF_ARG(chan->global_identifier));
-    tor_free(cell);
-    return -1;
+    goto end;
   }
   log_debug(LD_CHANNEL,
             "Writing %p to channel %p with global ID "
             U64_FORMAT, cell, chan, U64_PRINTF_ARG(chan->global_identifier));
 
-  return write_packed_cell(chan, cell);
+  ret = write_packed_cell(chan, cell);
+
+ end:
+  /* Whatever happens, we free the cell. Either an error occured or the cell
+   * was put on the connection outbuf, both cases we have ownership of the
+   * cell and we free it. */
+  packed_cell_free(cell);
+  return ret;
 }
 
 /**
diff --git a/src/or/channeltls.c b/src/or/channeltls.c
index e6ecc15381..023ccdefd3 100644
--- a/src/or/channeltls.c
+++ b/src/or/channeltls.c
@@ -832,6 +832,9 @@ channel_tls_write_cell_method(channel_t *chan, cell_t *cell)
  *
  * This implements the write_packed_cell method for channel_tls_t; given a
  * channel_tls_t and a packed_cell_t, transmit the packed_cell_t.
+ *
+ * Return 0 on success or negative value on error. The caller must free the
+ * packed cell.
  */
 
 static int
@@ -841,7 +844,6 @@ channel_tls_write_packed_cell_method(channel_t *chan,
   tor_assert(chan);
   channel_tls_t *tlschan = BASE_CHAN_TO_TLS(chan);
   size_t cell_network_size = get_cell_network_size(chan->wide_circ_ids);
-  int written = 0;
 
   tor_assert(tlschan);
   tor_assert(packed_cell);
@@ -849,18 +851,15 @@ channel_tls_write_packed_cell_method(channel_t *chan,
   if (tlschan->conn) {
     connection_buf_add(packed_cell->body, cell_network_size,
                             TO_CONN(tlschan->conn));
-
-    /* This is where the cell is finished; used to be done from relay.c */
-    packed_cell_free(packed_cell);
-    ++written;
   } else {
     log_info(LD_CHANNEL,
              "something called write_packed_cell on a tlschan "
              "(%p with ID " U64_FORMAT " but no conn",
              chan, U64_PRINTF_ARG(chan->global_identifier));
+    return -1;
   }
 
-  return written;
+  return 0;
 }
 
 /**
diff --git a/src/or/relay.c b/src/or/relay.c
index f36fc78cda..ac91032844 100644
--- a/src/or/relay.c
+++ b/src/or/relay.c
@@ -2521,17 +2521,6 @@ cell_queue_pop(cell_queue_t *queue)
   return cell;
 }
 
-/** Insert <b>cell</b> as the head of the <b>queue</b>. */
-static void
-cell_insert_head(cell_queue_t *queue, packed_cell_t *cell)
-{
-  tor_assert(queue);
-  tor_assert(cell);
-
-  TOR_SIMPLEQ_INSERT_HEAD(&queue->head, cell, next);
-  ++queue->n;
-}
-
 /** Return the total number of bytes used for each packed_cell in a queue.
  * Approximate. */
 size_t
@@ -2757,8 +2746,11 @@ channel_flush_from_first_active_circuit, (channel_t *chan, int max))
       /* this code is duplicated from some of the logic below. Ugly! XXXX */
       tor_assert(destroy_queue->n > 0);
       cell = cell_queue_pop(destroy_queue);
+      /* Send the DESTROY cell. It is very unlikely that this fails but just
+       * in case, get rid of the channel. */
       if (channel_write_packed_cell(chan, cell) < 0) {
-        cell_insert_head(destroy_queue, cell);
+        /* The cell has been freed. */
+        channel_mark_for_close(chan);
         continue;
       }
       /* Update the cmux destroy counter */
@@ -2836,11 +2828,11 @@ channel_flush_from_first_active_circuit, (channel_t *chan, int max))
                                 DIRREQ_TUNNELED,
                                 DIRREQ_CIRC_QUEUE_FLUSHED);
 
-    /* Now send the cell */
+    /* Now send the cell. It is very unlikely that this fails but just in
+     * case, get rid of the channel. */
     if (channel_write_packed_cell(chan, cell) < 0) {
-      /* Unable to send the cell, put it back at the start of the circuit
-       * queue so we can retry. */
-      cell_insert_head(queue, cell);
+      /* The cell has been freed at this point. */
+      channel_mark_for_close(chan);
       continue;
     }
     cell = NULL;
diff --git a/src/test/test_channel.c b/src/test/test_channel.c
index 88a13862bd..c3b99d87ac 100644
--- a/src/test/test_channel.c
+++ b/src/test/test_channel.c
@@ -199,7 +199,6 @@ chan_test_write_packed_cell(channel_t *ch,
 
   if (test_chan_accept_cells) {
     /* Free the cell and bump the counter */
-    packed_cell_free(packed_cell);
     ++test_cells_written;
     rv = 1;
   }
@@ -846,7 +845,6 @@ test_channel_lifecycle(void *arg)
  done:
   free_fake_channel(ch1);
   free_fake_channel(ch2);
-  tor_free(p_cell);
 
   UNMOCK(scheduler_channel_doesnt_want_writes);
   UNMOCK(scheduler_release_channel);