Opened 6 years ago

Closed 6 years ago

#9880 closed defect (fixed)

Possible race closing bad_for_new_circs channels, others

Reported by: nickm Owned by:
Priority: High Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay channels
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Skruffy on IRC reports this bug.

Suppose that we have a channel to a router that we don't want to use for new circuits, and we're opening a new channel, and we have some pending circuits.

When we call channel_closed() on the first channel (which should have bad_for_new_circs set on it), it will call circuit_n_chan_done(chan, 0), which will kill all the circuits pending on the second channel.

One option here is to call circuit_n_chan_done(chan, 0) only if the channel was not open.

Another (possibly) is to change circuit_get_all_pending_on_channel so that no circuit can be pending on a bad_for_new_circs channel.

Child Tickets

Change History (7)

comment:1 Changed 6 years ago by nickm

Summary: Possible race closing bad_for_new_circsPossible race closing bad_for_new_circs channels, others

This can also occur if we wanted a canonical connection and this isn't one.

comment:2 Changed 6 years ago by nickm

Status: newneeds_review

Here's the "obvious" fix. But is it right? Should it check for CLOSING too?

diff --git a/src/or/channel.c b/src/or/channel.c
index afe28bf..d130ede 100644
--- a/src/or/channel.c
+++ b/src/or/channel.c
@@ -1297,7 +1297,8 @@ channel_closed(channel_t *chan)
 
   /* Inform any pending (not attached) circs that they should
    * give up. */
-  circuit_n_chan_done(chan, 0);
+  if (chan->state != CHANNEL_STATE_OPEN)
+    circuit_n_chan_done(chan, 0);
 
   /* Now close all the attached circuits on it. */
   circuit_unlink_all_from_channel(chan, END_CIRC_REASON_CHANNEL_CLOSED);

comment:3 Changed 6 years ago by nickm

If that isn't right, or isn't obviously right, skruffy suggests adding a "was this channel *ever* open" flag, and looking at that here.

comment:4 Changed 6 years ago by nickm

Ah, Athena called my above fix wrong in #9776. Well, how about "bug9880_maybe" in my public repository?

comment:5 in reply to:  4 Changed 6 years ago by andrea

Replying to nickm:

Ah, Athena called my above fix wrong in #9776. Well, how about "bug9880_maybe" in my public repository?

I believe that has the desired effect but I'm not yet sure I understand/believe in the bug yet, so give me a bit more time to try to decipher it.

comment:6 Changed 6 years ago by nickm

bug9880_fix is the branch now.

comment:7 Changed 6 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged to 0.2.4 and master.

Note: See TracTickets for help on using tickets.