[tor-bugs] #25400 [Core Tor/Tor]: CIRC_BW event miscounts, should count all circ data

[Tor Bug Tracker & Wiki]

#25400: CIRC_BW event miscounts, should count all circ data
-------------------------------------------------+-------------------------
 Reporter:  mikeperry                            |          Owner:
                                                 |  mikeperry
     Type:  defect                               |         Status:
                                                 |  needs_review
 Priority:  Medium                               |      Milestone:  Tor:
                                                 |  0.3.4.x-final
Component:  Core Tor/Tor                         |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  tor-stats, review-group-34, 034      |  Actual Points:
  -roadmap-subtask, 034-triage-20180328,         |
  034-included-20180328                          |
Parent ID:  #25546                               |         Points:
 Reviewer:  dgoulet                              |        Sponsor:
-------------------------------------------------+-------------------------
Changes (by mikeperry):

 * status:  needs_information => needs_review


Comment:

 Replying to [comment:9 dgoulet]:
 > Good stuff Mike. I want to ask and clarify couple of things here.
 >
 > 1. Why not put this counting in `command_process_relay_cell()`? I'm
 asking because that function, before calling
 `circuit_receive_relay_cell()` can close the circuit because of invalid
 `RELAY_EARLY` cell or too many of them or if cells are received but the
 circuit state doesn't allow it. Don't you want to catch those also in your
 calculation? Looking at this comment, it seems you might?

 Actually, yes, let's move this block up towards the top of
 circuit_receive_relay_cell(). I had put it below since those violations
 closed the circuit, but you're right, let's count them too. Thanks!

 https://oniongit.eu/mikeperry/tor/commit/4371b54854545a3962f9455fe318711dc7b799f9

 > {{{
 > +  /* Count all circuit bytes here for control port accuracy. We want
 > +   * to count even invalid/dropped relay cells, hence counting
 > +   * before the recognized check and the connection_edge_process_relay
 > +   * cell checks.
 > }}}
 >
 > 2. The `CELL_PAYLOAD_SIZE`, as you stated in your previous comment,
 doesn't take into account the header size but that header _can_ bring the
 cell size up to 514 bytes (`CELL_MAX_NETWORK_SIZE`). I'm not sure I
 understand the reasoning behind not counting the header. You mention that
 the controller application should subtract the header size but it really
 shouldn't if tor is not counting them in the first place? Actually, the
 cell size can vary depending on the use of "wide circ ids" so shouldn't
 `get_cell_network_size(chan->wide_circ_ids)` be used instead of
 `CELL_PAYLOAD_SIZE`?

 I'm using the definition of "total bandwidth carried by the circuit". This
 means that the payload size is the natural value here. For purposes of the
 payload carried, it does not matter if the *circuit* headers are different
 sizes. My earlier comment was about *relay* headers, which are part of the
 (encrypted) circuit payload, and I think should be counted in this stat.

 > 3. The counting bytes code for read/written is really the same so I
 would be much happier with a function that does that and could take a
 pointer to `n_read_circ_bw` or `n_written_circ_bw` for the calculation (or
 not a pointer, just return the new value). Seems to me will be easier to
 unit test but also better code semantic as well.

 Good point. I made this u32 overflow add into a util.c helper, since it is
 a common pattern, independent of this circuit accounting:

 https://oniongit.eu/mikeperry/tor/commit/b9f28f475ecc546503bd85d1d4a0564f09b638a3
 (impl + tests)
 https://oniongit.eu/mikeperry/tor/commit/4e86178122c7aa20af91838c59ee5dd6fe6bb7d5
 (use)

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/25400#comment:13>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online