Negative cell-processed-cells values

Current 'cell-processed-cells' entries (an extrainfo descriptor attribute) sometimes has negative values. Moving from irc...

14:47 < atagar> karsten: Should 'cell-processed-cells' extrainfo entries allow
for negative values? Spec describes it as 'Mean number of processed cells per
circuit' but I'm spotting values like...
14:47 < atagar> cell-processed-cells -5644,65,22,10,5,4,2,1,1,1
15:07 < rransom> atagar: That looks like a Tor bug.
15:37 < atagar> thanks, I'll move to a ticket then

changed milestone to %Tor: 0.2.3.x-final

added component::core tor/tor milestone::Tor: 0.2.3.x-final priority::low resolution::fixed status::closed tor-relay type::defect labels

Looks like a bug. Do you know which Tor version produced that line?

Trac:
Component: Analysis to Tor Relay

My cached extrainfo descriptors had six instances of it... fingerprint: 253DFF1838A2B7782BE7735F74E50090D46CA1BC, cell-processed-cells: [-5279.0, 69.0, 24.0, 10.0, 6.0, 4.0, 2.0, 1.0, 1.0, 1.0] fingerprint: DB8C6D8E0D51A42BDDA81A9B8A735B41B2CF95D1, cell-processed-cells: [-5146.0, 65.0, 22.0, 10.0, 5.0, 4.0, 2.0, 1.0, 1.0, 1.0] fingerprint: 43691853EA556C21A77E006886A5DC579855F527, cell-processed-cells: [-10371.0, 139.0, 38.0, 18.0, 10.0, 7.0, 3.0, 1.0, 1.0, 1.0] fingerprint: F3D4C7479F8789758A77FF61D2D8929311568394, cell-processed-cells: [-7516.0, 121.0, 35.0, 17.0, 9.0, 6.0, 2.0, 1.0, 1.0, 1.0] fingerprint: B93DCC053D7F0472BB17A4514E06FE76D9FB714B, cell-processed-cells: [-5802.0, 63.0, 18.0, 9.0, 5.0, 4.0, 3.0, 1.0, 1.0, 1.0] fingerprint: 24B1F63F7DF9F85D711864811CC401BE5EB5FB9A, cell-processed-cells: [-5621.0, 63.0, 18.0, 8.0, 5.0, 4.0, 3.0, 1.0, 1.0, 1.0]

Those belong to...

• 253DFF1838A2B7782BE7735F74E50090D46CA1BC Tor 0.2.3.15-alpha (git-d2fd67f30991cbc4)

• DB8C6D8E0D51A42BDDA81A9B8A735B41B2CF95D1 Tor 0.2.3.15-alpha (git-d2fd67f30991cbc4)

• 43691853EA556C21A77E006886A5DC579855F527 Tor 0.2.3.13-alpha-dev (git-4703bf8792b69745)

• F3D4C7479F8789758A77FF61D2D8929311568394 Tor 0.2.3.13-alpha-dev (git-4703bf8792b69745)

• B93DCC053D7F0472BB17A4514E06FE76D9FB714B Tor 0.2.3.15-alpha (git-d2fd67f30991cbc4)

• 24B1F63F7DF9F85D711864811CC401BE5EB5FB9A Tor 0.2.3.15-alpha (git-d2fd67f30991cbc4)

So pretty recent. Also most (all?) of these are TorServers relays (not relevant, just something that stood out).

The problem is an integer overflow. Here are the entries from two cell-processed-cells lines published by 24B1F63F:

2012-05-09 08:34:31, 6839 84 32 14 7 5 3 1 1 1 2012-05-10 08:34:31, -5923 73 26 11 6 4 2 1 1 1

In order to calculate the 6839 in the first line, the relay summed up cells processed for the loudest 308806 circuits, that is, 2111924234 (close to INT32_MAX), and then divided by those 308806 to come up with the mean number of 6839 cells per circuit. When calculating the -5923 in the second line, the sum didn't fit into an integer, because there were 356842 circuits per decile.

See branch task-5849 in my public repo which is based on maint-0.2.2. Note that the patch is untested.

Trac:
Status: new to needs_review

I just asked Moritz to test this branch (or rather, a freshly rebased branch called task-5849-2) on a TorServers relay.

Assigning to 0.2.3.x-final, even though the bug was introduced in 0.2.2.1 and the fix is trivial. But the bug is harmless and I hear we leave 0.2.2.x alone unless fixes are critical.

Trac:
Milestone: N/A to Tor: 0.2.3.x-final

Found a task-5849-3, and looked at that instead.

Are those casts to uint64_t really necessary? They look like cargo-cultism to me. In any binary operation between a uint64_t and a uint32_t, the uint32_t should be promoted to uint64_t losslessly.

In any binary operation between a uint64_t and an int, the int will get converted to a uint64_t anyway (with the possibility of undesirable results if the int is negative). Maybe circs_in_share should be an unsigned type.

Replying to nickm:

Found a task-5849-3, and looked at that instead.

Ah, right. It's the very same patch, only rebased to master.

Are those casts to uint64_t really necessary? They look like cargo-cultism to me. In any binary operation between a uint64_t and a uint32_t, the uint32_t should be promoted to uint64_t losslessly.

In any binary operation between a uint64_t and an int, the int will get converted to a uint64_t anyway (with the possibility of undesirable results if the int is negative). Maybe circs_in_share should be an unsigned type.

Sounds reasonable. Pushed a new (fixup) commit to task-5849-3.

I'll watch Moritz' relay for a few more days which still runs the non-fixed-up commit. That should tell us whether the bug is fixed or not. Or do you think I should tell him to git pull and restart his relay?

No need to tell him to pull; the change is minor. Actually, I'm going to merge this branch today anyway, since it appears to be obviously right, and fixes a bug, even if we don't yet know whether it's the fix for this bug. Please comment on this ticket, reopening if necessary, once we know whether it solves the bug for Moritz?

Trac:
Resolution: N/A to fixed
Status: needs_review to closed

Looks like the bug is fixed:

router politkovskaja 77.247.181.165 443 0 80
platform Tor 0.2.3.17-beta on Linux
[...]

extra-info politkovskaja 7DCB5313B9541DD29C94BFDE0ADF91DC91D2CFE9
[...]
cell-stats-end 2012-06-25 19:24:13 (86400 s)
cell-processed-cells 9313,97,36,16,8,4,1,1,1,1
cell-queued-cells 0.32,0.02,0.01,0.00,0.00,0.00,0.00,0.00,0.00,0.00
cell-time-in-queue 77,28,18,16,14,11,13,14,22,16
cell-circuits-per-decile 268667

9313 * 268667 = 2502095771 which is > 2147483648.

Trac:
Keywords: N/A deleted, tor-relay added

Trac:
Component: Tor Relay to Tor

closed

moved to tpo/core/tor#5849 (closed)