#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


 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
 > 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

 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

 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

Reply via email to