Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent Time::Span overflow #5306

Closed
wants to merge 13 commits into from
41 changes: 22 additions & 19 deletions src/time/span.cr
Original file line number Diff line number Diff line change
Expand Up @@ -60,13 +60,24 @@ struct Time::Span
def self.new(days : Int, hours : Int, minutes : Int, seconds : Int, nanoseconds : Int = 0)
new(
seconds: compute_seconds!(days, hours, minutes, seconds),
nanoseconds: nanoseconds.to_i64,
nanoseconds: nanoseconds,
)
end

def initialize(*, seconds : Int, nanoseconds : Int)
# check for possible overflow
# seconds could be too big
unless Int64::MIN <= seconds <= Int64::MAX
raise ArgumentError.new "Time::Span too big or too small"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about raising a runtime error... it would be safer to enforce value range through type safety. Meaning, a BigInt would need to be cast to Int64 by the user.

end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does a negative Time::Span make sense?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, why wouldn't it? If you subtract an earlier instance from a later one, the difference is a negative duration.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it does. Otherwise some operations are a bit hard to do.

seconds = seconds.to_i64
# Normalize nanoseconds in the range 0...1_000_000_000
seconds += nanoseconds.tdiv(NANOSECONDS_PER_SECOND)
sec = nanoseconds.tdiv(NANOSECONDS_PER_SECOND)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that sec itself could be > Int64::MAX here. That overflow case isn't handled (i think?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be, but then nanoseconds would need to be really big. tdiv returns the result value with the type of nanoseconds, so sec will have the same possible range as nanoseconds.
But because sec will be smaller than nanoseconds it fits and doesn't overflow.

if ((seconds > Int64::MAX - sec) && (sec > 0)) || ((sec < 0) && (seconds < Int64::MIN - sec))
raise ArgumentError.new "Time::Span too big or too small"
else
seconds += sec
end
nanoseconds = nanoseconds.remainder(NANOSECONDS_PER_SECOND)

# Make sure that if seconds is positive, nanoseconds is
Expand All @@ -86,8 +97,8 @@ struct Time::Span

def self.new(*, nanoseconds : Int)
new(
seconds: nanoseconds.to_i64.tdiv(NANOSECONDS_PER_SECOND),
nanoseconds: nanoseconds.to_i64.remainder(NANOSECONDS_PER_SECOND),
seconds: 0,
nanoseconds: nanoseconds,
)
end

Expand Down Expand Up @@ -279,7 +290,10 @@ struct Time::Span
end

def +(other : self) : Time::Span
# TODO check overflow
# check seconds for possible integer overflow
if ((other.to_i > 0) && (to_i > Int64::MAX - other.to_i)) || ((other.to_i < 0) && (to_i < Int64::MIN - other.to_i))
raise ArgumentError.new "Overflow: Time::Span too big or too small"
end
Span.new(
seconds: to_i + other.to_i,
nanoseconds: nanoseconds + other.nanoseconds,
Expand All @@ -293,24 +307,13 @@ struct Time::Span
# Returns a `Time::Span` that is *number* times longer.
def *(number : Number) : Time::Span
# TODO check overflow
Span.new(
seconds: to_i.to_i64 * number,
nanoseconds: nanoseconds.to_i64 * number,
)
(total_nanoseconds * number).nanoseconds
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method should still return a Time::Span, not nanoseconds as Int.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does return Time::Span, because (Float64 * Number) results in a Float64 and invoking Float#nanoseconds method returns a Time::Span.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, my bad.

end

# Return a `Time::Span` that is divided by *number*.
def /(number : Number) : Time::Span
seconds = to_i.tdiv(number)
nanoseconds = self.nanoseconds.tdiv(number)

remainder = to_i.remainder(number)
nanoseconds += (remainder * NANOSECONDS_PER_SECOND) / number

# TODO check overflow
Span.new(
seconds: seconds,
nanoseconds: nanoseconds,
)
(total_nanoseconds / number).nanoseconds
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

end

def /(other : self) : Float64
Expand Down