[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 I generally agree with/am fine with going
 ahead and adjusting without debate):

 > 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.

 Yeah, will 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.

 Yeah, that's true. I think I meant to fix that and forgot to make a note
 of it.

 > 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.

 Hmm, it doesn't feel all that complex to me because it's a straightforward
 doubly linked list pattern, but I see now that you mention it.  It got
 that
 way because the old version had a singly linked list attached to
 or_connection_t that I moved to channel_t, and then picked up the previous
 link for something I didn't end up using.  Looks like we can simplify some
 of that.

 > 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.

 Yeah, that can go.

 > 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.

 Okay, will change that.

 > If channel_force_free() should only be called from channel_free_all(),
 should
 > it be static?

 Yeah, I think so.

 > 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?

 Yeah, I think so.

 > 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.

 Yeah.

 > Probably there should be a cell_queue_entry_new() function [or family of
 > functions] to allocate cell_queue_entry_t objects.

 Agreed.

 > 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);

 Yeah, okay.

 > I don't see the point of the rv=tmp dance in
 channel_find_by_remote_digest.

 Yeah, me neither. :)

 > The comment "+ /* That shouldn't happen; bail out */" in
 > channel_flush_some_cells_from_outgoing_queue seems orphaned?

 Yeah, looks like it.

 > 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.

 Yeah, I had a bit of the same sense myself, so if you saw it the same way
 it
 should probably get changed.

 > Doing a SMARTLIST_DEL_CURRENT() when freeing items in a list that you're
 > about to smartlist_free() is usually unnecessary.

 Okay.

 > 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?

 Okay.

 > 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.

 Okay; I'm fine with either.

 > 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"

 Okay, sure.

 > 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?

 Yeah, maybe better to be consistent there.

 > 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.

 Okay.

 > 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.

 Okay, I should make that clearer then.

 > 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.

 Yeah, that is kinda hairy.

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