[tor-bugs] #936 [Core Tor/Tor]: Race in picking connections for create cell

[Tor Bug Tracker & Wiki]

#936: Race in picking connections for create cell
--------------------------+----------------------------------
 Reporter:  arma          |          Owner:
     Type:  defect        |         Status:  closed
 Priority:  Low           |      Milestone:  Tor: unspecified
Component:  Core Tor/Tor  |        Version:  0.2.0.31
 Severity:  Normal        |     Resolution:  worksforme
 Keywords:  tor-relay     |  Actual Points:
Parent ID:                |         Points:
 Reviewer:                |        Sponsor:
--------------------------+----------------------------------
Description changed by arma:

Old description:

> 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]

New description:

 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]

--

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/936#comment:17>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online