Skip to content

Range-check the integral Rational offset - #183

Open
sjh9714 wants to merge 1 commit into
ruby:masterfrom
sjh9714:codex/20260819-181-offset-rational-range-check
Open

Range-check the integral Rational offset#183
sjh9714 wants to merge 1 commit into
ruby:masterfrom
sjh9714:codex/20260819-181-offset-rational-range-check

Conversation

@sjh9714

@sjh9714 sjh9714 commented Aug 19, 2026

Copy link
Copy Markdown

Summary

Fixes #181.

offset_to_sec range-checks the resulting number of seconds in every branch
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 n < -DAY_IN_SECONDS || n > DAY_IN_SECONDS guard that
sits in the else arm.

DateTime.new(2024, 1, 1, 0, 0, 0, 2).iso8601                  # => "2024-01-01T00:00:00+00:00"
DateTime.new(2024, 1, 1, 0, 0, 0, Rational(2, 1)).iso8601     # => "2024-01-01T00:00:00+48:00"
DateTime.new(2024, 1, 1, 0, 0, 0, Rational(49710, 1)).iso8601 # => "2024-01-01T00:00:00-06:28"

The Integer 2 is rejected and falls back to +00:00, but Rational(2, 1) is
the same quantity and yields a 48-hour offset. Rational(49710, 1) is
4_294_944_000 seconds, over INT_MAX, so the (int) narrowing turns a large
positive offset into a negative one.

Changes

  • Move the existing range check below the if/else in offset_to_sec so it
    covers both arms. This is the same check the T_FIXNUM, T_FLOAT and
    T_STRING branches already apply, and it bounds n before the long to
    int narrowing. The rounded: label path falls through to it as well.
  • Add test_civil__offset covering the three values above plus the in-range
    boundary. Rational(1, 1) is exactly DAY_IN_SECONDS and the guard is
    inclusive, so ±24:00 still round-trips.

After the change all three expressions above produce +00:00, matching the
Integer path, and the constructor emits the usual invalid offset is ignored
warning.

Testing

  • bundle exec rake compile
  • bundle exec rake test — 145 tests, 162600 assertions, 0 failures (144 tests
    before the new one; it fails on master and passes with the fix)
  • bundle exec rake — exit 0

Run on ruby 4.0.6 (arm64-darwin25) against 83eb9d4.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

offset_to_sec skips its range check for integral Rationals, so DateTime accepts a 48-hour offset (and wraps the sign at 2**31)

1 participant