Do not mark circs for close again after relay_send_command_from_edge()

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.

changed milestone to %Tor: 0.3.0.x-final

added component::core tor/tor milestone::Tor: 0.3.0.x-final owner::twim points::0.2 priority::medium resolution::implemented review-group-11 severity::minor status::closed type::defect labels

Trac:
0001-Do-not-mark-circs-for-close-again-after-relay_send_c.patch

Trac:
Status: new to needs_review

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?

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.

Trac:
Points: N/A to 0.2
Milestone: N/A to Tor: 0.3.0.x-final

Trac:
Keywords: N/A deleted, review-group-11 added

setting owner

Trac:
Owner: N/A to twim
Status: needs_review to assigned

Trac:
Status: assigned to needs_review

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.

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.

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

Trac:
Status: needs_review to merge_ready

looks okay; merged!

Trac:
Resolution: N/A to implemented
Status: merge_ready to closed

closed

changed time estimate to 1h 36m

moved to tpo/core/tor#20376 (closed)