Opened 5 months ago

Closed 3 months ago

#32021 closed defect (fixed)

hs-v3: Handle rendezvous client circuit build expire properly

Reported by: dgoulet Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs, tor-client, tor-circuit
Cc: Actual Points:
Parent ID: #30200 Points: 0.4
Reviewer: asn Sponsor: Sponsor27-must

Description

This is a subtask of the bigger larger problem in #25882.

In circuit_expire_building(), we have this code path:

    if (!(TO_ORIGIN_CIRCUIT(victim)->hs_circ_has_timed_out)) {
      switch (victim->purpose) {
      case CIRCUIT_PURPOSE_C_REND_READY:
        /* We only want to spare a rend circ if it has been specified in
         * an INTRODUCE1 cell sent to a hidden service.  A circ's
         * pending_final_cpath field is non-NULL iff it is a rend circ
         * and we have tried to send an INTRODUCE1 cell specifying it.
         * Thus, if the pending_final_cpath field *is* NULL, then we
         * want to not spare it. */
        if (TO_ORIGIN_CIRCUIT(victim)->build_state &&
            TO_ORIGIN_CIRCUIT(victim)->build_state->pending_final_cpath ==
            NULL)
          break;

Basically, this pending_final_cpath is only used by v2 which means v3 is not handle in that case.

And that case is: if we want to expire a rendezvous client circuit that is ready but has been waiting for a while on the introduction circuit as in its cookie has been sent in the INTRODUCE1, we want to spare it until the intro point client circuit collapses.

Because v3 is not handled in the above, rendezvous circuit will be tagged as timed out with the general cutoff instead of being kept until the intro circuit is ready or times out. And we time out intro circuit being established much later than an established rendezvous circuit for which the general_cutoff will be applied on.

Bottom line is that we need a flag within the rendezvous client circuit (probably hs_ident_t?) that its cookie was put in the INTRO1 cell and that we are waiting on the intro side signalling the circuit_expire_building() that it should wait more on that circuit.

Child Tickets

Change History (10)

comment:1 Changed 4 months ago by dgoulet

Owner: set to dgoulet
Status: newassigned

comment:2 Changed 3 months ago by dgoulet

Status: assignedneeds_review

Branch: ticket32021_043_01
PR: https://github.com/torproject/tor/pull/1581

comment:3 Changed 3 months ago by asn

Hmm, logic seems sound.

Just a suggestion: While we are at it, should we make a new function to host the body of if (!(TO_ORIGIN_CIRCUIT(victim)->hs_circ_has_timed_out)) {? We can put it in the HS subsystem so that the logic is contained in there.

comment:4 Changed 3 months ago by asn

Status: needs_reviewneeds_revision

comment:5 in reply to:  3 ; Changed 3 months ago by dgoulet

Replying to asn:

Hmm, logic seems sound.

Just a suggestion: While we are at it, should we make a new function to host the body of if (!(TO_ORIGIN_CIRCUIT(victim)->hs_circ_has_timed_out)) {? We can put it in the HS subsystem so that the logic is contained in there.

Do you mean add something like hs_circ_has_timed_out() type of function and return true or false for a circuit?

comment:6 in reply to:  5 Changed 3 months ago by asn

Replying to dgoulet:

Replying to asn:

Hmm, logic seems sound.

Just a suggestion: While we are at it, should we make a new function to host the body of if (!(TO_ORIGIN_CIRCUIT(victim)->hs_circ_has_timed_out)) {? We can put it in the HS subsystem so that the logic is contained in there.

Do you mean add something like hs_circ_has_timed_out() type of function and return true or false for a circuit?

Yeah something like that. Perhaps we can call it hs_circ_should_be_flagged_as_timed_out() given the semantics of the function.

What do you think?

comment:7 Changed 3 months ago by dgoulet

Status: needs_revisionneeds_review

See fixup commit. I'm open to a better function name... it is quite verbose but it is super specific as well.

comment:8 in reply to:  7 Changed 3 months ago by asn

Status: needs_reviewneeds_revision

Replying to dgoulet:

See fixup commit. I'm open to a better function name... it is quite verbose but it is super specific as well.

Hmm, I like the code logic but not a big fan of the function name (especially with the lack of function doc)

Suggestion: Perhaps we dont need to expose a low-level function name like hs_circ_is_rend_sent_in_intro1() to circuituse.c. Perhaps we can go with a high-level function name like hs_circ_rend_should_not_time_out() and explain the low-level stuff in the function itself.

In any case, please add a function doc for the function, and consider adapting the function name if you like the idea above.

comment:9 Changed 3 months ago by dgoulet

Status: needs_revisionneeds_review

I don't think going the hs_circ_rend_should_not_time_out() is correct.

The function I added does one and only one thing. The "not time out" needs to do way more things especially based its decision on the purpose. We would need to untangle circuit_expire_building() to create a high level "has timed out" function for that...

I agree the name is not so good but I'm quite certain we can not call it "timed out".

I added the documentation in a fixup commit.

comment:10 Changed 3 months ago by asn

Resolution: fixed
Status: needs_reviewclosed

Sounds good! Merged! Thanks!

Note: See TracTickets for help on using tickets.