Range-check the integral Rational offset - #183
Open
sjh9714 wants to merge 1 commit into
Open
Conversation
Every branch of offset_to_sec range-checks the resulting number of seconds except one path through the Rational branch: when the day fraction is an integral Rational, n is assigned inside the if arm and reaches *rof without passing the guard that sits in the else arm. DateTime.new(2024, 1, 1, 0, 0, 0, Rational(2, 1)) therefore produced a 48-hour offset, while the equivalent Integer 2 is rejected and falls back to +00:00. Rational(49710, 1) is 4_294_944_000 seconds, over INT_MAX, so the (int) narrowing turned a large positive offset into a negative one. Move the check below the if/else so it covers both arms. That also bounds n before the narrowing. Rational(1, 1) is exactly DAY_IN_SECONDS and the guard is inclusive, so in-range values are unaffected.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #181.
offset_to_secrange-checks the resulting number of seconds in every branchexcept one path through the Rational branch. When the day fraction is an
integral Rational,
nis assigned inside theifarm and reaches*rofwithout passing the
n < -DAY_IN_SECONDS || n > DAY_IN_SECONDSguard thatsits in the
elsearm.The Integer
2is rejected and falls back to+00:00, butRational(2, 1)isthe same quantity and yields a 48-hour offset.
Rational(49710, 1)is4_294_944_000 seconds, over
INT_MAX, so the(int)narrowing turns a largepositive offset into a negative one.
Changes
if/elseinoffset_to_secso itcovers both arms. This is the same check the
T_FIXNUM,T_FLOATandT_STRINGbranches already apply, and it boundsnbefore thelongtointnarrowing. Therounded:label path falls through to it as well.test_civil__offsetcovering the three values above plus the in-rangeboundary.
Rational(1, 1)is exactlyDAY_IN_SECONDSand the guard isinclusive, so
±24:00still round-trips.After the change all three expressions above produce
+00:00, matching theInteger path, and the constructor emits the usual
invalid offset is ignoredwarning.
Testing
bundle exec rake compilebundle exec rake test— 145 tests, 162600 assertions, 0 failures (144 testsbefore the new one; it fails on master and passes with the fix)
bundle exec rake— exit 0Run on ruby 4.0.6 (arm64-darwin25) against 83eb9d4.