Opened 5 years ago

Closed 5 years ago

#12862 closed enhancement (wontfix)

Refactor code to modify circ->n_chan only by circuit_set_circid_chan_helper

Reported by: cypherpunks Owned by:
Priority: Low Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay 026-triaged-1 026-deferrable
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Right now code assigns circ->n_chan before actually keeps it in chan-circid map. Previous value of circ->n_chan used as old_chan by circuit_set_n_circid_chan() and circuit_set_circid_chan_helper()

For example:

  if (old_chan) {
    /*
     * If we're changing channels or ID and had an old channel and a non
     * zero old ID and weren't marked for close (i.e., we should have been
     * attached), detach the circuit. ID changes require this because
     * circuitmux hashes on (channel_id, circuit_id).
     */
    if (old_id != 0 && (old_chan != chan || old_id != id) &&
        !(circ->marked_for_close)) {
      tor_assert(old_chan->cmux);
      circuitmux_detach_circuit(old_chan->cmux, circ);
    }

    /* we may need to remove it from the conn-circid map */
    search.circ_id = old_id;
    search.chan = old_chan;
    found = HT_REMOVE(chan_circid_map, &chan_circid_map, &search);
    if (found) {
      tor_free(found);
      if (direction == CELL_DIRECTION_OUT) {
        /* One fewer circuits use old_chan as n_chan */
        --(old_chan->num_n_circuits);
      } else {
        /* One fewer circuits use old_chan as p_chan */
        --(old_chan->num_p_circuits);
      }
    }
  }

It's useless to process such circ->n_chan as old_chan

Child Tickets

Change History (5)

comment:1 Changed 5 years ago by cypherpunks

It's probably enhancement ticket, if no actual harm after such old_chan.

comment:2 Changed 5 years ago by nickm

Milestone: Tor: 0.2.6.x-final
Priority: normalminor
Type: defectenhancement

comment:3 Changed 5 years ago by nickm

Keywords: tor-relay 026-triaged-1 026-deferrable added

comment:4 Changed 5 years ago by andrea

There's no bug here, and this ticket is just griping that old_chan (and old_id too) are copies of *chan_ptr and *circid_ptr in the first half of circuit_set_circid_chan_helper(), AFAICT.

Refactoring wouldn't eliminate the old_chan/old_id local variables, since we also need them after we change the channel/circuit id to decide whether we need to circuitmux_attach_circuit(). It'd only change the references to old_chan/old_id to *chan_ptr/*circid_ptr in the if-clause tests prior to the channel/circuit id change.

I'm not sure changing this actually counts as an improvement. The existing names make it manifestly clear that it's comparing the existing channel or circuit id to the value we're setting to, and thus that the purpose of the test is to detect if we have a real change or one/both of them is remaining constant. It'd also introduce an inconsistency in usage between the ones before the assignment and the circuitmux_attach_circuit() test after, where these are at present nicely symmetrical.

comment:5 Changed 5 years ago by andrea

Resolution: wontfix
Status: newclosed

Closing this ticket since no one objected to this analysis at weekly IRC meeting.

Note: See TracTickets for help on using tickets.