[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #28780 [Core Tor/Tor]: circpadding: Add machine flag for not closing circuit if machine is active
#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@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs