r9272@Kushana: nickm | 2006-10-19 12:52:37 -0400

Fix an XXX in handling destroy cells: when we get a destroy cell with reason FOO, do not tell the controller REASON=FOO.  Instead, say REASON=DESTROYED REMOTE_REASON=FOO. Suggested by a conversation with Mike Perry.


svn:r8760

ChangeLog

@@ -10,6 +10,8 @@ Changes in version 0.1.2.3-alpha - 2006-10-??
       event format.  Also, add additional reason codes to explain why a
       given circuit has been destroyed or truncated. (Patches from Mike
       Perry)
+    - Add a REMOTE_REASON field to CIRC events to tell the controller about
+      why a remote OR told us to close a circuit.
 
   o Security bugfixes:
     - When the user sends a NEWNYM signal, clear the client-side DNS

doc/control-spec.txt

@@ -788,7 +788,7 @@ $Id$
    The syntax is:
 
      "650" SP "CIRC" SP CircuitID SP CircStatus [SP Path]
-          [SP "REASON=" Reason] CRLF
+          [SP "REASON=" Reason [SP "REMOTE_REASON=" Reason]] CRLF
 
       CircStatus =
                "LAUNCHED" / ; circuit ID assigned to new circuit
@@ -813,6 +813,11 @@ $Id$
 
       NOPATH          (Not enough nodes to make circuit)
 
+   The "REMOTE_REASON" field is provided only when we receive a DESTROY or
+   TRUNCATE cell, and only if extended events are enabled.  It contains the
+   actual reason given by the remote OR for closing the circuit. Clients MUST
+   accept reasons not listed above.  Reasons are as listed in tor-spec.txt.
+
 4.1.2. Stream status changed
 
     The syntax is:

src/or/circuitlist.c

@@ -848,6 +848,10 @@ _circuit_mark_for_close(circuit_t *circ, int reason, int line,
      * to track them anyway so we can give them to the controller. */
     reason = END_CIRC_REASON_NONE;
   }
+
+  if (reason & END_CIRC_REASON_FLAG_REMOTE)
+    reason &= ~END_CIRC_REASON_FLAG_REMOTE;
+
   if (reason < _END_CIRC_REASON_MIN || reason > _END_CIRC_REASON_MAX) {
     log_warn(LD_BUG, "Reason %d out of range at %s:%d", reason, file, line);
     orig_reason = reason = END_CIRC_REASON_NONE;

src/or/command.c

@@ -359,7 +359,7 @@ static void
 command_process_destroy_cell(cell_t *cell, or_connection_t *conn)
 {
   circuit_t *circ;
-  uint8_t reason;
+  int reason;
 
   circ = circuit_get_by_circid_orconn(cell->circ_id, conn);
   reason = (uint8_t)cell->payload[0];
@@ -378,25 +378,7 @@ command_process_destroy_cell(cell_t *cell, or_connection_t *conn)
   } else { /* the destroy came from ahead */
     circuit_set_n_circid_orconn(circ, 0, NULL);
     if (CIRCUIT_IS_ORIGIN(circ)) {
-      /* Prevent arbitrary destroys from going unnoticed by controller */
+      circuit_mark_for_close(circ, reason|END_CIRC_REASON_FLAG_REMOTE);
-      if (reason == END_CIRC_REASON_NONE ||
-          reason == END_CIRC_REASON_FINISHED ||
-          reason == END_CIRC_REASON_REQUESTED) {
-        /* XXXX This logic is wrong.  Really, we should report the fact that
-         * the circuit was closed because of a DESTROY, *and* we should report
-         * the reason that we were given. -NM
-         *   Hrmm. We could store the fact that we sent a truncate and the
-         * reason for this truncate in circuit_t. If we ever get a destroy
-         * that doesn't match this reason, we could complain loudly -MP
-         *   That won't work for the cases where the destroy is not because of
-         * a truncate, though.  The idea is that if we get a DESTROYED cell
-         * with reason 'CONNECTFAILED' and another DESTROYED cell with reason
-         * 'RESOURCELIMIT', the controller may want to know the reported
-         * reason. -NM
-         */
-        reason = END_CIRC_REASON_DESTROYED;
-      }
-      circuit_mark_for_close(circ, reason);
     } else {
       char payload[1];
       log_debug(LD_OR, "Delivering 'truncated' back.");

src/or/control.c

@@ -2802,42 +2802,44 @@ connection_control_process_inbuf(control_connection_t *conn)
 static const char *
 circuit_end_reason_to_string(int reason)
 {
+  if (reason >= 0 && reason & END_CIRC_REASON_FLAG_REMOTE)
+    reason &= ~END_CIRC_REASON_FLAG_REMOTE;
   switch (reason) {
     case END_CIRC_AT_ORIGIN:
       /* This shouldn't get passed here; it's a catch-all reason. */
-      return "REASON=ORIGIN";
+      return "ORIGIN";
     case END_CIRC_REASON_NONE:
       /* This shouldn't get passed here; it's a catch-all reason. */
-      return "REASON=NONE";
+      return "NONE";
     case END_CIRC_REASON_TORPROTOCOL:
-      return "REASON=TORPROTOCOL";
+      return "TORPROTOCOL";
     case END_CIRC_REASON_INTERNAL:
-      return "REASON=INTERNAL";
+      return "INTERNAL";
     case END_CIRC_REASON_REQUESTED:
-      return "REASON=REQUESTED";
+      return "REQUESTED";
     case END_CIRC_REASON_HIBERNATING:
-      return "REASON=HIBERNATING";
+      return "HIBERNATING";
     case END_CIRC_REASON_RESOURCELIMIT:
-      return "REASON=RESOURCELIMIT";
+      return "RESOURCELIMIT";
     case END_CIRC_REASON_CONNECTFAILED:
-      return "REASON=CONNECTFAILED";
+      return "CONNECTFAILED";
     case END_CIRC_REASON_OR_IDENTITY:
-      return "REASON=OR_IDENTITY";
+      return "OR_IDENTITY";
     case END_CIRC_REASON_OR_CONN_CLOSED:
-      return "REASON=OR_CONN_CLOSED";
+      return "OR_CONN_CLOSED";
     case END_CIRC_REASON_FINISHED:
-      return "REASON=FINISHED";
+      return "FINISHED";
     case END_CIRC_REASON_TIMEOUT:
-      return "REASON=TIMEOUT";
+      return "TIMEOUT";
     case END_CIRC_REASON_DESTROYED:
-      return "REASON=DESTROYED";
+      return "DESTROYED";
     case END_CIRC_REASON_NOPATH:
-      return "REASON=NOPATH";
+      return "NOPATH";
     case END_CIRC_REASON_NOSUCHSERVICE:
-      return "REASON=NOSUCHSERVICE";
+      return "NOSUCHSERVICE";
     default:
       log_warn(LD_BUG, "Unrecognized reason code %d", (int)reason);
-      return "REASON=UNRECOGNIZED"; /* should never get called */
+      return NULL;
   }
 }
 
@@ -2867,7 +2869,7 @@ control_event_circuit_status(origin_circuit_t *circ, circuit_status_event_t tp,
   }
   if (EVENT_IS_INTERESTING1(EVENT_CIRCUIT_STATUS)) {
     const char *status;
-    const char *reason = "";
+    char reason_buf[64];
     switch (tp)
       {
       case CIRC_EVENT_LAUNCHED: status = "LAUNCHED"; break;
@@ -2881,7 +2883,21 @@ control_event_circuit_status(origin_circuit_t *circ, circuit_status_event_t tp,
       }
 
     if (tp == CIRC_EVENT_FAILED || tp == CIRC_EVENT_CLOSED) {
-      reason = circuit_end_reason_to_string(reason_code);
+      const char *reason_str = circuit_end_reason_to_string(reason_code);
+      char *reason = NULL;
+      if (!reason_str) {
+        reason = tor_malloc(16);
+        tor_snprintf(reason, 16, "UNKNOWN_%d", reason_code);
+        reason_str = reason;
+      }
+      if (reason_code > 0 && reason_code & END_CIRC_REASON_FLAG_REMOTE) {
+        tor_snprintf(reason_buf, sizeof(reason_buf),
+                     "REASON=DESTROYED REMOTE_REASON=%s", reason_str);
+      } else {
+        tor_snprintf(reason_buf, sizeof(reason_buf),
+                     "REASON=%s", reason_str);
+      }
+      tor_free(reason);
     }
 
     if (EVENT_IS_INTERESTING1S(EVENT_CIRCUIT_STATUS)) {
@@ -2889,7 +2905,7 @@ control_event_circuit_status(origin_circuit_t *circ, circuit_status_event_t tp,
       send_control1_event_extended(EVENT_CIRCUIT_STATUS, SHORT_NAMES,
                           "650 CIRC %lu %s%s%s@%s\r\n",
                           (unsigned long)circ->global_identifier,
-                          status, sp, path, reason);
+                          status, sp, path, reason_buf);
     }
     if (EVENT_IS_INTERESTING1L(EVENT_CIRCUIT_STATUS)) {
       char *vpath = circuit_list_path_for_controller(circ);
@@ -2897,7 +2913,7 @@ control_event_circuit_status(origin_circuit_t *circ, circuit_status_event_t tp,
       send_control1_event_extended(EVENT_CIRCUIT_STATUS, LONG_NAMES,
                           "650 CIRC %lu %s%s%s@%s\r\n",
                           (unsigned long)circ->global_identifier,
-                          status, sp, vpath, reason);
+                          status, sp, vpath, reason_buf);
       tor_free(vpath);
     }
   }

src/or/or.h

@@ -497,6 +497,7 @@ typedef enum {
 /* Negative reasons are internal */
 #define END_CIRC_REASON_NOPATH          -2
 #define END_CIRC_AT_ORIGIN              -1
+
 #define _END_CIRC_REASON_MIN            0
 #define END_CIRC_REASON_NONE            0
 #define END_CIRC_REASON_TORPROTOCOL     1
@@ -513,6 +514,11 @@ typedef enum {
 #define END_CIRC_REASON_NOSUCHSERVICE   12
 #define _END_CIRC_REASON_MAX            12
 
+/* OR this with the argument to circuit_mark_for_close, or
+ * control_event_circuit_status to indicate that the reason came from a
+ * destroy or truncate cell. */
+#define END_CIRC_REASON_FLAG_REMOTE     512
+
 /** Length of 'y' portion of 'y.onion' URL. */
 #define REND_SERVICE_ID_LEN 16