Avoid chan/circ linear lookups for requests

The solution I took is to not free a circuit with a pending
uncancellable work item, but rather to set its magic number to a
sentinel value.  When we get a work item, we check whether the circuit
has that magic sentinel, and if so, we free it rather than processing
the reply.

src/or/circuitlist.c

@@ -745,6 +745,7 @@ circuit_free(circuit_t *circ)
 {
   void *mem;
   size_t memlen;
+  int should_free = 1;
   if (!circ)
     return;
 
@@ -784,6 +785,8 @@ circuit_free(circuit_t *circ)
     memlen = sizeof(or_circuit_t);
     tor_assert(circ->magic == OR_CIRCUIT_MAGIC);
 
+    should_free = (ocirc->workqueue_entry == NULL);
+
     crypto_cipher_free(ocirc->p_crypto);
     crypto_digest_free(ocirc->p_digest);
     crypto_cipher_free(ocirc->n_crypto);
@@ -826,8 +829,18 @@ circuit_free(circuit_t *circ)
    * "active" checks will be violated. */
   cell_queue_clear(&circ->n_chan_cells);
 
-  memwipe(mem, 0xAA, memlen); /* poison memory */
-  tor_free(mem);
+  if (should_free) {
+    memwipe(mem, 0xAA, memlen); /* poison memory */
+    tor_free(mem);
+  } else {
+    /* If we made it here, this is an or_circuit_t that still has a pending
+     * cpuworker request which we weren't able to cancel.  Instead, set up
+     * the magic value so that when the reply comes back, we'll know to discard
+     * the reply and free this structure.
+     */
+    memwipe(mem, 0xAA, memlen);
+    circ->magic = DEAD_CIRCUIT_MAGIC;
+  }
 }
 
 /** Deallocate the linked list circ-><b>cpath</b>, and remove the cpath from

src/or/cpuworker.c

@@ -152,8 +152,7 @@ typedef struct cpuworker_reply_t {
 } cpuworker_reply_t;
 
 typedef struct cpuworker_job_u {
-  uint64_t chan_id;
-  uint32_t circ_id;
+  or_circuit_t *circ;
   union {
     cpuworker_request_t request;
     cpuworker_reply_t reply;
@@ -297,16 +296,13 @@ cpuworker_log_onionskin_overhead(int severity, int onionskin_type,
          onionskin_type_name, (unsigned)overhead, relative_overhead*100);
 }
 
-/** */
+/** Handle a reply from the worker threads. */
 static void
 cpuworker_onion_handshake_replyfn(void *work_)
 {
   cpuworker_job_t *job = work_;
   cpuworker_reply_t rpl;
-  uint64_t chan_id;
-  circid_t circ_id;
-  channel_t *p_chan = NULL;
-  circuit_t *circ = NULL;
+  or_circuit_t *circ = NULL;
 
   --total_pending_tasks;
 
@@ -338,46 +334,40 @@ cpuworker_onion_handshake_replyfn(void *work_)
       }
     }
   }
-  /* Find the circ it was talking about */
-  chan_id = job->chan_id;
-  circ_id = job->circ_id;
 
-  p_chan = channel_find_by_global_id(chan_id);
-
-  if (p_chan)
-    circ = circuit_get_by_circid_channel(circ_id, p_chan);
+  circ = job->circ;
 
   log_debug(LD_OR,
-            "Unpacking cpuworker reply %p, chan_id is " U64_FORMAT
-            ", circ_id is %u, p_chan=%p, circ=%p, success=%d",
-            job, U64_PRINTF_ARG(chan_id), (unsigned)circ_id,
-            p_chan, circ, rpl.success);
+            "Unpacking cpuworker reply %p, circ=%p, success=%d",
+            job, circ, rpl.success);
+
+  if (circ->base_.magic == DEAD_CIRCUIT_MAGIC) { 
+    /* The circuit was supposed to get freed while the reply was
+     * pending. Instead, it got left for us to free so that we wouldn't freak
+     * out when the job->circ field wound up pointing to nothing. */
+    log_debug(LD_OR, "Circuit died while reply was pending. Freeing memory.");
+    circ->base_.magic = 0;
+    tor_free(circ);
+    goto done_processing;
+  }
+
+  circ->workqueue_entry = NULL;
 
   if (rpl.success == 0) {
     log_debug(LD_OR,
               "decoding onionskin failed. "
               "(Old key or bad software.) Closing.");
     if (circ)
-      circuit_mark_for_close(circ, END_CIRC_REASON_TORPROTOCOL);
+      circuit_mark_for_close(TO_CIRCUIT(circ), END_CIRC_REASON_TORPROTOCOL);
     goto done_processing;
   }
-  if (!circ) {
-    /* This happens because somebody sends us a destroy cell and the
-     * circuit goes away, while the cpuworker is working. This is also
-     * why our tag doesn't include a pointer to the circ, because we'd
-     * never know if it's still valid.
-     */
-    log_debug(LD_OR,"processed onion for a circ that's gone. Dropping.");
-    goto done_processing;
-  }
-  tor_assert(! CIRCUIT_IS_ORIGIN(circ));
-  TO_OR_CIRCUIT(circ)->workqueue_entry = NULL;
-  if (onionskin_answer(TO_OR_CIRCUIT(circ),
+
+  if (onionskin_answer(circ,
                        &rpl.created_cell,
                        (const char*)rpl.keys,
                        rpl.rend_auth_material) < 0) {
     log_warn(LD_OR,"onionskin_answer failed. Closing.");
-    circuit_mark_for_close(circ, END_CIRC_REASON_INTERNAL);
+    circuit_mark_for_close(TO_CIRCUIT(circ), END_CIRC_REASON_INTERNAL);
     goto done_processing;
   }
   log_debug(LD_OR,"onionskin_answer succeeded. Yay.");
@@ -527,8 +517,7 @@ assign_onionskin_to_cpuworker(or_circuit_t *circ,
     tor_gettimeofday(&req.started_at);
 
   job = tor_malloc_zero(sizeof(cpuworker_job_t));
-  job->chan_id = circ->p_chan->global_identifier;
-  job->circ_id = circ->p_circ_id;
+  job->circ = circ;
   memcpy(&job->u.request, &req, sizeof(req));
   memwipe(&req, 0, sizeof(req));
 
@@ -542,8 +531,9 @@ assign_onionskin_to_cpuworker(or_circuit_t *circ,
     tor_free(job);
     return -1;
   }
-  log_debug(LD_OR, "Queued task %p (qe=%p, chanid="U64_FORMAT", circid=%u)",
-            job, queue_entry, U64_PRINTF_ARG(job->chan_id), job->circ_id);
+
+  log_debug(LD_OR, "Queued task %p (qe=%p, circ=%p)",
+            job, queue_entry, job->circ);
 
   circ->workqueue_entry = queue_entry;
 

src/or/or.h

@@ -2725,8 +2725,14 @@ typedef struct {
   time_t expiry_time;
 } cpath_build_state_t;
 
+/** "magic" value for an origin_circuit_t */
 #define ORIGIN_CIRCUIT_MAGIC 0x35315243u
+/** "magic" value for an or_circuit_t */
 #define OR_CIRCUIT_MAGIC 0x98ABC04Fu
+/** "magic" value for a circuit that would have been freed by circuit_free,
+ * but which we're keeping around until a cpuworker reply arrives.  See
+ * circuit_free() for more documentation. */
+#define DEAD_CIRCUIT_MAGIC 0xdeadc14c
 
 struct create_cell_t;