Opened 9 years ago

Last modified 2 years ago

#1784 new defect

RELAY_TRUNCATE should give cell queues a chance to flush

Reported by: nickm Owned by:
Priority: Low Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-relay protocol RELAY_TRUNCATE queues
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Since we introduced cell queues, truncating a circuit has meant that cells pending on the n_cell_queue would likely get dropped when the RELAY_TRUNCATE cell was first received. This isn't very useful behavior: if you sent some data cells then a relay_end then a truncate, you almost certainly wanted the data to arrive.

Fortunately, Tor doesn't (AFAIK) use TRUNCATE like this today. But it would be a good thing to fix.

The implementation could be a little tricky here, since once we process the TRUNCATE, we need to accept EXTEND requests. One solution might be to queue and process _all_ relay cells inorder, not just the data cells. But that would probably create problems of its own.

This came out of a discussion of bug #1184 by wanoskarnet.

Child Tickets

Change History (6)

comment:1 Changed 9 years ago by arma

Yeah, this one's definitely a mess.

Another simpler option would be to just move all the queued cells onto the n_conn buffer before queuing the outgoing destroy cell for that circuit. Not exactly polite or fair, but it would restore correctness.

My main hesitation about that plan is that it opens a way for clients to be unfair.

I need to remember more about the actual protocol here though. If relay X gets a truncate request, it sends a destroy cell to relay X+1, which

  if (!CIRCUIT_IS_ORIGIN(circ) &&
      cell->circ_id == TO_OR_CIRCUIT(circ)->p_circ_id) {
    /* the destroy came from behind */
    circuit_set_p_circid_orconn(TO_OR_CIRCUIT(circ), 0, NULL);
    circuit_mark_for_close(circ, reason|END_CIRC_REASON_FLAG_REMOTE);
  }

which ends up calling such lines as

    for (conn=or_circ->n_streams; conn; conn=conn->next_stream)
      connection_edge_destroy(or_circ->p_circ_id, conn);
    or_circ->n_streams = NULL;

So is it the case right now that an outgoing destroy cell will blow away things in the n cell queue, and an outgoing truncate cell also will, and we're thinking about fixing the latter while leaving the former alone?

comment:2 Changed 9 years ago by nickm

Milestone: Tor: 0.2.3.x-final

comment:3 Changed 8 years ago by nickm

Milestone: Tor: 0.2.3.x-finalTor: unspecified

comment:4 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:5 Changed 7 years ago by nickm

Component: Tor RelayTor

comment:6 Changed 2 years ago by nickm

Keywords: protocol RELAY_TRUNCATE queues added
Priority: MediumLow
Severity: Normal

another option here is just to deprecate RELAY_TRUNCATE; I don't think we actually came up with a use for it in the last 7 years.

Note: See TracTickets for help on using tickets.