#5849 closed defect (fixed)
Negative cell-processed-cells values
Reported by: | atagar | Owned by: | |
---|---|---|---|
Priority: | Low | Milestone: | Tor: 0.2.3.x-final |
Component: | Core Tor/Tor | Version: | |
Severity: | Keywords: | tor-relay | |
Cc: | Actual Points: | ||
Parent ID: | Points: | ||
Reviewer: | Sponsor: |
Description
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
Child Tickets
Change History (10)
comment:1 Changed 8 years ago by
Component: | Analysis → Tor Relay |
---|
comment:2 Changed 8 years ago by
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).
comment:3 Changed 8 years ago by
Status: | new → needs_review |
---|
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.
comment:4 Changed 8 years ago by
Milestone: | → Tor: 0.2.3.x-final |
---|
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.
comment:5 follow-up: 6 Changed 8 years ago by
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.
comment:6 Changed 8 years ago by
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?
comment:7 Changed 8 years ago by
Resolution: | → fixed |
---|---|
Status: | needs_review → closed |
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?
comment:8 Changed 7 years ago by
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.
comment:9 Changed 7 years ago by
Keywords: | tor-relay added |
---|
comment:10 Changed 7 years ago by
Component: | Tor Relay → Tor |
---|
Looks like a bug. Do you know which Tor version produced that line?