obfs4proxy not killed when unused

changed milestone to %Tor: unspecified

added 031-deferred-20170425 034-removed-20180328 034-triage-20180328 blog component::core tor/tor milestone::Tor: unspecified points::1.5 priority::medium severity::normal status::new tbb-needs tor-pt type::defect user-feedback labels

Trac:
Cc: N/A to mcs

Trac:
Cc: mcs to mcs, gk

(Thanks asn! We first noticed this bug in #21314 (moved), but it's good to get it its own dedicated ticket here.)

Seems like Tor does not successfuly kill obfs4proxy when it's no longer used. It's unclear why. Maybe obfs4proxy never receives the SIGTERM, or never notices that the stdin is closed?

It's "tor never issues the SIGTERM, or close stdin".

As far as I can tell, the Bridge directives are always parsed, regardless of the UseBridges setting, so if a Bridge entry exists for a transport at all (regardless of it if is used), the PT will always exist.

Removing Bridges with RESETCONF doesn't seem to affect bridge_list at the time when pts would be torn down either.

All of this is crap is half broken on the tor side basically, and it has nothing to do with the process teardown code.

Replying to yawning:

Removing Bridges with RESETCONF doesn't seem to affect bridge_list at the time when pts would be torn down either.

Check out this code in config.c:

  if (options->Bridges) {
    mark_bridge_list();
    for (cl = options->Bridges; cl; cl = cl->next) {
      bridge_line_t *bridge_line = parse_bridge_line(cl->value);
      if (!bridge_line) {
        log_warn(LD_BUG,
                 "Previously validated Bridge line could not be added!");
        return -1;
      }
      bridge_add_from_config(bridge_line);
    }
    sweep_bridge_list();
  }

If we moved the mark and sweep to outside the if, then we would clear out bridge_list in the case where options->Bridges is 0. That seems wise.

Or if we prefered, we could start this stanza with

  if (!options->Bridges) {
    clear_bridge_list();
  } else {
    ...

Replying to arma:

If we moved the mark and sweep to outside the if, then we would clear out bridge_list in the case where options->Bridges is 0. That seems wise.

Compare to how it's done for Transports:

  mark_transport_list();
  pt_prepare_proxy_list_for_config_read();
  if (!options->DisableNetwork) {
    if (options->ClientTransportPlugin) {
      [...]
    }
  }
  sweep_transport_list();
  sweep_proxy_list();

Ok, that turns out to have been the easy part of the fix.

The fix above does indeed sweep unconfigured bridges from the bridge_list.

But the managed proxy still stays running.

That's because the transports and managed proxies get marked:

  mark_transport_list();
  pt_prepare_proxy_list_for_config_read();

but then they (both the transport and the managed proxy) get rescued in pt_kickstart_proxy:

    if (mp->was_around_before_config_read) {
      /* If this managed proxy was around even before we read the
         config this time, it means that it was already enabled before
         and is not useless and should be kept. If it's marked for
         removal, unmark it and make sure that we check whether it
         needs to be restarted. */
      if (mp->marked_for_removal) {
        mp->marked_for_removal = 0;
        check_if_restarts_needed = 1;
      }

      /* For each new transport, check if the managed proxy used to
         support it before the SIGHUP. If that was the case, make sure
         it doesn't get removed because we might reuse it. */
      SMARTLIST_FOREACH_BEGIN(with_transport_list, const char *, transport) {
        old_transport = transport_get_by_name(transport);
        if (old_transport)
          old_transport->marked_for_removal = 0;
      } SMARTLIST_FOREACH_END(transport);
    }

And with comments like that...are we sure this ever worked? It sure seems to be intentional about keeping them around.

Wugh. There's some sort of subtlety in pt_configure_remaining_proxies() where it restarts managed proxies sometimes, and it might implicitly drop transports it doesn't know it needs, or something. See the call to sweep_transport_list() in proxy_prepare_for_restart(). But by then it's too late because pt_kickstart_proxy set mp->marked_for_removal to 0 and nothing ever marks it again.

(Whoever decided that transport_t should have a marked_for_removal flag, and managed_proxy_t should have an identically named marked_for_removal flag, is not a nice person.)

Triage: batch-defer unowned items of priority Medium or lower to 0.3.2.

Trac:
Keywords: N/A deleted, 031-deferred-20170425 added
Milestone: Tor: 0.3.1.x-final to Tor: 0.3.2.x-final

Trac:
Milestone: Tor: 0.3.2.x-final to Tor: 0.3.4.x-final

Trac:
Cc: mcs, gk to mcs, gk, arlolra

Trac:
Keywords: N/A deleted, tbb-wants added

Trac:
Keywords: N/A deleted, 034-triage-20180328 added

Per our triage process, these tickets are pending removal from 0.3.4.

Trac:
Keywords: N/A deleted, 034-removed-20180328 added

These tickets, tagged with 034-removed-*, are no longer in-scope for 0.3.4. We can reconsider any of them, if time permits.

Trac:
Milestone: Tor: 0.3.4.x-final to Tor: unspecified

Prefer the more common tbb-needs to tbb-wants. There doesn't appear to be any difference in how much TBB needs based on the flag.

Trac:
Keywords: tbb-wants deleted, tbb-needs added

Some comments on this bug here: !https://blog.torproject.org/comment/280961#comment-280961

Trac:
Keywords: N/A deleted, blog, user-feedback added

changed time estimate to 12h

moved to tpo/core/tor#21967

obfs4proxy not killed when unused

Child items 0

Activity