[tor-bugs] #1184 [Tor Client]: Sending useless relay cells after the destroy cell

[Tor Bug Tracker & Wiki]

#1184: Sending useless relay cells after the destroy cell
--------------------------------+-------------------------------------------
 Reporter:  arma                |         Type:  defect    
   Status:  needs_review        |     Priority:  minor     
Milestone:  Tor: 0.2.2.x-final  |    Component:  Tor Client
  Version:  0.2.2.6-alpha       |   Resolution:  None      
 Keywords:                      |       Parent:            
--------------------------------+-------------------------------------------

Comment(by nickm):

 >  "is it the case that when we try to move a cell from circ's cell queue
 onto the outbuf, we notice that circ->n_conn is null and choose not to
 send the cell"

 See below....

 > "where the heck do we move cells from the circ cell queue onto the
 outbuf other than connection_or_flush_from_first_active_circuit() which
 seems to only move one cell and then stop"

 That's the only place [source: grepping for uses of cell_queue_pop, which
 is the only place that pulls a cell of a cell queue].  The function
 doesn't "only move one cell and then stop", though.  The "for (n_flushed =
 0; n_flushed < max && queue->head; ) {" loop in the middle sends more than
 one cell.  It stops when "max" cells have been flushed to the or_conn's
 outbuf, or when there are no more active circuits (that is, ones with
 cells to flush).

 So, to answer the first question above: the process of moving cells to an
 orconn outbuf is entirely driven by the orconn looking through its list of
 active circuits, and not by doing anything to the circuit, it's not
 possible that a null n_conn on the circ will get followed.

 >  "are there any cases where an inactive circuit moves cells to the
 outbuf" (I think no)

 I agree.

 > "at what point in closing a circuit do we clear its cell queue" (I guess
 when we're freeing it and not before).

 Right. cell_queue_clear() is called in circuit_free and nowhere else.
 [This branch changes it so that it is now also called by
 circuit_clear_cell_queue.]

 > but now I'm wondering why we don't call that from the two cases in
 _circuit_mark_for_close() that send destroy cells.

 I guess we could unlink the circuit from p_conn/n_conn as appropriate, but
 I am not sure we need to.  What would the rationale be? When we call
 circuit_clear_cell_queue(), the circuit becomes inactive.  It can't get
 any more cells added to it, because it's marked, so it can't become
 active, so it can't flush cells.

 /me checks whether "It can't get any more cells added to it, because it's
 marked" is true.

 Processing.

 Processing.

 Hm.  This is some complicated stuff.  It might be a good idea to add a
 check to append_cell_to_circuit_queue() so that we drop cells that anybody
 tries to queue on a marked circuit.

 Also, I can't see a reason _not_ to unlink the circuit just after sending
 the destroy.  What Could Go Wrong?

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/1184#comment:15>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online