[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [tor-bugs] #18873 [Core Tor/Tor]: Refactor circuit_predict_and_launch_new()



#18873: Refactor circuit_predict_and_launch_new()
--------------------------+------------------------------------
 Reporter:  asn           |          Owner:
     Type:  defect        |         Status:  needs_review
 Priority:  Low           |      Milestone:  Tor: 0.3.0.x-final
Component:  Core Tor/Tor  |        Version:
 Severity:  Normal        |     Resolution:
 Keywords:  refactoring   |  Actual Points:
Parent ID:                |         Points:
 Reviewer:  dgoulet       |        Sponsor:
--------------------------+------------------------------------

Comment (by teor):

 Replying to [comment:9 teor]:
 > Replying to [comment:8 chelseakomlo]:
 > > ...
 > > I added a separate commit for the refactor to remove the check for
 CIRCUIT_IS_ORIGIN in circuit_is_available_for_use. The refactor is because
 we first check the purpose of the circuit to see if it is an origin
 circuit, but then we later reject all cicuits that do not have the purpose
 CIRCUIT_PURPOSE_C_GENERAL.
 > >
 > > We shouldn't need to use the BUG macro because
 circuit_is_available_for_use should be able to accept both origin and non-
 origin circuits, and silently return if the circuit's purpose is not
 CIRCUIT_PURPOSE_C_GENERAL (which I believe also entails that it is an
 origin circuit).
 > >
 > > `#define CIRCUIT_PURPOSE_IS_ORIGIN(p) ((p)>CIRCUIT_PURPOSE_OR_MAX_)`
 > > `#define CIRCUIT_PURPOSE_OR_MAX_ 4`
 > > `#define CIRCUIT_PURPOSE_C_GENERAL 5`
 > >
 > > If this is the case, we shouldn't be able to crash in
 TO_ORIGIN_CIRCUIT() as all circuits that reach this point will have the
 purpose CIRCUIT_PURPOSE_C_GENERAL and therefore (I think) are origin
 circuits.
 >
 > I think you are confusing CIRCUIT_PURPOSE_IS_ORIGIN() and
 CIRCUIT_IS_ORIGIN().

 Oops, no, I am confusing them:

 CIRCUIT_PURPOSE_IS_ORIGIN() and CIRCUIT_IS_ORIGIN() are equivalent - one
 tests the purpose itself, the other tests the circuit.

 But TO_ORIGIN_CIRCUIT does `tor_assert(x->magic ==
 ORIGIN_CIRCUIT_MAGIC);`, and does not check the purpose. So it is not
 equivalent to either of those other macros.

 > And while I agree that is the current logic, I have seen too many cases
 like this where calling code breaks an assumption, and then the called
 code crashes.

 In particular, the assumption that if a circuit satisfies `purpose ==
 CIRCUIT_PURPOSE_C_GENERAL`, it will also have the right magic number.

 But in the end, I think you're right, because assert_circuit_ok() checks
 that invariant. So we can skip that check.

--
Ticket URL: <https://troodi.torproject.org/projects/tor/ticket/18873#comment:10>
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