Opened 3 years ago

Closed 3 years ago

#20467 closed defect (fixed)

bwauth aggregator divides by zero if a node class is missing

Reported by: teor Owned by: aagbsn
Priority: Medium Milestone:
Component: Core Tor/Torflow Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The bwauth code expects there to always be a non-zero number of each of "Guard+Exit", "Guard", "Exit", and "Middle" nodes.

In small networks, this isn't always the case:

diff --git a/NetworkScanners/BwAuthority/aggregate.py b/NetworkScanners/BwAuthority/aggregate.py
index 01ea8b5..cae436b 100755
--- a/NetworkScanners/BwAuthority/aggregate.py
+++ b/NetworkScanners/BwAuthority/aggregate.py
@@ -489,10 +489,15 @@ def main(argv):
 
     for cl in ["Guard+Exit", "Guard", "Exit", "Middle"]:
       c_nodes = filter(lambda n: n.node_class() == cl, nodes.itervalues())
-      true_filt_avg[cl] = sum(map(lambda n: n.filt_bw, c_nodes))/float(len(c_nodes))
-      true_strm_avg[cl] = sum(map(lambda n: n.strm_bw, c_nodes))/float(len(c_nodes))
-      true_circ_avg[cl] = sum(map(lambda n: (1.0-n.circ_fail_rate),
+      if len(c_nodes) > 0:
+        true_filt_avg[cl] = sum(map(lambda n: n.filt_bw, c_nodes))/float(len(c_nodes))
+        true_strm_avg[cl] = sum(map(lambda n: n.strm_bw, c_nodes))/float(len(c_nodes))
+        true_circ_avg[cl] = sum(map(lambda n: (1.0-n.circ_fail_rate),
                              c_nodes))/float(len(c_nodes))
+      else:
+        true_filt_avg[cl] = 0.0
+        true_strm_avg[cl] = 0.0
+        true_circ_avg[cl] = 0.0
 
       # FIXME: This may be expensive
       pid_tgt_avg[cl] = true_filt_avg[cl]
@@ -835,10 +840,13 @@ def main(argv):
     c_nodes = filter(lambda n: n.node_class() == cl, nodes.itervalues())
     nc_nodes = filter(lambda n: n.pid_error < 0, c_nodes)
     pc_nodes = filter(lambda n: n.pid_error > 0, c_nodes)
-    plog("INFO", "Avg "+cl+"  pid_error="+str(sum(map(lambda n: n.pid_error, c_nodes))/len(c_nodes)))
-    plog("INFO", "Avg "+cl+" |pid_error|="+str(sum(map(lambda n: abs(n.pid_error), c_nodes))/len(c_nodes)))
-    plog("INFO", "Avg "+cl+" +pid_error=+"+str(sum(map(lambda n: n.pid_error, pc_nodes))/len(pc_nodes)))
-    plog("INFO", "Avg "+cl+" -pid_error="+str(sum(map(lambda n: n.pid_error, nc_nodes))/len(nc_nodes)))
+    if len(c_nodes) > 0:
+      plog("INFO", "Avg "+cl+"  pid_error="+str(sum(map(lambda n: n.pid_error, c_nodes))/len(c_nodes)))
+      plog("INFO", "Avg "+cl+" |pid_error|="+str(sum(map(lambda n: abs(n.pid_error), c_nodes))/len(c_nodes)))
+    if len(pc_nodes) > 0:
+      plog("INFO", "Avg "+cl+" +pid_error=+"+str(sum(map(lambda n: n.pid_error, pc_nodes))/len(pc_nodes)))
+    if len(nc_nodes) > 0:
+      plog("INFO", "Avg "+cl+" -pid_error="+str(sum(map(lambda n: n.pid_error, nc_nodes))/len(nc_nodes)))
 
   n_nodes = filter(lambda n: n.pid_error < 0, nodes.itervalues())
   p_nodes = filter(lambda n: n.pid_error > 0, nodes.itervalues())

We could fix this by guarding every division by a length with a check.
Refactoring would make this much easier.

Child Tickets

Attachments (2)

0001-Ignore-node-classes-with-zero-members-rather-than-as.patch (2.8 KB) - added by teor 3 years ago.
Patch for this issue
0007-fixup-Ignore-node-classes-with-zero-members-rather-t.patch (1.2 KB) - added by teor 3 years ago.
Turns out that division by zero can happen in another location as well

Download all attachments as: .zip

Change History (5)

Changed 3 years ago by teor

Patch for this issue

comment:1 Changed 3 years ago by teor

Status: newneeds_review

Changed 3 years ago by teor

Turns out that division by zero can happen in another location as well

comment:2 Changed 3 years ago by tom

This is testing.

comment:3 Changed 3 years ago by tom

Resolution: fixed
Status: needs_reviewclosed

Merged

Note: See TracTickets for help on using tickets.