[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 nickm):

 PART 2:

 A general thing: this branch doesn't actually compile for me.  I get lots
 of warnings like this:
 {{{
 src/or/channel.c:1595: warning: format ‘%lu’ expects type ‘long unsigned
 int’, but argument 7 has type ‘uint64_t’
 }}}
 Please try building on a more recent GCC and/or a 64-bit platform and/or a
 32-bit platform and/or with a recent clang -- whatever you haven't tried
 yet.

 Another general thing: I'm not done reading yet, but I'm a little nervous
 about queues and buffers here.  As I understand it, there will be 5 layers
 where cells get queued:
  * The circuit's queue of cells
  * The channel's queue of cells [*]
  * The or_connection_t's queue of data to encrypt (conn->outbuf)
  * SSL's write buffer. (ssl->wbuf)
  * The kernel's write buffer.

 The one marked with [*] above is new; what does it give us that
 conn->outbuf doesn't?  And what are we doing to make sure it doesn't get
 too big?  We don't want it getting large, since we want to use the circuit
 pqueue mechanism to choose an active circuit late.  Filling buffers
 aggressively here means that loud circuits fill the queue, and quiet low-
 latency circuits don't get heard.


 Last general thing: Some of these functions should probably take const
 channel_t*, not channel_t*.

 Other stuff, going on with my reading:
  * In channel_process_incoming, do we want to process that queue in-order?
 It seems that doing the DEL_CURRENT approach is likely to have weird
 consequences. Perhaps it would be better to extract all the members into a
 separate list (smartlist_add_all(), smartlist_clear()), then process the
 separate list, then free it.
  * Should channel_more_to_flush() check outgoing_queue rather than
 cell_queue?
  * Should cell_queue be renamed incoming_queue for consistency? I think it
 should.
  * In channel_process_cells, I feel like maybe we're going to hit trouble
 if there isn't a way to break out in the middle of processing all those
 cells?  I guess that might be happening in the middle of the processing
 functions, where they note if there's an error and don't process anything
 if an error has occurred.
  * It seems like channel_process_cells, channel_queue_cell, and
 channel_queue_var_cell have some common code that should be extracted.
  * In channel_send_destroy, I am confused about why we're calling
 channel_write_cell instead of channel_queue_cell.  In fact, it's likely
 that I'm going to have this confusion about every
 channel_write_cell/channel_queue_cell instance.  Hmm.  I wonder if there's
 any way we can make those more distinct/obvious/safe?
  * I'm confused about why channel_send_destroy() is here, but there aren't
 other channel_send_foo() functions.
  * The first part of channel_free_all() seems redundant with
 channel_run_cleanup().
  * Typo in comment in channel.h : "see thw channel_t"
  * Crazy idea: How badly, if at all, would we break things by making the
 channel_s definition internal so that only channel.c (and later others if
 needed) was allowed to look at it?  It's not how we do most stuff right
 now, but I bet it would make us happier in the long run.

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