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

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

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

 * status:  needs_review => needs_information


Comment:

 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?

 {{{
 +  /* 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`?

 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.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/25400#comment:9>
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