r17930@catbus: nickm | 2008-02-05 18:20:40 -0500

 Initial attempts to track down bug 600, and refactor possibly offending code.  1) complain early if circuit state is set to OPEN when an onionskin is pending.  2) refactor onionskin field into one only used when n_conn is pending, and a separate onionskin field waiting for attention by a cpuworker.  This might even fix the bug.  More likely, it will make it fail with a more useful core.


svn:r13394

ChangeLog

@@ -42,6 +42,14 @@ Changes in version 0.2.0.19-alpha - 2008-02-??
       port fails because we have overrun the limit on the number of
       connections, tell the controller that the request has failed.
 
+  o Code simplifications and refactoring:
+    - Remove some needless generality from cpuworker code, for improved
+      type-safety.
+    - Stop overloading the circuit_t.onionskin field for both "onionskin
+      from a CREATE cell that we are waiting for a cpuworker to be
+      assigned" and "onionskin from an EXTEND cell that we are going to
+      send to an OR as soon as we are connected".
+
 
 Changes in version 0.2.0.18-alpha - 2008-01-25
   o New directory authorities:

src/or/circuitbuild.c

@@ -461,12 +461,13 @@ circuit_n_conn_done(or_connection_t *or_conn, int status)
         }
       } else {
         /* pull the create cell out of circ->onionskin, and send it */
-        tor_assert(circ->onionskin);
-        if (circuit_deliver_create_cell(circ,CELL_CREATE,circ->onionskin)<0) {
+        tor_assert(circ->n_conn_onionskin);
+        if (circuit_deliver_create_cell(circ,CELL_CREATE,
+                                        circ->n_conn_onionskin)<0) {
           circuit_mark_for_close(circ, END_CIRC_REASON_RESOURCELIMIT);
           continue;
         }
-        tor_free(circ->onionskin);
+        tor_free(circ->n_conn_onionskin);
         circuit_set_state(circ, CIRCUIT_STATE_OPEN);
       }
     });
@@ -757,8 +758,8 @@ circuit_extend(cell_t *cell, circuit_t *circ)
     log_info(LD_CIRC|LD_OR,"Next router (%s:%d) not connected. Connecting.",
              tmpbuf, circ->n_port);
 
-    circ->onionskin = tor_malloc(ONIONSKIN_CHALLENGE_LEN);
-    memcpy(circ->onionskin, onionskin, ONIONSKIN_CHALLENGE_LEN);
+    circ->n_conn_onionskin = tor_malloc(ONIONSKIN_CHALLENGE_LEN);
+    memcpy(circ->n_conn_onionskin, onionskin, ONIONSKIN_CHALLENGE_LEN);
     circuit_set_state(circ, CIRCUIT_STATE_OR_WAIT);
 
     /* imprint the circuit with its future n_conn->id */

src/or/circuitlist.c

@@ -200,6 +200,8 @@ circuit_set_state(circuit_t *circ, int state)
     /* add to waiting-circuit list. */
     smartlist_add(circuits_pending_or_conns, circ);
   }
+  if (state == CIRCUIT_STATE_OPEN)
+    tor_assert(!circ->n_conn_onionskin);
   circ->state = state;
 }
 
@@ -413,8 +415,6 @@ circuit_free(circuit_t *circ)
       other->rend_splice = NULL;
     }
 
-    tor_free(circ->onionskin);
-
     /* remove from map. */
     circuit_set_p_circid_orconn(ocirc, 0, NULL);
 
@@ -423,6 +423,8 @@ circuit_free(circuit_t *circ)
     cell_queue_clear(&ocirc->p_conn_cells);
   }
 
+  tor_free(circ->n_conn_onionskin);
+
   /* Remove from map. */
   circuit_set_n_circid_orconn(circ, 0, NULL);
 
@@ -1162,7 +1164,7 @@ assert_circuit_ok(const circuit_t *c)
   tor_assert(c->deliver_window >= 0);
   tor_assert(c->package_window >= 0);
   if (c->state == CIRCUIT_STATE_OPEN) {
-    tor_assert(!c->onionskin);
+    tor_assert(!c->n_conn_onionskin);
     if (or_circ) {
       tor_assert(or_circ->n_crypto);
       tor_assert(or_circ->p_crypto);

src/or/command.c

@@ -262,11 +262,11 @@ command_process_create_cell(cell_t *cell, or_connection_t *conn)
   circ->_base.purpose = CIRCUIT_PURPOSE_OR;
   circuit_set_state(TO_CIRCUIT(circ), CIRCUIT_STATE_ONIONSKIN_PENDING);
   if (cell->command == CELL_CREATE) {
-    circ->_base.onionskin = tor_malloc(ONIONSKIN_CHALLENGE_LEN);
-    memcpy(circ->_base.onionskin, cell->payload, ONIONSKIN_CHALLENGE_LEN);
+    char *onionskin = tor_malloc(ONIONSKIN_CHALLENGE_LEN);
+    memcpy(onionskin, cell->payload, ONIONSKIN_CHALLENGE_LEN);
 
     /* hand it off to the cpuworkers, and then return. */
-    if (assign_to_cpuworker(NULL, CPUWORKER_TASK_ONION, circ) < 0) {
+    if (assign_onionskin_to_cpuworker(NULL, circ, onionskin) < 0) {
       log_warn(LD_GENERAL,"Failed to hand off onionskin. Closing.");
       circuit_mark_for_close(TO_CIRCUIT(circ), END_CIRC_REASON_INTERNAL);
       return;

src/or/cpuworker.c

@@ -387,15 +387,16 @@ static void
 process_pending_task(connection_t *cpuworker)
 {
   or_circuit_t *circ;
+  char *onionskin = NULL;
 
   tor_assert(cpuworker);
 
   /* for now only process onion tasks */
 
-  circ = onion_next_task();
+  circ = onion_next_task(&onionskin);
   if (!circ)
     return;
-  if (assign_to_cpuworker(cpuworker, CPUWORKER_TASK_ONION, circ) < 0)
+  if (assign_onionskin_to_cpuworker(cpuworker, circ, onionskin))
     log_warn(LD_OR,"assign_to_cpuworker failed. Ignoring.");
 }
 
@@ -433,28 +434,22 @@ cull_wedged_cpuworkers(void)
 /** If cpuworker is defined, assert that he's idle, and use him. Else,
  * look for an idle cpuworker and use him. If none idle, queue task onto
  * the pending onion list and return.
- * If question_type is CPUWORKER_TASK_ONION then task is a circ.
- * No other question_types are allowed.
+ * DOCDOC this function is now less general
  */
 int
-assign_to_cpuworker(connection_t *cpuworker, uint8_t question_type,
-                    void *task)
+assign_onionskin_to_cpuworker(connection_t *cpuworker,
+                              or_circuit_t *circ, char *onionskin)
 {
-  or_circuit_t *circ;
+  char qbuf[1];
   char tag[TAG_LEN];
 
-  tor_assert(question_type == CPUWORKER_TASK_ONION);
-
   cull_wedged_cpuworkers();
   spawn_enough_cpuworkers();
 
-  if (question_type == CPUWORKER_TASK_ONION) {
-    circ = task;
-    tor_assert(circ->_base.onionskin);
-
+  if (1) {
     if (num_cpuworkers_busy == num_cpuworkers) {
       log_debug(LD_OR,"No idle cpuworkers. Queuing.");
-      if (onion_pending_add(circ) < 0)
+      if (onion_pending_add(circ, onionskin) < 0)
         return -1;
       return 0;
     }
@@ -467,6 +462,7 @@ assign_to_cpuworker(connection_t *cpuworker, uint8_t question_type,
 
     if (!circ->p_conn) {
       log_info(LD_OR,"circ->p_conn gone. Failing circ.");
+      tor_free(onionskin);
       return -1;
     }
     tag_pack(tag, circ->p_conn->_base.addr, circ->p_conn->_base.port,
@@ -479,11 +475,11 @@ assign_to_cpuworker(connection_t *cpuworker, uint8_t question_type,
     cpuworker->timestamp_lastwritten = time(NULL);
     num_cpuworkers_busy++;
 
-    connection_write_to_buf((char*)&question_type, 1, cpuworker);
+    qbuf[0] = CPUWORKER_TASK_ONION;
+    connection_write_to_buf(qbuf, 1, cpuworker);
     connection_write_to_buf(tag, sizeof(tag), cpuworker);
-    connection_write_to_buf(circ->_base.onionskin, ONIONSKIN_CHALLENGE_LEN,
-                            cpuworker);
-    tor_free(circ->_base.onionskin);
+    connection_write_to_buf(onionskin, ONIONSKIN_CHALLENGE_LEN, cpuworker);
+    tor_free(onionskin);
   }
   return 0;
 }

src/or/onion.c

@@ -19,6 +19,7 @@ const char onion_c_id[] =
  * to process a waiting onion handshake. */
 typedef struct onion_queue_t {
   or_circuit_t *circ;
+  char *onionskin;
   time_t when_added;
   struct onion_queue_t *next;
 } onion_queue_t;
@@ -37,13 +38,14 @@ static int ol_length=0;
  * if ol_list is too long, in which case do nothing and return -1.
  */
 int
-onion_pending_add(or_circuit_t *circ)
+onion_pending_add(or_circuit_t *circ, char *onionskin)
 {
   onion_queue_t *tmp;
   time_t now = time(NULL);
 
   tmp = tor_malloc_zero(sizeof(onion_queue_t));
   tmp->circ = circ;
+  tmp->onionskin = onionskin;
   tmp->when_added = now;
 
   if (!ol_tail) {
@@ -86,7 +88,7 @@ onion_pending_add(or_circuit_t *circ)
  * NULL if the list is empty.
  */
 or_circuit_t *
-onion_next_task(void)
+onion_next_task(char **onionskin_out)
 {
   or_circuit_t *circ;
 
@@ -97,6 +99,8 @@ onion_next_task(void)
   tor_assert(ol_list->circ->p_conn); /* make sure it's still valid */
   tor_assert(ol_length > 0);
   circ = ol_list->circ;
+  *onionskin_out = ol_list->onionskin;
+  ol_list->onionskin = NULL; /* prevent free. */
   onion_pending_remove(ol_list->circ);
   return circ;
 }
@@ -139,6 +143,7 @@ onion_pending_remove(or_circuit_t *circ)
 
   /* now victim points to the element that needs to be removed */
 
+  tor_free(victim->onionskin);
   tor_free(victim);
 }
 
@@ -448,6 +453,7 @@ clear_pending_onions(void)
   while (ol_list) {
     onion_queue_t *victim = ol_list;
     ol_list = victim->next;
+    tor_free(victim->onionskin);
     tor_free(victim);
   }
   ol_list = ol_tail = NULL;

src/or/or.h

@@ -1775,11 +1775,10 @@ typedef struct circuit_t {
    * more. */
   int deliver_window;
 
-  /** For storage while passing to cpuworker (state
-    * CIRCUIT_STATE_ONIONSKIN_PENDING), or while n_conn is pending
+  /** For storage while n_conn is pending
     * (state CIRCUIT_STATE_OR_WAIT). When defined, it is always
     * length ONIONSKIN_CHALLENGE_LEN. */
-  char *onionskin;
+  char *n_conn_onionskin;
 
   time_t timestamp_created; /**< When was this circuit created? */
   time_t timestamp_dirty; /**< When the circuit was first used, or 0 if the
@@ -2979,8 +2978,9 @@ void cpuworkers_rotate(void);
 int connection_cpu_finished_flushing(connection_t *conn);
 int connection_cpu_reached_eof(connection_t *conn);
 int connection_cpu_process_inbuf(connection_t *conn);
-int assign_to_cpuworker(connection_t *cpuworker, uint8_t question_type,
-                        void *task);
+int assign_onionskin_to_cpuworker(connection_t *cpuworker,
+                                  or_circuit_t *circ,
+                                  char *onionskin);
 
 /********************************* directory.c ***************************/
 
@@ -3398,8 +3398,8 @@ void nt_service_set_state(DWORD state);
 
 /********************************* onion.c ***************************/
 
-int onion_pending_add(or_circuit_t *circ);
-or_circuit_t *onion_next_task(void);
+int onion_pending_add(or_circuit_t *circ, char *onionskin);
+or_circuit_t *onion_next_task(char **onionskin_out);
 void onion_pending_remove(or_circuit_t *circ);
 
 int onion_skin_create(crypto_pk_env_t *router_key,