Opened 3 years ago

Closed 3 years ago

#20376 closed defect (implemented)

Do not mark circs for close again after relay_send_command_from_edge()

Reported by: twim Owned by: twim
Priority: Medium Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version:
Severity: Minor Keywords: review-group-11
Cc: Actual Points:
Parent ID: Points: 0.2
Reviewer: Sponsor:

Description

Since relay_send_command_from_edge() already does mark failed circs for close there is no reason to do it again.
Please see the patch attached or branch duplicate-mark-for-close at https://gitlab.com/nogoegst/tor.

Child Tickets

Attachments (1)

0001-Do-not-mark-circs-for-close-again-after-relay_send_c.patch (2.9 KB) - added by twim 3 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 3 years ago by twim

Status: newneeds_review

comment:2 Changed 3 years ago by twim

Hmm, haven't noticed before that relay_send_command_from_edge_() also can return non-zero value here:

  if (cpath_layer) {
    cell.circ_id = circ->n_circ_id;
    cell_direction = CELL_DIRECTION_OUT;
  } else if (! CIRCUIT_IS_ORIGIN(circ)) {
    cell.circ_id = TO_OR_CIRCUIT(circ)->p_circ_id;
    cell_direction = CELL_DIRECTION_IN;
  } else {
    return -1;
  }

Should it mark such circ to close here?

comment:3 Changed 3 years ago by twim

Only OR-side circs should be filtered out here. So relay_send_command_from_edge() should not be called on such circuits, right? If yes, there should be an assertion instead of return -1.

comment:4 Changed 3 years ago by asn

Milestone: Tor: 0.3.0.x-final
Points: 0.2

comment:5 Changed 3 years ago by nickm

Keywords: review-group-11 added

comment:6 Changed 3 years ago by nickm

Owner: set to twim
Status: needs_reviewassigned

setting owner

comment:7 Changed 3 years ago by nickm

Status: assignedneeds_review

comment:8 Changed 3 years ago by dgoulet

Hey thanks for this twim! Good find!

So I had to think hard about this one as it has quite a bit of implication especially when adding an assert like that. I've refactored your branch slightly (and fixing couple things) in this branch: bug20376_030_01.

The main part you'll notice is the change to relay_send_command_from_edge_() which I turned into a simple if/else (no unknown else condition anymore) where we either have a origin circuit or not and depending on that, we set the direction and assert on the cpath_layer which MUST be present if we are outbound. As far as I understand it, that function only does OP->OR or OR->OP meaning it can't be a middle hop and thus if it's an origin circuit, it must be outbound and have a cpath else we are using that function very wrong.

Plausible?

The branch passes make test-network-all.

comment:9 in reply to:  8 Changed 3 years ago by twim

Replying to dgoulet:

As far as I understand it, that function only does OP->OR or OR->OP meaning it can't be a middle hop and thus if it's an origin circuit, it must be outbound and have a cpath else we are using that function very wrong.

Just checked your branch. Thanks, the condition now looks much better! Comments and fixed callsites look good to me also.

comment:10 Changed 3 years ago by dgoulet

Status: needs_reviewmerge_ready

Ok I'll put this one in merge_ready as two of us already looked at it. Thanks!

comment:11 Changed 3 years ago by nickm

Resolution: implemented
Status: merge_readyclosed

looks okay; merged!

Note: See TracTickets for help on using tickets.