[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [tor-bugs] #3656 [Tor Bridge]: Support spawning multiple protocols using the same managed proxy

#3656: Support spawning multiple protocols using the same managed proxy
------------------------+---------------------------------------------------
 Reporter:  asn         |          Owner:  asn         
     Type:  defect      |         Status:  needs_review
 Priority:  normal      |      Milestone:              
Component:  Tor Bridge  |        Version:              
 Keywords:              |         Parent:              
   Points:              |   Actualpoints:              
------------------------+---------------------------------------------------

Comment(by asn):

 === What I fixed: ===
  * there is still an assert() or two that should be tor_assert().
  * I think there are some c99isms that snuck in; try building with
 --enable-gcc-warnings
  * connection_uses_transport: I kinda want to have this look  at a field
 that's set at connection-launch time, so that if the set of  transports or
 bridges changes between when we launch the connection and  when we check,
 we will still get the same answer every time we check.
  * Please double-check that the argv pointers we pass to
 pt_managed_launch_client_proxy actually get freed someplace?  Maybe try
 running this whole thing under valgrind to look for leaks; see
 doc/HACKING for info how.
  * get_transport_in_state_by_name will fail badly if there are two
 transports A and B such that A's name is a prefix of B's.


  * You shouldn't need to be messing with this STRUCT_VAR_P stuff and
 config_find_option stuff if you know the name of the field you want to
 access -- just use C and say "get_or_state()->!TransportProxies"[[BR]]
  * get_transport_bindaddr should probably check that the line actually
 starts with the name of the transport and a space.
  * When in doubt, prefer tor_addr_t
  * smartlist_clear() before smartlist_free() is redundant.
  * This design has gotten complicated enough that we could  probably use a
 big comment someplace explaining the relationship between  proxies,
 transports, and bridges; who manages which when; how stuff  gets launched
 and checked; how stuff gets stopped; etc

 === What I questionably fixed: ===
  * log_warn() is only for problems; never for things that are working as
 expected.

 I sanitized the logging severities. I didn't think about it too much and
 some of them might be spammy in real usage. I tried to keep LD_WARN to the
 minimum and mainly use LD_{INFO,NOTICE}.

  *
 managed_proxy_has_argv/managed_proxy_get_by_argv/add_transport_to_proxy/get_bindaddr_for_proxy:
 constify if they're not already constified.

 I try to pay attention to constification and e8715b30 ended up being quite
 empty. Stuff like `get_managed_proxy_by_argv_and_type()!` that I would
 like to constify, give me a compiler warning if I do. (`proxy_argv` is not
 const in !`pt_kickstart_proxy`).

  * make check-spaces ; and make sure that --enable-gcc-warnings works

 I'm getting an: EOL@EOF:src/or/config.c:5995 warning in check-spaces. I
 tried to remove the EOL with three different editors and I couldn't. I
 gave up after a bit.

  * Have a look at the new functions added to .h files through  this whole
 series.  The ones that are only used in the file that  declares them
 should be static functions in that file.  The ones that  are only used in
 the file that that declares them, plus in the  unit  tests, should use the
 #ifdef FOO_PRIVATE trick

 I also try to pay attention to static functions and I couldn't find
 anything other than !`get_transport_bindaddr()`. It wouldn't surprise me
 '''at all''' if I had forgotten a function or three though, so if you have
 anything in mind do say.

  * Try this stuff under valgrind; there are enough new things allocated in
 one place and freed in another that I'm a little unconfident that I
 actually checked everything completely.  (valgrind instructions in
 doc/HACKING are a must)

 Did that a hundred times with different torrc etc. I plugged all memleaks
 I managed to find, but some allocation are quite complex (and there are
 many edge cases as well) and I'm still afraid I might have left memleaks
 lying around...

  * Please don't commit prints

 I always forget you are reviewing by reading diffs and I forget prints
 inside. Sorry. I killed all stray printf()s in 2e73f9b3.

  * If I HUP tor to reload the configuration, do I wind up  re-launching
 all the proxies?  I didn't see the code that prevents this  from
 happening.

 Alright, this was a hard one. I had to hunt for edge cases all around the
 code and I didn't like it. I'm not very satisfied with the results but I
 couldn't find a better way to do it. It should detect all torrc
 modifications and react accordingly '''except''' when external and managed
 proxies are mixed together. More on this later.

 === What I didn't fix: ===
  * would a comma-separated list of transports be easier than  multiple
 lines here?  I'm not sure the behavior is intuitive as it is.   Is this
 how we designed it?

 It's true, comma-seperated transports will probably be more intuitive.
 It's in my TODO list.

  *  validate_transports_in_state doesn't do what it's documented to do: It
 never returns -1

 Hm, I'm not sure what I should be doing in the case state transports are
 not sane. I remember you saying that the state transports format should be
 more extensible and that we shouldn't trash the state for now. So, I'm
 just issuing an LD_WARN to the user and leaving the state as is.

  *  For save_transport_to_state, would it be simpler to just clear the
 entire !TransportProxies field, and re-add *every* transport?

 I'm not sure, it could be simpler indeed. I didn't fix this because I
 would have to figure out when a transport is a server transport and
 !`transport_t`s can't do that yet (very easy to implement thought).

 I'm also semi-confidentâ that the code logic is correct even thought it
 looks like spaggheti. This probably goes to the TODO list as well.

 == BUGS ==
 There is a bug that I haven't fixed yet. When there is a transport that is
 supported by a managed proxy *and* an external proxy, the code will always
 register the external proxy version, even if the managed proxy is higher
 in the torrc.

 This happens because managed proxies are configured in several
 !`run_scheduled_events()` ticks, while external proxies are configured in
 parse_client_transport_line(). This means that external proxy transports
 hit the circuitbuild.c subsystem instantly, when managed proxies will
 conflict when they try to add their transports later on. Because of the
 first-come-first-served transport registration, external proxy transports
 will always be prefered over managed proxy transports.

 I didn't fix this one because the fix will put even more edge case
 catching in the code and my heart is not prepared for that just yet. I
 would probably have to introduce 'priorities' to the transports based on
 their position in torrc. I would then have to respect that priority every
 time a transport tries to get registered in circuitbuild.c. That priority
 will also have to be respected in the case of a HUP, etc.

 == TODO ==
 We probably have to introduce a LOG_PT and put the transports.c etc.
 logging in that domain.

 We probably also have to introduce a unique identifier to every managed
 proxy so that logs become a bit more meaningful.

 ==  ==
 ==  ==

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/3656#comment:6>
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