[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #2841 [Tor Bridge]: Implementing the pluggable-transport spec
#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 asn):
Replying to [comment:9 nickm]:
> First thoughts:
>
> In circuitbuild.c
>
> * Lots of your doxygen comments don't match Tor's style. Look at how
the others are formatted.
>
Done! (I think I caught them all)
> * By convention, our *_free() functions check for NULL and do nothing if
called with NULL as an argument.
>
Done!
> * 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.)
>
Done!
> * It looks like you could use a "transport_get_by_name()" function that
transport_add_from_config and match_bridges_with_transports() could use.
>
Done!
> * 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.
>
Alright I'll handle these three comments together.
I removed the transport_t pointer from the bridge_info_t. I'm now using
transport_get_by_name() to get the bridge's transport instead of using
bridge->transport.
This change though, essentially makes match_bridges_with_transports()
semi-useless. Do you think we should still keep
match_bridges_with_transports() around so that it warns the user of
configuration anomalies (like the ones you mentioned in those two comments
above)?
I'm not sure what the correct course of action is, when the user gives
Bridge lines with methods without corresponding ClientTransportPlugin
lines (or the opposite.).
I was thinking that it would probably be user misconfiguration (typo or
forgetting to add a line) and that's why I was erroring out.
I don't have too strong feelings about this, but there are some small
things we should take care off if we don't error out.
For example with the current code, if the user gives a Bridge line with a
method without a ClientTransportPlugin line, tor attempts to connect to
the Bridge addrport directly thinking that there is no pluggable transport
for this bridge.
This happens because find_transport_by_bridge_addrport() returns either
the transport or NULL, and would return NULL on the above case. Do you
think this is a case we should be handling with something more than a
warning?
> * The return convention mostly used in Tor is 0 on success, -1 on
failure. Not 1 on success, -1 on failure.
>
Done!
> * English possessive "its" is "its"; "it's" is a contraction for "it
is". Sorry; I didn't make it up.
>
> * Successful, not successfull.
>
Your right! Done!
> * 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?
>
Oh my, you are right! Done!
> * clear_transport_list should probably set transport_list to NULL.
>
What do you mean? clear_transport_list() just initiates transport_list,
like clear_bridge_list() does for bridge_list. It doesn't free it,
transport_list is a static smartlist just like bridge_list.
> 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.
>
Oh you are right! Shouldn't we do the same thing for Bridges?
(mark_bridge_list() and sweep_bridge_list() are only called if Bridges is
set)
> * Never assert(); always tor_assert().
>
Done!
> * 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.)
>
I have removed match_bridges_with_transports() for now.
> * In the"REALLY ugly" check, how about something like:
> {{{
> if ((options->Socks4Proxy != NULL) + (options->Socks5Proxy != NULL) +
> (options->HTTPSProxy != NULL) + (options->ClientTransportPlugins
!= NULL) > 1)
> }}}
> ?
>
Oh great, thank you!
> * Do we want to actually have ClientTransportPlugin exclusive with all
other proxy settings?
>
I'm not sure. The current code won't work if ClientTransportPlugin is not
exclusive with all other proxy settings, and I was considering that a
feature.
If you think that it shouldn't be exclusive, we should change that.
> * By convention, we try not to do:
> {{{
> if (x) {
> a;
> } else
> b;
> }}}
> That is, if one case is bracketed, we should bracket both cases.
>
Done!
> * It looks like the log_debug in parse_bridge_line will log
transport_name even if transport_name is NULL; that's not kosher.
>
Done!
> In connection.c:
>
> * Looks like you forgot to run "make check-spaces".
>
Done!
> * In connection_or_connect: we set tor_addr_t values using
tor_addr_copy, not =.
>
Done!
[The above message was written while I was having a terrible headache. If
something doesn't make sense; poke me and I shall rewrite it.]
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/2841#comment:10>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs