Opened 5 years ago

Closed 5 years ago

#13290 closed defect (fixed)

Avoid division by zero in circuitstats pareto calculations

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Keywords: tor-router needs-test
Cc: nickm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

In circuit_build_times_calculate_timeout() in circuitstats.c, avoid dividing by zero in the pareto calculations.

If either the alpha or p parameters are 0, we would divide by zero, yielding an infinite result; which would be clamped to INT32_MAX anyway. So rather than dividing by zero, we just skip the offending calculation(s), and just use INT32_MAX for the result.

Division by zero a crash under clang -fsanitize=undefined-trap -fsanitize-undefined-trap-on-error

I'll submit a github branch once I have the bug number for the changes file name.

tor version: 2.6.0-alpha-dev

Child Tickets

Attachments (1)

continuous-testing.sh (174 bytes) - added by teor 5 years ago.
Script for repeatedly testing for intermittent failures

Download all attachments as: .zip

Change History (9)

comment:1 Changed 5 years ago by teor

Status: newneeds_review

I've committed a fix to github at:

Branch: circuitstats-pareto-avoid-div-zero
Repository: ​https://github.com/teor2345/tor.git
Commit: 4d0ad34a92dff2be0b23e75ca5373054a5c9334a

tor tests now run without occasionally (due to random test data?) trapping on divide by zero in circuit_build_times_calculate_timeout() in circuitstats.c under clang -fsanitize=undefined-trap -fsanitize-undefined-trap-on-error.

Changed 5 years ago by teor

Attachment: continuous-testing.sh added

Script for repeatedly testing for intermittent failures

comment:2 Changed 5 years ago by teor

Attached a script that repeatedly runs make test until it fails. Good for testing for intermittent failures like this one.

Last edited 5 years ago by teor (previous) (diff)

comment:3 Changed 5 years ago by nickm

Keywords: 025-backport 024-backport added
Milestone: Tor: 0.2.6.x-final

comment:4 Changed 5 years ago by nickm

Keywords: 025-backport 024-backport removed
Resolution: fixed
Status: needs_reviewclosed

Merged to master; looks good.

(I've removed the backport tags, since in practice the behavior has been to have infinity get clamped to INT32_MAX.)

comment:5 Changed 5 years ago by teor

Keywords: needs-test added
Resolution: fixed
Status: closedreopened

This could do with unit tests for zero values.

comment:6 Changed 5 years ago by teor

Owner: set to teor
Status: reopenedassigned

comment:7 Changed 5 years ago by Sebastian

Status: assignedneeds_review

Added a unit test in branch bug13290 in my repo. I confirmed it traps the unpatched code.

comment:8 Changed 5 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

lgtm; merged!

Note: See TracTickets for help on using tickets.