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

7 files changed, 51 insertions, 38 deletions

diff --git a/ChangeLog b/ChangeLog
index 0777caa1e..74d251035 100644
--- a/ChangeLog
+++ b/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:
diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c
index 785926b09..512b5d59d 100644
--- a/src/or/circuitbuild.c
+++ b/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 */
diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c
index 8eb442df8..002a7df23 100644
--- a/src/or/circuitlist.c
+++ b/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);
diff --git a/src/or/command.c b/src/or/command.c
index ea626416b..1d16fdb7a 100644
--- a/src/or/command.c
+++ b/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;
diff --git a/src/or/cpuworker.c b/src/or/cpuworker.c
index b1f56d3db..7d5cf679b 100644
--- a/src/or/cpuworker.c
+++ b/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;
 }
diff --git a/src/or/onion.c b/src/or/onion.c
index fb516da24..c91b7d556 100644
--- a/src/or/onion.c
+++ b/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;
diff --git a/src/or/or.h b/src/or/or.h
index a82c44be4..6041e84c7 100644
--- a/src/or/or.h
+++ b/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,