Opened 6 years ago

Closed 6 years ago

#12191 closed defect (fixed)

command_process_create_cell: calls channel_send_destroy() for already exist circuit

Reported by: cypherpunks Owned by:
Priority: Medium Milestone: Tor: 0.2.5.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay 025-triaged
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

command_process_create_cell tries to check several conditions and can to call channel_send_destroy for already exist circuit. channel_send_destroy is more complex mechanism than sending of some cell as it happened before.
It shouldn't to call channel_send_destroy if circ_id already exist for chan. It should to check circ_id and drop if already exist and only then to check all another stuff.

Child Tickets

Change History (14)

comment:1 Changed 6 years ago by nickm

Milestone: Tor: 0.2.5.x-final

comment:2 Changed 6 years ago by nickm

Keywords: 024-backport added

comment:3 Changed 6 years ago by nickm

Keywords: 024-backport removed

Oh wait, we never backported that code, so this is 0.2.5 only.

comment:4 Changed 6 years ago by nickm

(I'm guessing that you found this while investigating #12184 . This looks like a real bug to me, but I don't think it is the cause of #12184. Do you?)

comment:5 Changed 6 years ago by cypherpunks

It's harmless for current code, all function making circuit usable and unusable should still to works correctly. It's just wrong to send destroy cell back for this case.

comment:6 Changed 6 years ago by nickm

Keywords: tor-relay 025-triaged added

comment:7 Changed 6 years ago by nickm

There's a corner-case here: what to do if we get a CREATE cell for a circuit on which we have a pending DESTROY.

(I don't think that should be possible in correct operation, since if we have a pending DESTROY to send towards the client, we have not yet processed a DESTROY from the client, and so the client should still believe the circuit is there and not send us a CREATE cell for it.)

comment:8 Changed 6 years ago by cypherpunks

There's a corner-case here: what to do if we get a CREATE cell for a circuit on which we have a pending DESTROY.

Dropping of cell is correct behavior. The one circuit is the one create cell and the one destroy cell that was received or sent. It's sad if another party can't to get any answer for such corner cases but broken logic for code where we sends several destroy cells is worse solution.

Last edited 6 years ago by cypherpunks (previous) (diff)

comment:9 Changed 6 years ago by nickm

Status: newneeds_review

Possible patch in "bug12191".

comment:10 Changed 6 years ago by cypherpunks

What about "cell->circ_id == 0". Sending DESTROY cell with zero id have no sense too.

comment:11 Changed 6 years ago by nickm

Added another commit to the branch. Does it look good now?

comment:12 Changed 6 years ago by cypherpunks

Why this fix changes order of checking for zero circ_id and for already used circ_id? What purpose to check if zero circ_id used already instead of dropping cell with zero circ_id as before, if relay receives such broken request?

Last edited 6 years ago by cypherpunks (previous) (diff)

comment:13 Changed 6 years ago by nickm

I don't think the order makes a difference in this case. The original order probably makes more sense, though. Any other issues?

comment:14 Changed 6 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Assuming not. Still looks ok to me. Re-ordered, squash-rebased, merging.

Note: See TracTickets for help on using tickets.