Opened 11 years ago

Last modified 7 years ago

#805 closed defect (Fixed)

choice of the best OR connection

Reported by: rovv Owned by: nickm
Priority: Low Milestone: 0.2.1.x-final
Component: Core Tor/Tor Version:
Severity: Keywords:
Cc: rovv, nickm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:


connection_or_get_by_identity_digest() returns a correct best connection of type OR; at most cases.
(if no other problems with other parts of code)

But checks not generalize enough. As example:
Let Tor has two (or more) non-obsolete connections with the same digest and one of them
is non-canonical and newest (self present at orconn_identity_map).
While other conditions the same, function returns those non-canonical connection
from orconn_identity_map even if oldest connections (also non-obsolete) is canonical.

This situation with several not marked for close non-obsolete connections
was hypothetical but it's happened.

here a version of fix

--- connection_or.original.c Sat Jul 26 22:36:20 2008
+++ connection_or.c Tue Aug 19 09:38:30 2008
@@ -480,6 +480,8 @@

continue; /* We never prefer obsolete over non-obsolete connections. */

if (

+ /* We prefer canonical connections: */

+ (!best->is_canonical && conn->is_canonical)

/* We prefer non-obsolete connections: */

(best->_base.or_is_obsolete && !conn->_base.or_is_obsolete)

/* If both have circuits we prefer the newer: */

[Automatically added by flyspray2trac: Operating System: All]

Child Tickets

Change History (4)

comment:1 Changed 11 years ago by rovv

one more piece of thoughts.
(correct me, if I am wrong)
condition from connection_or_get_by_identity_digest():

newer = best->_base.timestamp_created < conn->_base.timestamp_created;

never be true.

Linked list ->next_with_same_id->next_with_same_id->next_with_same_id...
actually ordered by ages of connections. Each subsequent connection from
chain older than previous, with top of newer stored at orconn_identity_map.
Timestamps can be affected by clock skew, but actually age
of connections not changes.

On the other hand it's more general checking.

comment:2 Changed 11 years ago by nickm

Right. The test is never actually true because of how the linked list is implemented. On the other hand,
it doesn't hurt anything to have it there. If we took it out, we would need to document that the list
had to be kept sorted, and possibly take steps to sort it in the future. I'm going to leave it as-is.

comment:3 Changed 11 years ago by nickm

flyspray2trac: bug closed.

comment:4 Changed 7 years ago by nickm

Component: Tor RelayTor
Note: See TracTickets for help on using tickets.