[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #25400 [Core Tor/Tor]: CIRC_BW event miscounts, should count all circ data
#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
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs