Opened 11 years ago

Closed 2 years ago

Last modified 2 years ago

#936 closed defect (worksforme)

Race in picking connections for create cell

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

Description (last modified by arma)

Reported by lark.

I possible found edge case for doubling conns between two relays, the
one inbound and the one outbound for the node. It's can trigger if conns
launched by relays to each other at the same time, very precisely. Which can
happens for two relays with high uptime and fast speed, after
TIME_BEFORE_OR_CONN_IS_TOO_OLD.

--- connection_or.c.orig	Thu Feb  5 00:42:22 2009
+++ connection_or.c	Fri Mar  6 19:15:04 2009
 -452,7 +452,7 @@
                         const or_connection_t *b,
                         int forgive_new_connections)
 {
-  int newer;
+  int newer, delta, inbound_a, outbound_b, prefer_conn_a;
 /** Do not definitively deprecate a new connection with no circuits on it
  * until this much time has passed. */
 #define NEW_CONN_GRACE_PERIOD (15*60)
 -461,15 +461,32 @@
     return 0; /* A canonical connection is better than a non-canonical
                * one, no matter how new it is or which has circuits. */
 
-  newer = b->_base.timestamp_created < a->_base.timestamp_created;
+  inbound_a = tor_tls_is_server(a->tls);
+  outbound_b = !tor_tls_is_server(b->tls);
 
+  delta=(int)(a->_base.timestamp_created - b->_base.timestamp_created);
+  newer= delta > 0;
+
+ /* We prefer the "a" conn at equal conditions:
+  *
+  * if "a" is inbound from a node with higher identity key
+  * and created at the same time or later than outbound "b" conn;
+  *
+  * or
+  *
+  * if "a" newer than "b" for any other inbound-outbound
+  * combinations */
+  prefer_conn_a = ((newer && !(inbound_a && outbound_b)) ||
+                   (inbound_a && outbound_b && delta >= 0 &&
+                    a->circ_id_type == CIRC_ID_TYPE_LOWER));
+
   if (
       /* We prefer canonical connections regardless of newness. */
       (!b->is_canonical && a->is_canonical) ||
       /* If both have circuits we prefer the newer: */
-      (b->n_circuits && a->n_circuits && newer) ||
+      (b->n_circuits && a->n_circuits && prefer_conn_a) ||
       /* If neither has circuits we prefer the newer: */
-      (!b->n_circuits && !a->n_circuits && newer))
+      (!b->n_circuits && !a->n_circuits && prefer_conn_a))
     return 1;
 
   /* If one has no circuits and the other does... */

[Automatically added by flyspray2trac: Operating System: All]

Child Tickets

Change History (17)

comment:1 Changed 11 years ago by nickm

Looks worthwhile, but I think the actual circumstances where this comes up are rare enough that we
could safely wait till 0.2.2.x to do this.

comment:2 Changed 11 years ago by arma

"suggested fix is wrong. need absolute different ways for solves it."

comment:3 Changed 9 years ago by nickm

Milestone: post 0.2.1.xTor: 0.2.2.x-final

comment:4 Changed 9 years ago by nickm

Description: modified (diff)

So there are two problems at work here:

  • Two connections could have been created in the same second, such that neither would seem newer than the other.
  • Two ORs will not necessarily have the same view of when two connections were created relative to another; each will think that the connection that came from it will was created before the other's.

We could add a tie-breaker to be used only in the case where the two connections were created in the same second, but that wouldn't help in the case where the ORs each think that a different connection is newest. We could add a tie-breaker to be used in the case where the connections were created within some small time-step of one another, but there's no reason that ORs would necessarily agree on the delta.

comment:5 Changed 9 years ago by nickm

07:18 < armadev> if it's happening on every fast relay, we want to know that
07:19 < armadev> how often do we have two OR conns open to a relay where 
                 neither of them is really old?
07:19 < armadev> (open and with circuits on them)
07:19 < nickm> hm.  Perhaps check for two open live OR conns to the same relay 
               both created within some delta of one another?
07:19 < nickm> That would be how this would manifest
07:20 < armadev> sounds plausible.

comment:6 Changed 9 years ago by arma

The line right before the quote nick provided was "we could put in some checks to see how often it happens".

Triage: it would be good to get more understanding of this bug, but there's no reason for it to hold up 0.2.2.

comment:7 Changed 8 years ago by nickm

Milestone: Tor: 0.2.2.x-finalTor: 0.2.3.x-final

Deferring to 0.2.3; this needs a proposal. Adding checks in 0.2.3 would be good though.

comment:8 Changed 7 years ago by nickm

Milestone: Tor: 0.2.3.x-finalTor: 0.2.4.x-final

comment:9 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:10 Changed 7 years ago by nickm

Component: Tor RelayTor

comment:11 Changed 7 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: 0.2.5.x-final

comment:12 Changed 6 years ago by nickm

Cc: arma,nickmarma, nickm
Milestone: Tor: 0.2.5.x-finalTor: 0.2.???

comment:13 Changed 3 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:14 Changed 3 years ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:15 Changed 2 years ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:16 Changed 2 years ago by nickm

Resolution: Noneworksforme
Severity: Normal
Status: newclosed

The underlying code here has changed significantly over the last 6 years; in particular, the recently merged "canonical connection" stuff from Mike in 0.3.1 has some promise to solve this.

comment:17 Changed 2 years ago by arma

Description: modified (diff)
Note: See TracTickets for help on using tickets.