[tor-bugs] #6465 [Tor Relay]: Build abstraction layer around TLS

[Tor Bug Tracker & Wiki]

#6465: Build abstraction layer around TLS
-----------------------+----------------------------------------------------
 Reporter:  andrea     |          Owner:  andrea            
     Type:  project    |         Status:  needs_review      
 Priority:  major      |      Milestone:  Tor: 0.2.4.x-final
Component:  Tor Relay  |        Version:  Tor: unspecified  
 Keywords:             |         Parent:                    
   Points:             |   Actualpoints:                    
-----------------------+----------------------------------------------------

Comment(by andrea):

 Responses to part 1 (points in potential need of further discussion):

 > In channel_free, we free the active_circuit_pqueue smartlist, but don't
 do
 > anything to its members. Should it be documented what does mark/free
 them?

 The members are circuits, and they shouldn't be freed there because that
 queue just tracks which are ready to send; they get added/removed all the
 time other than from channel_free().  It's a bit ugly, but it was that way
 on or_connection_t and just got moved, and it's going to go away when I
 finish 6816 anyway.

 > Does connection_write_cell() potentially double-flush? That is, can it
 try
 > to call write_cell, then call channel_flush_cells() on the same cell
 even
 > if the first write didn't work? If so, is that a problem?

 (Assuming you meant channel_write_cell()) It could, but only if
 write_cell()
 failed the first time, so if channel_flush_cells() calls it again it
 should
 either succeed or be idempotent.

 > channel_clear_identity_digest makes me kind of wish that we had a bit on
 the
 > channel_t to tell us whether the channel should be in the id-digest map.
 > Looking at states like this feels a little fragile.

 Change that to 'function to compute whether a channel should be in the id-
 digest map' and I'll go with it; adding a bit just means a bit that needs
 to
 be kept in sync with the other state and creates room for bugs if it
 isn't.

 > In channel_closed(), we only call "circuit_n_chan_done(chan, 0);" in one
 > of the close paths. What tells those circuits that the channel is
 closing
 > otherwise? Or am I missing something?

 The circuit_unlink_all_from_channel(); I think it's the same logic
 or_connection_t used to have.

 > Using a smartlist of cells for an outgoing queue makes me a little
 nervous.
 > Popping the first item from this queue is going to be O(N).

 Agreed, but you were complaining about me having all that doubly-linked
 list
 logic elsewhere.  Either make up your mind, or we need to add some stuff
 to
 src/common/container.c (and while we're at it I wouldn't mind a balanced
 tree
 usable for maps and sets for something with 6816).

 > In channel_write_cell: It looks like there are only two possible return
 > values from write_cell looking at this function's use of it. I would
 have
 > at least expected "I could write it" "I couldn't write it", and "I got
 an
 > error." Must investigate later.

 'Must investigate later'? I think your code review has a bug there. :)

 Anyway, if there's an error that can be retried, the lower layer should
 just
 return 'I couldn't write it' and it'll get retried, and if it can't, then
 the lower layer can close the channel for it.  Should there be some other
 behavior?

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