#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