[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