Opened 7 years ago

Closed 7 years ago

#9309 closed defect (fixed)

broken canonical connection detection in channel_matches_target_addr_for_extend()

Reported by: asn Owned by:
Priority: High Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client channel
Cc: andrea Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

channel_matches_target_addr_for_extend(): skruffy in IRC believes that the negation in !channel_matches_target_addr_for_extend is wrong.

He/she said that the old pre-channel code has the correct behavior.

Child Tickets

Attachments (1)

0001-Fix-bug9309-and-n_noncanonical-count-continue-code.patch (2.7 KB) - added by nickm 7 years ago.

Download all attachments as: .zip

Change History (6)

comment:1 Changed 7 years ago by nickm

Cc: andrea added
Milestone: Tor: 0.2.5.x-finalTor: 0.2.4.x-final

I think that the name might be inverted for the behavior. That could be leading to some confusion.
Here's the current code:

    if (!channel_is_canonical(chan) &&
         channel_is_canonical_is_reliable(chan) &&
        !channel_matches_target_addr_for_extend(chan, target_addr)) {
      ++n_noncanonical;
      continue;
    }

and the implementation it's calling:

static int
channel_tls_matches_target_method(channel_t *chan,
                                  const tor_addr_t *target)
{
  channel_tls_t *tlschan = BASE_CHAN_TO_TLS(chan);
  /* ... */
  return tor_addr_compare(&(tlschan->conn->real_addr),
                          target, CMP_EXACT);
}

Here's the old code:

    if (!conn->is_canonical && conn->link_proto >= 2 &&
        tor_addr_compare(&conn->real_addr, target_addr, CMP_EXACT)) {
      ++n_noncanonical;
      continue;
    }

I think that there should be a negation in channel_matches_target_addr_for_extend.

comment:2 Changed 7 years ago by nickm

Priority: normalmajor

This one, however, seems right:

0.2.4:

    if (chan->state != CHANNEL_STATE_OPEN) {
      /* If the address matches, don't launch a new connection for this
       * circuit. */
      if (!channel_matches_target_addr_for_extend(chan, target_addr))
        ++n_inprogress_goodaddr;
      continue;
    }

0.2.3:

    if (conn->_base.state != OR_CONN_STATE_OPEN) {
      /* If the address matches, don't launch a new connection for this
       * circuit. */
      if (!tor_addr_compare(&conn->real_addr, target_addr, CMP_EXACT))
        ++n_inprogress_goodaddr;
      continue;
    }

My recommendation is that "matches" should return !tor_addr-compare(), that the function documentation should say "Return true iff..." (i.e., describe the return value). The n_noncanonical case should say !matches; the n_inprogress_goodaddr case should say "matches".

comment:3 Changed 7 years ago by nickm

Status: newneeds_review

Okay, there's a 10 minute break in the dev mtg.

Please review bug9309 in my public repository. (Untested.) Also someone please check if skruffy would prefer his name in the changelog or not.

comment:4 Changed 7 years ago by nickm

Also, attached the patch for review convenience.

comment:5 Changed 7 years ago by andrea

Resolution: fixed
Status: needs_reviewclosed

Fix looks good; thanks to skruffy for spotting it. Merging to maint-0.2.4 and master now.

Note: See TracTickets for help on using tickets.