[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #25226 [Core Tor/Tor]: Circuit cell queue can fill up memory
#25226: Circuit cell queue can fill up memory
-------------------------------------------------+-------------------------
Reporter: dgoulet | Owner: dgoulet
Type: defect | Status:
| needs_review
Priority: Medium | Milestone: Tor:
| 0.3.3.x-final
Component: Core Tor/Tor | Version:
Severity: Normal | Resolution:
Keywords: tor-cell, tor-relay, tor-dos, | Actual Points:
033-must, review-group-34, security, |
033-triage-20180320, 033-included-20180320 |
Parent ID: | Points:
Reviewer: arma | Sponsor:
-------------------------------------------------+-------------------------
Changes (by dgoulet):
* status: needs_revision => needs_review
Comment:
Replying to [comment:30 arma]:
> Ok, review of the actual branch:
>
> {{{
> + if (PREDICT_UNLIKELY(queue->n > max_circuit_cell_queue_size)) {
> }}}
>
> This wants to be >=, right? Otherwise we let it grow one past the
maximum.
>
> and then in the log message, s/cell/cells/.
Fixup commit `ed963d7604`
>
> The default if we don't have a consensus param is 9000? I'm a bit
nervous about edge cases where we somehow fail to set a consensus param,
or when relays act before they know a consensus param, especially in the
future once we've rolled out Mike's "defensive dropping" website
fingerprinting design. What would you say to having a default of 50000?
Since that's what we're imagining to set as the default for now anyway.
I'm fine with it. 50k cells is 25MB so ~40 circuits like that would start
putting memory pressure at 1GB. Not many circuits but even at 9k limit,
the number of circuits would be a bit less than 250 which is not much
either.
Fixup commit `7d25850415`
>
> Also, the minimum settable number is 8000? It seems like we want to
allow a lower number for chutney experiments where we control all of the
traffic and we're trying to check for bugs. I think 1000 might be a little
bit too low, for example because if you ask for two streams and they
finish, you'll get 500+500+1+1=1002 cells coming at you from the exit. Or
if you're using optimistic data, you'll get the CONNECTED cells back
first, bumping it up to more like 1004. But still, I think setting the
minimum to 1000 is a legitimate choice, since that makes it easy for
experimentors to experiment however they see fit. We should just avoid
setting the minimum possible value in the consensus, but that's already
true for other consensus params, e.g. we should avoid setting
DOS_CC_CIRCUIT_BURST_DEFAULT to 1 even though it's allowed by the code.
Fixup commit `497954459d`
>
> And, now that we understand more what counts in the sendme window and
what doesn't, it might be smart to revise the huge comment to be more
accurate.
Fixup commit `d4ee02a714`
>
> And, a changes file. :)
Fixup commit `f87c2ff62a`
All the above have been pushed in my `_01` branch. I have a `_02` branch
that squashes everything but also changes one commit message to reflect
the changes made above ^.
Branch (with fixup): `bug25226_033_01`
Branch (squashed): `bug25226_033_02`
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/25226#comment:31>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs