[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_revision
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 arma):
* status: needs_review => needs_revision
Comment:
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/.
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.
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.
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.
And, a changes file. :)
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/25226#comment:30>
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