Skip to content

Improve leap years counting performance in distance_of_time_in_words #54923

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

Merged
merged 1 commit into from
Apr 15, 2025

Conversation

Stazer
Copy link
Contributor

@Stazer Stazer commented Apr 14, 2025

Motivation / Background

The distance_of_time_in_words function can lead to a denial of service if the given from_time and to_time arguments are far apart. I replaced the concerning code with a constant calculation.

Detail

The distance_of_time_in_words function counts the leap years by using a range and the count function:

  def distance_of_time_in_words(from_time, to_time = 0, options = {})
    # ...    
    leap_years = (from_year > to_year) ? 0 : (from_year..to_year).count { |x| Date.leap?(x) }
    # ...    

This code can lead to a denial of service if from_year and to_year are far apart, as it iterates over a range generated from the difference of the two arguments, effectively blocking execution. I replaced the code with the following, which calculates the leap years in constant time:

    leap_years_up_to = ->(year) { (year / 4) - (year / 100) + (year / 400) }
    leap_years = (from_year > to_year) ? 0 : leap_years_up_to.call(to_year) - leap_years_up_to.call(from_year - 1)

This particular case may seem irrelevant at first, but I discovered this problem in a project of mine. Users who are able to set a timestamp can easily trigger this problem when the timestamp is passed somewhere to the distance_of_time_in_words function.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Unrelated changes should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@rails-bot rails-bot bot added the actionview label Apr 14, 2025
@byroot byroot force-pushed the main branch 2 times, most recently from d016520 to f08836b Compare April 15, 2025 01:19
@byroot
Copy link
Member

byroot commented Apr 15, 2025

Nice trick. I just changed the code a bit not to rely on a lambda.

@byroot byroot merged commit 4ee7909 into rails:main Apr 15, 2025
2 of 3 checks passed
byroot added a commit that referenced this pull request Apr 15, 2025
Improve leap years counting performance in distance_of_time_in_words
byroot added a commit that referenced this pull request Apr 15, 2025
Improve leap years counting performance in distance_of_time_in_words
byroot added a commit that referenced this pull request Apr 15, 2025
Improve leap years counting performance in distance_of_time_in_words
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants