[tor-bugs] #2841 [Tor Bridge]: Implementing the pluggable-transport spec

[Tor Bug Tracker & Wiki]

#2841: Implementing the pluggable-transport spec
------------------------+---------------------------------------------------
 Reporter:  asn         |          Owner:  asn               
     Type:  task        |         Status:  needs_review      
 Priority:  normal      |      Milestone:  Tor: 0.2.3.x-final
Component:  Tor Bridge  |        Version:                    
 Keywords:              |         Parent:                    
   Points:              |   Actualpoints:                    
------------------------+---------------------------------------------------

Comment(by nickm):

 First thoughts:

 In circuitbuild.c

 * Lots of your doxygen comments don't match Tor's style.  Look at how the
 others are formatted.

 * By convention, our *_free() functions check for NULL and do nothing if
 called with NULL as an argument.

 * In general, it's not the best idea to call structures "foo_info_t" or
 "foo_data_t".  After all, this is a computer program: everything is
 information! everything is data!  Let's just call it foo_t.  (Yes, I know
 that Tor already violates this in places like bridge_info_t.  I think
 that's a mistake.)

 * It looks like you could use a "transport_get_by_name()" function that
 transport_add_from_config and match_bridges_with_transports() could use.

 * I don't think it should be an error to have a bridge listed with a
 corresponding transport configured: it makes the bridge unusable, sure,
 but we can still use the other bridges.

 * Why give a warning and reject the configuration if we have transports
 configured without corresponding bridges?  That seems acceptable to me:
 just because I know about a transport doesn't mean I should have to use
 it.

 * Having bridge_info_t keep a pointer to the transport_(info_)t seems
 potentially risky if there's a chance we'll free the transport before we
 free the bridge.  Even if that can't happen now, it seems needlessly
 fragile.

 * The return convention mostly used in Tor is 0 on success, -1 on failure.
 Not 1 on success, -1 on failure.

 * English possessive "its" is "its"; "it's" is a contraction for "it is".
 Sorry; I didn't make it up.

 * Successful, not successfull.

 * The check for duplicate names bridge_add_from_config seems very wrong:
 it is totally okay for two bridges to use the same transport, right?

 * clear_transport_list should probably set transport_list to NULL.

 In config.c:

 * In options_act(), it looks like we only call clear_transport_list() if
 ClientTransportPlugin is set.  Consider what this will do if we have some
 transports configured, and then we remove all the configured transports.
 It seems that all the old transports will remain configured.

 * Never assert(); always tor_assert().

 * You call match_bridges_with_transports() only if both Bridges and
 ClientTransportPlugin are set.  This means that if the user sets up a
 bunch of Bridges with transports but sets no ClientTransportPlugins, the
 code will never notice that the bridges have have their transports
 missing. ..... (Oh, wait.  Later on I see that you check for this case in
 parse_bridge_line.  But that seems redundant:
 match_bridges_with_transports() already does this test for you.)

 * In the"REALLY ugly" check, how about something like:
 {{{
   if ((options->Socks4Proxy != NULL) + (options->Socks5Proxy != NULL) +
       (options->HTTPSProxy != NULL) + (options->ClientTransportPlugins !=
 NULL) > 1)
 }}}
 ?

 * Do we want to actually have ClientTransportPlugin exclusive with all
 other proxy settings?

 * By convention, we try not to do:
 {{{
    if (x) {
      a;
    } else
      b;
 }}}
   That is, if one case is bracketed, we should bracket both cases.

 * It looks like the log_debug in parse_bridge_line will log transport_name
 even if transport_name is NULL; that's not kosher.

 In connection.c:

 * Looks like you forgot to run "make check-spaces".

 * In connection_or_connect: we set tor_addr_t values using tor_addr_copy,
 not =.

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