Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#13393 closed defect (fixed)

Avoid signed overflow in format_time_interval, add unit tests

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

Description

I've discovered a signed overflow in format_time_interval while writing unit tests for it.

The signed overflow occurs when the function takes the absolute value of LONG_MIN (which is unlikely in practice). To avoid the overflow, I've patched it to use LONG_MAX when format_time_interval is passed LONG_MIN. (This doesn't change the output at all, as seconds aren't printed in this case.)

I've also created comprehensive unit tests.

tor version: 2.6.0-alpha
tor git: f94e5f2e5212034cb8b2666716eeaa61e874065b

I'll post the GitHub branch after I've created the changes file.

Child Tickets

Change History (4)

comment:1 Changed 5 years ago by teor

Git blame says that this code goes all the way back to:
r16279@catbus: nickm | 2007-10-30 11:14:29 -0400
Improved skew reporting: "You are 365 days in the duture" is more useful than "You are 525600 minutes in the future". Also, when we get something that proves we are at least an hour in the past, tell the controller "CLOCK_SKEW MIN_SKEW=-3600" rather than just "CLOCK_SKEW"
svn:r12283

Nick(?), I'll let you decide how far back to backport it, if at all.
(Do we backport unit tests? If not, I can split them from the function commit.)

Repository: ​​​https://github.com/teor2345/tor.git

Branch: format-time-interval-overflow-test
Avoid overflow in format_time_interval, create unit tests

comment:2 Changed 5 years ago by nickm

Milestone: Tor: 0.2.6.x-final
Resolution: fixed
Status: newclosed

Looks good to me. I'm going to suggest "no backport" for the whole thing, since an interval of LONG_MIN is not actually likely in practice. Merging to master. Thanks!

comment:3 Changed 5 years ago by nickm

BTW, your text editor probably has a feature that will make it highlight whitespace at the end of a line. If you turn that on, it will be easy to make sure your patches don't have ragged trailing whitespace.

comment:4 Changed 5 years ago by teor

I'm using Xcode - I've changed the settings so it trims whitespace on empty lines as well.
No highlighting, unfortunately. I'll try to remember to run make check-spaces before I commit.

Note: See TracTickets for help on using tickets.