[tor-commits] [tor/maint-0.3.4] Ticket #25573: Count TRUNCATED cells.

[nickm at torproject.org]

commit ce894e20b597d2d21b56ac8a8f13d1ea4063731d
Author: Mike Perry <mikeperry-git at torproject.org>
Date:   Tue Aug 7 04:23:33 2018 +0000

    Ticket #25573: Count TRUNCATED cells.
    
    TRUNCATED cells were ignored while in path bias. Now they are obeyed, and
    cause us to tear down the circuit. The actual impact is minimal, since we
    would just wait around for a probe that would never arrive before.
    
    This commit changes client behavior.
---
 src/or/circpathbias.c     | 10 ++++++++++
 src/or/circuitbuild.c     |  3 +--
 src/or/circuitbuild.h     |  3 +--
 src/or/relay.c            |  9 ++++++++-
 src/test/test_relaycell.c | 18 ++++++++++++++++++
 5 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/src/or/circpathbias.c b/src/or/circpathbias.c
index 923941e5b..3cdd16261 100644
--- a/src/or/circpathbias.c
+++ b/src/or/circpathbias.c
@@ -929,6 +929,16 @@ pathbias_count_valid_cells(circuit_t *circ, const cell_t *cell)
   /* Check to see if this is a cell from a previous connection,
    * or is a request to close the circuit. */
   switch (rh.command) {
+    case RELAY_COMMAND_TRUNCATED:
+      /* Truncated cells can arrive on path bias circs. When they do,
+       * just process them. This closes the circ, but it was junk anyway.
+       * No reason to wait for the probe. */
+      circuit_read_valid_data(ocirc, rh.length);
+      circuit_truncated(TO_ORIGIN_CIRCUIT(circ),
+                        get_uint8(cell->payload + RELAY_HEADER_SIZE));
+
+      break;
+
     case RELAY_COMMAND_END:
       if (connection_half_edge_is_valid_end(ocirc->half_streams,
                                              rh.stream_id)) {
diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c
index 3d1c9c1ab..8f17f2786 100644
--- a/src/or/circuitbuild.c
+++ b/src/or/circuitbuild.c
@@ -1419,13 +1419,12 @@ circuit_finish_handshake(origin_circuit_t *circ,
  * just give up: force circ to close, and return 0.
  */
 int
-circuit_truncated(origin_circuit_t *circ, crypt_path_t *layer, int reason)
+circuit_truncated(origin_circuit_t *circ, int reason)
 {
 //  crypt_path_t *victim;
 //  connection_t *stream;
 
   tor_assert(circ);
-  tor_assert(layer);
 
   /* XXX Since we don't send truncates currently, getting a truncated
    *     means that a connection broke or an extend failed. For now,
diff --git a/src/or/circuitbuild.h b/src/or/circuitbuild.h
index 0184898e2..e3d30c110 100644
--- a/src/or/circuitbuild.h
+++ b/src/or/circuitbuild.h
@@ -37,8 +37,7 @@ int circuit_init_cpath_crypto(crypt_path_t *cpath,
 struct created_cell_t;
 int circuit_finish_handshake(origin_circuit_t *circ,
                              const struct created_cell_t *created_cell);
-int circuit_truncated(origin_circuit_t *circ, crypt_path_t *layer,
-                      int reason);
+int circuit_truncated(origin_circuit_t *circ, int reason);
 int onionskin_answer(or_circuit_t *circ,
                      const struct created_cell_t *created_cell,
                      const char *keys, size_t keys_len,
diff --git a/src/or/relay.c b/src/or/relay.c
index 13f2b56bc..81bb94d5a 100644
--- a/src/or/relay.c
+++ b/src/or/relay.c
@@ -1748,7 +1748,14 @@ connection_edge_process_relay_cell(cell_t *cell, circuit_t *circ,
                "'truncated' unsupported at non-origin. Dropping.");
         return 0;
       }
-      circuit_truncated(TO_ORIGIN_CIRCUIT(circ), layer_hint,
+
+      /* Count the truncated as valid, for completeness. The
+       * circuit is being torn down anyway, though.  */
+      if (CIRCUIT_IS_ORIGIN(circ)) {
+        circuit_read_valid_data(TO_ORIGIN_CIRCUIT(circ),
+                                rh.length);
+      }
+      circuit_truncated(TO_ORIGIN_CIRCUIT(circ),
                         get_uint8(cell->payload + RELAY_HEADER_SIZE));
       return 0;
     case RELAY_COMMAND_CONNECTED:
diff --git a/src/test/test_relaycell.c b/src/test/test_relaycell.c
index 4c406a9b7..3f84ee830 100644
--- a/src/test/test_relaycell.c
+++ b/src/test/test_relaycell.c
@@ -116,6 +116,16 @@ mock_connection_mark_unattached_ap_(entry_connection_t *conn, int endreason,
 }
 
 static void
+mock_mark_circ_for_close(circuit_t *circ, int reason, int line,
+                          const char *file)
+{
+  (void)reason; (void)line; (void)file;
+
+  circ->marked_for_close = 1;
+  return;
+}
+
+static void
 mock_mark_for_close(connection_t *conn,
                         int line, const char *file)
 {
@@ -694,6 +704,7 @@ test_circbw_relay(void *arg)
   MOCK(connection_start_reading, mock_start_reading);
   MOCK(connection_mark_for_close_internal_, mock_mark_for_close);
   MOCK(relay_send_command_from_edge_, mock_send_command);
+  MOCK(circuit_mark_for_close_, mock_mark_circ_for_close);
 
   circ = helper_create_origin_circuit(CIRCUIT_PURPOSE_C_GENERAL, 0);
   circ->cpath->state = CPATH_STATE_AWAITING_KEYS;
@@ -856,11 +867,18 @@ test_circbw_relay(void *arg)
   if (!subtest_circbw_halfclosed(circ, 6))
     goto done;
 
+  /* Path bias: truncated */
+  tt_int_op(circ->base_.marked_for_close, OP_EQ, 0);
+  PACK_CELL(0, RELAY_COMMAND_TRUNCATED, "Data1234");
+  pathbias_count_valid_cells(circ, &cell);
+  tt_int_op(circ->base_.marked_for_close, OP_EQ, 1);
+
  done:
   UNMOCK(connection_start_reading);
   UNMOCK(connection_mark_unattached_ap_);
   UNMOCK(connection_mark_for_close_internal_);
   UNMOCK(relay_send_command_from_edge_);
+  UNMOCK(circuit_mark_for_close_);
   circuit_free_(TO_CIRCUIT(circ));
   connection_free_minimal(ENTRY_TO_CONN(entryconn1));
 }