Skip to content

[RateLimiter] Resolve crash on near-round timestamps #44941

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 24, 2022
Merged

[RateLimiter] Resolve crash on near-round timestamps #44941

merged 1 commit into from
Jan 24, 2022

Conversation

xesxen
Copy link
Contributor

@xesxen xesxen commented Jan 7, 2022

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR -

Occasionally timestamps will be fully round (eg. 1234567890.000000) or close to it (eg. 1234567890.000031).

When converting those specific float timestamps to string in SlidingWindow (due to DateTimeImmutable::createFromFormat), the number is formatted by PHP without any digits. This causes the resulting value of SlidingWindow::getRetryAfter() to be violated with the boolean value false.

This patch formats the float to include the decimal digits.

Return value of Symfony\Component\RateLimiter\Policy\SlidingWindow::getRetryAfter() must be an instance of DateTimeImmutable, bool returned
#0 .../vendor/symfony/rate-limiter/Policy/SlidingWindowLimiter.php(84): Symfony\Component\RateLimiter\Policy\SlidingWindow->getRetryAfter()
#1 .../usercode.php(123): Symfony\Component\RateLimiter\Policy\SlidingWindowLimiter->consume(1)

@carsonbot carsonbot added this to the 5.4 milestone Jan 7, 2022
kbond
kbond approved these changes Jan 7, 2022
View reviewed changes
Copy link
Member

@kbond kbond left a comment

Choose a reason for hiding this comment

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

Interesting bug, good catch. This makes sense to me. @wouterj, wdyt?

GromNaN
GromNaN approved these changes Jan 12, 2022
View reviewed changes
Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

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

I confirm this behavior of DateTimeImmutable::createFromFormat and the resulting bug.
https://3v4l.org/37Qov

@xesxen xesxen force-pushed the ratelimit-fix-retryafter branch from 26640cc to 4965952 Compare January 23, 2022 16:26
@fabpot
Copy link
Member

fabpot commented Jan 24, 2022

Thank you @xesxen.

@fabpot fabpot merged commit 9738b1d into symfony:5.4 Jan 24, 2022
@nesl247
Copy link

nesl247 commented Jan 27, 2022

Is there any chance of getting a release for this? This explains why we are randomly seeing errors about TypeErrors in our application.

@kbond
Copy link
Member

kbond commented Jan 27, 2022

@nesl247, new releases are coming in the next few days.

This was referenced Jan 28, 2022
Merged
Merged
@wouterj wouterj mentioned this pull request Jan 31, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@kbond kbond kbond approved these changes

@GromNaN GromNaN GromNaN approved these changes

Assignees
No one assigned
Labels
Bug RateLimiter Status: Reviewed
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.
6 participants
@xesxen @fabpot @nesl247 @kbond @GromNaN @carsonbot