Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#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 karsten

Component: AnalysisTor Relay

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

comment:2 Changed 8 years ago by 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).

comment:3 Changed 8 years ago by karsten

Status: newneeds_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 karsten

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 Changed 8 years ago by 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.

comment:6 in reply to:  5 Changed 8 years ago by 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?

comment:7 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

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

comment:9 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:10 Changed 7 years ago by nickm

Component: Tor RelayTor
Note: See TracTickets for help on using tickets.