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

 General notes as I read.  There will likely be a few of these as this is a
 big patch.  I'm reading though the _squashed branch with git log -p
 --reverse, so if I say something now that a later commit in that branch
 will make nonsensical, that's probably why.

 PART 1:
  * Functions like channel_register probably want to note not only what
 they do, but when you must call them.  Similarly, when there's a field in
 a data structure that shouldn't ever be changed except by a particular
 group of functions, that field's documentation should say so.
  * Your doxygen usage style isn't the one we've been using.  Some stuff
 (like using @param and @return) is cool and we should consider whether we
 want to do that more.  But some stuff (like dropping the imperative-
 sentence) style shouldn't proliferate.  This doesn't need a cleanup before
 we can merge this branch, but should get made more uniform sooner or later
 once we decide what to do.
  * You're doing a printf of global_identifier using the %lu format.  That
 won't work on systems where long isn't 64 bits, right?  To format a uint64
 portably, you need to do the dance with U64_FORMAT and U64_PRINTF_ARG.
  * channel_remove_from_digest_map is very very complex.  Can anything be
 done to make it simpler?  For instance, could the error-case code get
 extracted into a separate function or something?
  * Assignments like
 "chan->u.cell_chan.prev_with_same_id->u.cell_chan.next_with_same_id =
 chan->u.cell_chan.next_with_same_id;" are pretty verbose and make me
 wonder whether we've got some lack-of-genericity problem. Not sure if we
 need to solve them now or what the right solution is.
  * I don't see the point of the rv=tmp dance in
 channel_find_by_remote_digest.
  * channel_find_by_remote_nickname really shouldn't exist: it shouldn't be
 necessary to look up a channel by the peer's nickname, since nicknames are
 pretty fragile.  (I note that it seems to have no callers.
  * Style issue: you seem to like single-point of return a little more than
 me.  That's fine; but please don't think you need to do it on my account.
 For instance, channel_next_with_digest could probably have its body
 replaced with the two asserts and "return
 chan->u.cell_chan.next_with_same_id"
  * Should listener channels be a different type entirely from cell
 channels? They seem to have very little in common.  IOW, so far in my
 reading, it looks like there aren't a lot of the same operations that it
 makes sense to invoke on one as on another, or a lot of code that makes
 sense to share.  There are also a lot of functions that have to assert
 that the channel_t they got is really one or another type of channel_t,
 which also suggests that using the type system here could be right.  I
 also get this impression from looking at the data structure.
  * By convention, it's always a no-op to pass NULL to any of Tor's
 foo_free() functions. channel_free() doesn't follow this convention.
  * If channel_force_free() should only be called from channel_free_all(),
 should it be static?
  * 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?
  * In channel_force_free(), we tor_free one or more cell_queue_entry_t
 items, which is presumably done elsewhere for cases where we would hit
 channel_free().  Can/should that be extracted into its own function?
  * When we're freeing cell_queue_entry_t items in that function, what
 frees the underlying cell_t/var_cell_t/packed_cell_t ?  Maybe there should
 be a cell_queue_entry_free() function
  * Doing a SMARTLIST_DEL_CURRENT() when freeing items in a list that
 you're about to smartlist_free() is usually unnecessary.
  * re channel_get_listener: For functions that return function pointers, I
 strongly prefer to have a typedef for the function type so as to avoid the
 Trickiest Syntax In C.  Also, we're using the word "listener" here both to
 mean a kind of a channel and a function.  Maybe the function that returns
 the function should be called channel_get_listener_fn() or
 channel_get_listener_cb() or channel_get_listener_handler() or something?
  * Let's eliminate every function to set the channel cell handlers other
 than channel_set_cell_handlers(). That's the only one we currently use,
 and I don't see why you'd need the other two. As they stand, they're
 mostly duplicated code. If we need them later, we can reimplement them by
 calling e.g. channel_set_sell_handlers(chan,
 channel_get_old_cell_handler(chan), new_var_cell_handler);
  * By convention, channel_request_close() seems like what we would name
 channel_mark_for_close.  Is it different enough from the existing
 mark_for_close() functions that we should keep its current name?
  * 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?
  * 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.
  * re channel_write_cell: As I understand it, we have one function here
 that'll want to queue a cell, and another that will want to deliver it.
 From the documentation, I can't tell which one this function is.
  * 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.
  * 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).
  * Probably there should be a cell_queue_entry_new() function [or family
 of functions] to allocate cell_queue_entry_t objects.
  * 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?
  * The channel_write*cell() functions are mostly duplicate code with each
 other.  That needs to get fixed; there should be one implementation
 function here and two-three wrappers, I think.
  * The comment "+      /* That shouldn't happen; bail out */" in
 channel_flush_some_cells_from_outgoing_queue seems orphaned?
  * The swtich statement in channel_flush_some_cells_from_outgoing_queue
 seems like it could stand to be refactored into its own function, maybe
 with some of the common code between the different cases eliminated.

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