#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
_______________________________________________ tor-bugs mailing list tor-bugs@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs