[tor-bugs] #28780 [Core Tor/Tor]: circpadding: Add machine flag for not closing circuit if machine is active

[Tor Bug Tracker & Wiki]

#28780: circpadding: Add machine flag for not closing circuit if machine is active
-------------------------------------------------+-------------------------
 Reporter:  asn                                  |          Owner:  (none)
     Type:  defect                               |         Status:
                                                 |  needs_information
 Priority:  Very High                            |      Milestone:  Tor:
                                                 |  0.4.1.x-final
Component:  Core Tor/Tor                         |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  wtf-pad, tor-relay, tor-cell,        |  Actual Points:  6
  padding, 041-proposed, network-team-           |
  roadmap-2019-Q1Q2                              |
Parent ID:  #28634                               |         Points:  5
 Reviewer:  asn                                  |        Sponsor:
                                                 |  Sponsor2
-------------------------------------------------+-------------------------

Comment (by asn):

 Replying to [comment:30 mikeperry]:
 > Can we back up and discuss the root reasons why you feel this approach
 risky? My read of the above is that your three concerns are: code clarity,
 memory leaks, and purpose changes. Is that right?
 >

 Yes, I think these are legitimate reasons to worry about. I think another
 worry is that because of overriding `mark_for_close()` we might get into a
 situation where the body of the function actually never gets called, and
 circuits stay alive for ever. I find this worry reasonable because
 `circpad_circuit_should_be_marked_for_close()` is a non-trivial function
 and we should make sure it does not permanently block a circuit from
 getting closed. Given that we have no functional machine right now,
 testing that this code works as intended in the real network is hard and
 hence this worry is even more reasonable.

 That said, I'm also not sure if Nick's suggested solution of using an
 intermediate function `circuit_transition_to_shutdown()` is good enough.
 As has been said, `mark_for_close()` is a function that is consumed all
 over the codebase and by every developer, and if we introduce our own
 intermediate function this means that all developers need to be aware of
 when the intermediate function should be used instead of
 `mark_for_close()`. I'm already having trouble thinking about where
 `mark_for_close()` should be replaced by `transition_to_shutdown()` in the
 current codebase, let alone having all developers keep this in mind for
 ever in the future. That's why I think having the circuitpadding subsystem
 keep track of this might make more sense, in a similar way that the
 pathbias subsystem uses `pathbias_check_close()` to track the same thing.

 ----

 Here is a **suggestion** of how to meet in the middle here. We keep the
 current logic, but we also add an assert-like function that checks whether
 the invariants here are preserved as we wish them to be. We can call that
 function from `assert_circuit_ok()` and also from
 `circuit_expire_old_circuits_clientside()` and we make sure that if
 something is going wrong it warns loud enough that we get bug reports and
 notice it quickly.

 Here is an example of a scenario that we should be checking: "If a circuit
 is alive and has a machine that manages its own lifetime, the machine
 should still be running, or if has ENDed and the circuit must have not
 expired yet.". Or, to catch errors that would let vanilla circuits never
 get marked for close, we could add an extra debug-field to a circuit that
 gets set in the beginning of `mark_for_close()` and we make sure that a
 circuit cannot linger around with that field if it does not have a machine
 that manages its own lifetime". These are just quick examples: to make
 good ones, we should think of the behavior we want from our circuits, and
 come up with the right invariants to preserve those behaviors.

 What do you say?

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/28780#comment:31>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online