[tor-bugs] #3613 [Pluggable transport]: obfsproxy: groundwork for new protocols

[Tor Bug Tracker & Wiki]

#3613: obfsproxy: groundwork for new protocols
---------------------------------+------------------------------------------
 Reporter:  zwol                 |          Owner:  asn         
     Type:  enhancement          |         Status:  needs_review
 Priority:  normal               |      Milestone:              
Component:  Pluggable transport  |        Version:              
 Keywords:                       |         Parent:              
   Points:                       |   Actualpoints:              
---------------------------------+------------------------------------------

Comment(by nickm):

 Looking through the branch with asn now...

 As a general issue, it's best to split code changes from moving things
 around.  (I think I already mentioned this and am just looking at older
 commits here, but just in case I should say it again.

 Try to use the standard git commit message format where the first line is
 a *short* summary of what the commit does.  I'm not insisting on a strict
 60-char limit or whatever the stricter projects are using, but having the
 first line of the commit msg  span more than one 80-char line is a little
 much.

 It looks like after your assert() changes, there are some bare abort()s in
 logging (maybe elsewhere too?)  It's better to use assert(0) or
 assert(0==1) so that we get a message printed with the line number.  Also,
 maybe log_error() ought to abort() rather than exit(1), so that we can get
 stack traces.

 I don't like how your patch that allegedly was going to change from DLL to
 smartlist also removed stuff like memory-poison-before-free and removed
 calls to listener_free() [replacing it with a partial implementation
 inline].  What's the point of that?  Why not just call listener_free to
 free a listener?

 Using smartlist_remove() to remove a connection from a list is a step
 backwards.  That's an O(N) operation since it needs to do a linear search
 over all the connections.  I guess it won't happen often, though.

 Also, close_conn probably wants to be named close_and_free_conn() or
 something to make it clear that the connection is now unexistent after
 calling it.

 The new asserts in input_event_cb() are dangerous.  Libevent is allowed to
 add new event types in the future; if we get a BEV_EVENT_FOOBAR, we should
 just ignore it, not die with an assertion.  The other event handling
 functions have a similar problem.

 I think the next step is to review all the new callbacks logic and to try
 it out a bit, clean up the major issues from above, then merge.

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