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

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

Merged
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)

kbond
kbond approved these changes Jan 7, 2022
Copy link
Member

@kbond kbond left a comment

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

Copy link
Member

@GromNaN GromNaN left a comment

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 Jan 23, 2022
fabpot
fabpot approved these changes Jan 24, 2022
@fabpot
Copy link
Member

fabpot commented Jan 24, 2022

Thank you @xesxen.

@fabpot fabpot merged commit 9738b1d into symfony:5.4 Jan 24, 2022
7 of 11 checks passed
@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.

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.

None yet

6 participants