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

Add tests to cover EXPIRE overflow detection fix #9839

Merged
merged 1 commit into from
Nov 24, 2021

Conversation

enjoy-binbin
Copy link
Collaborator

In #8287, some overflow checks have been added. But when
when *= 1000 overflows, it will become a positive number.
And the check not able to catch it. The key will be added with
a short expiration time and will deleted a few seconds later.

In #9601, will check the overflow after *= and return an
error first, and avoiding this situation.

In this commit, added some tests to cover those code paths.
Found it in #9825, and close it.

In redis#8287, some overflow checks have been added. But when
`when *= 1000` overflows, it will become a positive number.
And the check not able to catch it. The key will be added with
a short expiration time and will deleted a few seconds later.

In redis#9601, will check the overflow after `*=` and return an
error first, and avoiding this situation.

In this commit, added some tests to cover those code paths.
Found it in redis#9825, and close it.
@oranagra oranagra linked an issue Nov 24, 2021 that may be closed by this pull request
@oranagra oranagra changed the title Add tests to cover EXPIRE overflow Add tests to cover EXPIRE overflow detection fix Nov 24, 2021
@oranagra oranagra added the release-notes indication that this issue needs to be mentioned in the release notes label Nov 24, 2021
@oranagra
Copy link
Member

@enjoy-binbin did you verify that this test fails on the code before the fix (v6.2)?

@enjoy-binbin
Copy link
Collaborator Author

Actually only this r EXPIRE foo 18446744073709561 will fail, in 6.2.6

My words is a bit unclear, when *= overflow can be negative or integer, for example:

expire foor 18446744063600560 -> `when *=` will became -10108991616
expire foor 18446744073709561 -> `when *=` will became 9384

In old code, if when *= 1000 overflow and became a small negative number, the when += basetime will correct it to be positive

    if (unit == UNIT_SECONDS) when *= 1000;
    when += basetime;

@oranagra
Copy link
Member

ok, great, i assumed only one of them is the one who fails on 6.2, but it's good you added all of them.

@oranagra oranagra merged commit 9273d09 into redis:unstable Nov 24, 2021
@enjoy-binbin enjoy-binbin deleted the add_expire_test branch November 24, 2021 07:48
hwware pushed a commit to hwware/redis that referenced this pull request Dec 20, 2021
In redis#8287, some overflow checks have been added. But when
`when *= 1000` overflows, it will become a positive number.
And the check not able to catch it. The key will be added with
a short expiration time and will deleted a few seconds later.

In redis#9601, will check the overflow after `*=` and return an
error first, and avoiding this situation.

In this commit, added some tests to cover those code paths.
Found it in redis#9825, and close it.
@oranagra oranagra added the breaking-change This change can potentially break existing application label Mar 22, 2022
This was referenced Apr 27, 2022
oranagra pushed a commit that referenced this pull request Apr 27, 2022
In #8287, some overflow checks have been added. But when
`when *= 1000` overflows, it will become a positive number.
And the check not able to catch it. The key will be added with
a short expiration time and will deleted a few seconds later.

In #9601, will check the overflow after `*=` and return an
error first, and avoiding this situation.

In this commit, added some tests to cover those code paths.
Found it in #9825, and close it.

(cherry picked from commit 9273d09)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change This change can potentially break existing application release-notes indication that this issue needs to be mentioned in the release notes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[BUG] EXPIRE and SETEX have inconsistent overflow handling
2 participants