Re: [tor-bugs] #5084 [Tor Bridge]: pt_prepare_proxy_list_for_config_read: Assertion unconfigured_proxies_n == 0 failed

["Tor Bug Tracker & Wiki" <torproject-admin@xxxxxxxxxxxxxx>]

#5084: pt_prepare_proxy_list_for_config_read: Assertion unconfigured_proxies_n ==
0 failed
------------------------+---------------------------------------------------
 Reporter:  arma        |          Owner:                    
     Type:  defect      |         Status:  new               
 Priority:  major       |      Milestone:  Tor: 0.2.3.x-final
Component:  Tor Bridge  |        Version:                    
 Keywords:              |         Parent:                    
   Points:              |   Actualpoints:                    
------------------------+---------------------------------------------------

Comment(by asn):

 OK, as you have noticed, the SIGHUP code of src/or/transports.c is not
 elegant. If you can find a better logic for it, I'd be very glad. The
 current logic is the best I could think of when I was writing it.

 This bug has to do with the way we count unconfigured proxies between
 config reads.

 In the first SIGHUP, the managed proxy is already launched and configured.
 During the SIGHUP, we re-parse the config, find the proxy line and give it
 to  pt_kickstart_proxy()`. In `pt_kickstart_proxy()`, we go into the
 {{{
 } else { /* known proxy. add its transport to its transport list */
 }}}
 branch, since we've already parsed this managed proxy when we launched
 tor. The proxy was there from before the SIGHUP, so it's already marked
 with `got_hup` and `marked_for_removal`. So we go into the next two `if`
 blocks and end up incrementing `unconfigured_proxies_n`. We need to do
 that, so that in the next `run_scheduled_events()` tick, we execute
 `pt_configure_remaining_proxies()` and examine if the proxy needs to be
 restarted or not. In this case, the proxy wouldn't need to be restarted,
 `proxy_needs_restart()` would have returned False, and
 `unconfigured_proxies_n` would have been reduced back to 0. Meanwhile,,
 the proxy has been in the `PT_PROTO_COMPLETED` state, since it was spawned
 and configured back when we launched tor.

 So in this bug, we never do another tick of `run_scheduled_events()`, and
 instead we do a second SIGHUP. This means that we re-read the config, and
 call `pt_prepare_proxy_list_for_config_read()`. In this code,
 `unconfigured_proxies_n == 1` from the previous SIGHUP, and since the
 proxy is in `PT_PROTO_COMPLETED` we don't decrement
 `unconfigured_proxies_n`. Instead, we reach the bottom of the function and
 hit the assert with `1 != 0`.

 ----

 Notice how `unconfigured_proxies_n` is only actually used in
 `pt_proxies_configuration_pending()`, to see whether we should configure
 managed proxies in `run_scheduled_events()`. It's also used in some logs,
 and to assert that the code does what I expect it to do.

 My original stupid fix idea, was to remove `unconfigured_proxies_n--;`
 from `pt_prepare_proxy_list_for_config_read()` and simply set
 `unconfigured_proxies_n` to 0 in the end of the function, since we are
 planning on re-parsing the config file anyway. That might have worked.

 Maybe a cleaner solution, would be to remove the `unconfigured_proxies_n`
 logic completely, and instead use a boolean, named `need_to_configure_pt`
 or something, for  pt_proxies_configuration_pending()`. This might be
 cleaner, and still not too radical.

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