[tor-bugs] #25226 [Core Tor/Tor]: Circuit cell queue can fill up memory

[Tor Bug Tracker & Wiki]

#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