[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
[tor-bugs] Re: #1184 [Tor - Tor client]: Sending useless relay cells after the destroy cell
#1184: Sending useless relay cells after the destroy cell
--------------------------------+-------------------------------------------
Reporter: arma | Type: defect
Status: needs_review | Priority: minor
Milestone: Tor: 0.2.2.x-final | Component: Tor - Tor client
Version: 0.2.2.6-alpha | Resolution: None
Keywords: | Parent:
--------------------------------+-------------------------------------------
Changes (by nickm):
* status: new => needs_review
Old description:
> r9913 took out the "clear cell queue after sending destroy cell" logic.
>
> Bug 1150 (comment 3519) points out that the cells are just going to get
> dropped by the other side anyway.
>
> I think it's right.
>
> There are three places that call connection_or_send_destroy().
>
> The first is in relay.c, where we unlink the circ from the n_conn
> immediately after.
>
> The second is in command.c, where we refuse new circuits for various
> reasons. No cells queued yet.
>
> The third is in circuitlist.c, during circuit-mark-for-close. So the
> question is, if the circuit is marked, do we really want to still be
> trying to send its cells? Either it got marked from the other side,
> in which case the other side has already decided it doesn't want to hear
> from us. Or it got marked from our side, in which case either that was
> an error (circuit needs to die) or it has no streams on it, in which
> case there's no harm in clearing its queue.
>
> Did I miss anything?
>
> Perhaps we should check, in main.c when we're considering whether to mark
> it because it's idle, whether there are any cells still in its queue.
>
> Or maybe we should just pass an argument into
> connection_or_send_destroy()
> from the truncate case in relay.c saying "clear the queue please". Then
> we
> can remain ambiguous in the cases above where I said "no harm because
> surely
> the queue has nothing in it". That's certainly the safest approach, but
> wouldn't it be nice to simplify rather than complexify? :)
>
> [Automatically added by flyspray2trac: Operating System: All]
New description:
r9913 took out the "clear cell queue after sending destroy cell" logic.
Bug 1150 (comment 3519) points out that the cells are just going to get
dropped by the other side anyway.
I think it's right.
There are three places that call connection_or_send_destroy().
The first is in relay.c, where we unlink the circ from the n_conn
immediately after.
The second is in command.c, where we refuse new circuits for various
reasons. No cells queued yet.
The third is in circuitlist.c, during circuit-mark-for-close. So the
question is, if the circuit is marked, do we really want to still be
trying to send its cells? Either it got marked from the other side,
in which case the other side has already decided it doesn't want to hear
from us. Or it got marked from our side, in which case either that was
an error (circuit needs to die) or it has no streams on it, in which
case there's no harm in clearing its queue.
Did I miss anything?
Perhaps we should check, in main.c when we're considering whether to mark
it because it's idle, whether there are any cells still in its queue.
Or maybe we should just pass an argument into connection_or_send_destroy()
from the truncate case in relay.c saying "clear the queue please". Then we
can remain ambiguous in the cases above where I said "no harm because
surely
the queue has nothing in it". That's certainly the safest approach, but
wouldn't it be nice to simplify rather than complexify? :)
[Automatically added by flyspray2trac: Operating System: All]
--
Comment:
So if the destroy cell is outgoing, the client really won't process any
cells on the circuit, so it's safe for the client to clear the queue. And
if it's incoming at a relay, it's going to turn into a truncated, and get
queued up behind all the other relay cells for the client. And if it's
incoming at a client, there is currently no cell queue there at all, since
we crypt cells aggressively on receiving them. So I am pretty sure that
clearing cell queues on circuit_mark_for_close is a safe thing to do. I
don't think we _were_ sending any cells in these cases, but if we were,
they weren't getting processed.
The one case that puzzles me is RELAY_COMMAND_TRUNCATE. We call
connection_or_send_destroy() there. But do we really mean to cancel every
relay cell that the intermediate relay received before the TRUNCATE
command but has not yet relayed, or do we mean them to go on their way? I
think it's safe to cancel them given how Tor works now, but our spec
doesn't envision that possibility.
>Perhaps we should check, in main.c when we're considering whether to mark
it because it's idle, whether there are any cells still in its queue.
You were talking about connections, perhaps? I don't see anywhere in
main.c where we close idle circuits. But connections don't have cell
queues; circuits do.
Anyway, I've hacked up a fix. See branch bug1184 in my public repository.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/1184#comment:2>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online