#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@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs