sendme: Refactor SENDME cell processing

This is a bit of a complicated commit. It moves code but also refactors part
of it. No behavior change, the idea is to split things up so we can better
handle and understand how SENDME cells are processed where ultimately it will
be easier to handle authenticated SENDMEs (prop289) using the intermediate
functions added in this commit.

The entry point for the cell arriving at the edge (Client or Exit), is
connection_edge_process_relay_cell() for which we look if it is a circuit or
stream level SENDME. This commit refactors that part where two new functions
are introduced to process each of the SENDME types.

The sendme_process_circuit_level() has basically two code paths. If we are a
Client (the circuit is origin) or we are an Exit. Depending on which, the
package window is updated accordingly. Then finally, we resume the reading on
every edge streams on the circuit.

The sendme_process_stream_level() applies on the edge connection which will
update the package window if needed and then will try to empty the inbuf if
need be because we can now deliver more cells.

Again, no behavior change but in order to split that code properly into their
own functions and outside the relay.c file, code modification was needed.

Part of #26840.

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

src/core/or/relay.c

@@ -1807,87 +1807,52 @@ connection_edge_process_relay_cell(cell_t *cell, circuit_t *circ,
                (unsigned)circ->n_circ_id, rh.stream_id);
       return 0;
     case RELAY_COMMAND_SENDME:
+    {
+      int ret;
+
       if (!rh.stream_id) {
-        if (layer_hint) {
+        /* Circuit level SENDME cell. */
-          if (layer_hint->package_window + CIRCWINDOW_INCREMENT >
+        ret = sendme_process_circuit_level(layer_hint, circ, rh.length);
-                CIRCWINDOW_START_MAX) {
+        if (ret < 0) {
-            static struct ratelim_t exit_warn_ratelim = RATELIM_INIT(600);
+          return ret;
-            log_fn_ratelim(&exit_warn_ratelim, LOG_WARN, LD_PROTOCOL,
-                   "Unexpected sendme cell from exit relay. "
-                   "Closing circ.");
-            return -END_CIRC_REASON_TORPROTOCOL;
         }
-          layer_hint->package_window += CIRCWINDOW_INCREMENT;
+        /* Resume reading on any streams now that we've processed a valid
-          log_debug(LD_APP,"circ-level sendme at origin, packagewindow %d.",
+         * SENDME cell that updated our package window. */
-                    layer_hint->package_window);
         circuit_resume_edge_reading(circ, layer_hint);
-
+        /* We are done, the rest of the code is for the stream level. */
-          /* We count circuit-level sendme's as valid delivered data because
-           * they are rate limited.
-           */
-          if (CIRCUIT_IS_ORIGIN(circ)) {
-            circuit_read_valid_data(TO_ORIGIN_CIRCUIT(circ),
-                                    rh.length);
-          }
-
-        } else {
-          if (circ->package_window + CIRCWINDOW_INCREMENT >
-                CIRCWINDOW_START_MAX) {
-            static struct ratelim_t client_warn_ratelim = RATELIM_INIT(600);
-            log_fn_ratelim(&client_warn_ratelim,LOG_PROTOCOL_WARN, LD_PROTOCOL,
-                   "Unexpected sendme cell from client. "
-                   "Closing circ (window %d).",
-                   circ->package_window);
-            return -END_CIRC_REASON_TORPROTOCOL;
-          }
-          circ->package_window += CIRCWINDOW_INCREMENT;
-          log_debug(LD_APP,
-                    "circ-level sendme at non-origin, packagewindow %d.",
-                    circ->package_window);
-          circuit_resume_edge_reading(circ, layer_hint);
-        }
         return 0;
       }
+
+      /* No connection, might be half edge state. We are done if so. */
       if (!conn) {
         if (CIRCUIT_IS_ORIGIN(circ)) {
           origin_circuit_t *ocirc = TO_ORIGIN_CIRCUIT(circ);
           if (connection_half_edge_is_valid_sendme(ocirc->half_streams,
                                                    rh.stream_id)) {
             circuit_read_valid_data(ocirc, rh.length);
-            log_info(domain,
+            log_info(domain, "Sendme cell on circ %u valid on half-closed "
-                    "sendme cell on circ %u valid on half-closed "
+                             "stream id %d",
-                    "stream id %d", ocirc->global_identifier, rh.stream_id);
+                     ocirc->global_identifier, rh.stream_id);
           }
         }
 
-        log_info(domain,"sendme cell dropped, unknown stream (streamid %d).",
+        log_info(domain, "SENDME cell dropped, unknown stream (streamid %d).",
                  rh.stream_id);
         return 0;
       }
 
-      /* Don't allow the other endpoint to request more than our maximum
+      /* Stream level SENDME cell. */
-       * (i.e. initial) stream SENDME window worth of data. Well-behaved
+      ret = sendme_process_stream_level(conn, circ, rh.length);
-       * stock clients will not request more than this max (as per the check
+      if (ret < 0) {
-       * in the while loop of sendme_connection_edge_consider_sending()).
+        /* Means we need to close the circuit with reason ret. */
-       */
+        return ret;
-      if (conn->package_window + STREAMWINDOW_INCREMENT >
-          STREAMWINDOW_START_MAX) {
-        static struct ratelim_t stream_warn_ratelim = RATELIM_INIT(600);
-        log_fn_ratelim(&stream_warn_ratelim, LOG_PROTOCOL_WARN, LD_PROTOCOL,
-               "Unexpected stream sendme cell. Closing circ (window %d).",
-               conn->package_window);
-        return -END_CIRC_REASON_TORPROTOCOL;
       }
 
-      /* At this point, the stream sendme is valid */
+      /* We've now processed properly a SENDME cell, all windows have been
-      if (CIRCUIT_IS_ORIGIN(circ)) {
+       * properly updated, we'll read on the edge connection to see if we can
-        circuit_read_valid_data(TO_ORIGIN_CIRCUIT(circ),
+       * get data out towards the end point (Exit or client) since we are now
-                                rh.length);
+       * allowed to deliver more cells. */
-      }
 
-      conn->package_window += STREAMWINDOW_INCREMENT;
-      log_debug(domain,"stream-level sendme, packagewindow now %d.",
-                conn->package_window);
       if (circuit_queue_streams_are_blocked(circ)) {
         /* Still waiting for queue to flush; don't touch conn */
         return 0;
@@ -1900,6 +1865,7 @@ connection_edge_process_relay_cell(cell_t *cell, circuit_t *circ,
         return 0;
       }
       return 0;
+    }
     case RELAY_COMMAND_RESOLVE:
       if (layer_hint) {
         log_fn(LOG_PROTOCOL_WARN, LD_APP,

src/core/or/sendme.c

@@ -9,8 +9,10 @@
 
 #include "core/or/or.h"
 
+#include "app/config/config.h"
 #include "core/mainloop/connection.h"
 #include "core/or/circuitlist.h"
+#include "core/or/circuituse.h"
 #include "core/or/relay.h"
 #include "core/or/sendme.h"
 
@@ -82,3 +84,99 @@ sendme_circuit_consider_sending(circuit_t *circ, crypt_path_t *layer_hint)
     }
   }
 }
+
+/* Process a circuit-level SENDME cell that we just received. The layer_hint,
+ * if not NULL, is the Exit hop of the connection which means that we are a
+ * client. In that case, circ must be an origin circuit. The cell_body_len is
+ * the length of the SENDME cell payload (excluding the header).
+ *
+ * Return 0 on success that is the SENDME is valid and the package window has
+ * been updated properly.
+ *
+ * On error, a negative value is returned which indicate that the circuit must
+ * be closed using the value as the reason for it. */
+int
+sendme_process_circuit_level(crypt_path_t *layer_hint,
+                             circuit_t *circ, uint16_t cell_body_len)
+{
+  tor_assert(circ);
+
+  /* If we are the origin of the circuit, we are the Client so we use the
+   * layer hint (the Exit hop) for the package window tracking. */
+  if (CIRCUIT_IS_ORIGIN(circ)) {
+    if ((layer_hint->package_window + CIRCWINDOW_INCREMENT) >
+        CIRCWINDOW_START_MAX) {
+      static struct ratelim_t exit_warn_ratelim = RATELIM_INIT(600);
+      log_fn_ratelim(&exit_warn_ratelim, LOG_WARN, LD_PROTOCOL,
+                     "Unexpected sendme cell from exit relay. "
+                     "Closing circ.");
+      return -END_CIRC_REASON_TORPROTOCOL;
+    }
+    layer_hint->package_window += CIRCWINDOW_INCREMENT;
+    log_debug(LD_APP, "circ-level sendme at origin, packagewindow %d.",
+              layer_hint->package_window);
+
+    /* We count circuit-level sendme's as valid delivered data because they
+     * are rate limited. */
+    circuit_read_valid_data(TO_ORIGIN_CIRCUIT(circ), cell_body_len);
+  } else {
+    /* We aren't the origin of this circuit so we are the Exit and thus we
+     * track the package window with the circuit object. */
+    if ((circ->package_window + CIRCWINDOW_INCREMENT) >
+        CIRCWINDOW_START_MAX) {
+      static struct ratelim_t client_warn_ratelim = RATELIM_INIT(600);
+      log_fn_ratelim(&client_warn_ratelim, LOG_PROTOCOL_WARN, LD_PROTOCOL,
+                     "Unexpected sendme cell from client. "
+                     "Closing circ (window %d).", circ->package_window);
+      return -END_CIRC_REASON_TORPROTOCOL;
+    }
+    circ->package_window += CIRCWINDOW_INCREMENT;
+    log_debug(LD_EXIT, "circ-level sendme at non-origin, packagewindow %d.",
+              circ->package_window);
+  }
+
+  return 0;
+}
+
+/* Process a stream-level SENDME cell that we just received. The conn is the
+ * edge connection (stream) that the circuit circ is associated with. The
+ * cell_body_len is the length of the payload (excluding the header).
+ *
+ * Return 0 on success that is the SENDME is valid and the package window has
+ * been updated properly.
+ *
+ * On error, a negative value is returned which indicate that the circuit must
+ * be closed using the value as the reason for it. */
+int
+sendme_process_stream_level(edge_connection_t *conn, circuit_t *circ,
+                            uint16_t cell_body_len)
+{
+  tor_assert(conn);
+  tor_assert(circ);
+
+  /* Don't allow the other endpoint to request more than our maximum (i.e.
+   * initial) stream SENDME window worth of data. Well-behaved stock clients
+   * will not request more than this max (as per the check in the while loop
+   * of sendme_connection_edge_consider_sending()). */
+  if ((conn->package_window + STREAMWINDOW_INCREMENT) >
+      STREAMWINDOW_START_MAX) {
+    static struct ratelim_t stream_warn_ratelim = RATELIM_INIT(600);
+    log_fn_ratelim(&stream_warn_ratelim, LOG_PROTOCOL_WARN, LD_PROTOCOL,
+                   "Unexpected stream sendme cell. Closing circ (window %d).",
+                   conn->package_window);
+    return -END_CIRC_REASON_TORPROTOCOL;
+  }
+  /* At this point, the stream sendme is valid */
+  conn->package_window += STREAMWINDOW_INCREMENT;
+
+  /* We count circuit-level sendme's as valid delivered data because they are
+   * rate limited. */
+  if (CIRCUIT_IS_ORIGIN(circ)) {
+    circuit_read_valid_data(TO_ORIGIN_CIRCUIT(circ), cell_body_len);
+  }
+
+  log_debug(CIRCUIT_IS_ORIGIN(circ) ? LD_APP : LD_EXIT,
+            "stream-level sendme, package_window now %d.",
+            conn->package_window);
+  return 0;
+}

src/core/or/sendme.h

@@ -17,4 +17,9 @@ void sendme_connection_edge_consider_sending(edge_connection_t *edge_conn);
 void sendme_circuit_consider_sending(circuit_t *circ,
                                      crypt_path_t *layer_hint);
 
+int sendme_process_circuit_level(crypt_path_t *layer_hint,
+                                 circuit_t *circ, uint16_t cell_body_len);
+int sendme_process_stream_level(edge_connection_t *conn, circuit_t *circ,
+                                uint16_t cell_body_len);
+
 #endif /* !defined(TOR_SENDME_H) */