Negative cell-processed-cells values

[atagar]

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

[karsten]

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

[atagar]

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).

[karsten]

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.

[karsten]

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.

[nickm]

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.

[karsten]

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?

[nickm]

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?

[karsten]

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.