broken canonical connection detection in channel_matches_target_addr_for_extend()

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.

changed milestone to %Tor: 0.2.4.x-final

added channel component::core tor/tor milestone::Tor: 0.2.4.x-final priority::high resolution::fixed status::closed tor-client type::defect labels

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.

Trac:
Cc: N/A to andrea
Milestone: Tor: 0.2.5.x-final to Tor: 0.2.4.x-final

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".

Trac:
Priority: normal to major

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.

Trac:
Status: new to needs_review

Trac:
0001-Fix-bug9309-and-n_noncanonical-count-continue-code.patch

Also, attached the patch for review convenience.

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

Trac:
Resolution: N/A to fixed
Status: needs_review to closed

closed

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