Describe the bug

SETEX is documented as being equivalent to doing (atomically) SET followed by EXPIRE. However, the code for checking for overflow in EXPIRE is different and only reports an invalid expiry time if multiplying by 1000 gives a negative time.

To reproduce

SETEX rejects an expiry time that is more than 2^63 ms:

127.0.0.1:6379> setex foo 18446744073709561 blah
(error) ERR invalid expire time in setex

EXPIRE accepts it, and expires the key about 10 seconds later.

127.0.0.1:6379> set foo bar
OK
127.0.0.1:6379> expire foo 18446744073709561
(integer) 1

Expected behavior

Expected EXPIRE to match the behaviour of SETEX, and reject expiry times that overflow when converted to absolute milliseconds. It looks like this helper function might be useful for having EXPIRE apply the same logic.

Additional information

By eyeball, #9601 looks like it might fix the bug. The PR description suggests that the author might not have realised that this fixes an actual bug rather than just a UBSan violation, and that the intent was not to backport it to stable branches (cc @tezc @oranagra).

Comment From: enjoy-binbin

for the record, based on the latest unstable branch

127.0.0.1:6379> setex foo 18446744073709561 blah
(error) ERR invalid expire time in setex
127.0.0.1:6379> set foo bar
OK
127.0.0.1:6379> expire foo 18446744073709561
(error) ERR invalid expire time in expire

Comment From: tygzx

In the version of redis 6.2,the setex command would check the expire time is less than LLONG_MAX / 1000, But the exipre command not check that. In the version of unstable branch. expire command check .so the bug is fixed.

Comment From: tezc

I wasn't aware of the bug. I thought it covered all the overflow scenarios previously but it was relying on an UB / implementation defined behavior. So, I've changed that part a bit to follow language rules and make the sanitizer happy.

Code before my PR : 

    int negative_when = when < 0;
    if (unit == UNIT_SECONDS) when *= 1000;
    when += basetime;
    if (((when < 0) && !negative_when) || ((when-basetime > 0) && negative_when)) {
        addReplyErrorFormat(c, "invalid expire time in %s", c->cmd->name);
        return;
    }

Looks like GCC generates a positive number for when *= 1000 on overflow. So, 'if check' below wouldn't catch it.

Comment From: oranagra

the original PR that added these checks is https://github.com/redis/redis/pull/8287 where it says:

Respond with error if expire time overflows from positive to negative of vice versa

so maybe in that sense it was not a bug if the overflow check failed to detect it. but actually, if we disregard the wording, an overflow does happen (just doesn't result in negative). as is clear from the fact the key is deleted a few seconds later.

i can mark this issue to be backported, to 6.2 and use that as a trigger to backport the overflow check fix. or maybe a better alternative is to add a test that verifies this behavior, and i'll mark that one for backport. @bmerry wanna make a PR with a test?

p.s. note that EXPIRE and SETEX are not meant to be consistent:

EXPIRE and EXPIREAT was ok taking negative values (would implicitly delete the key), we keep that, but we do error if the user provided a value that changes sign when manipulated

Comment From: bmerry

@bmerry wanna make a PR with a test?

I'm afraid I'm not going to have the time - I'm only spending one day a week in a project of which a small part of that time is spent on a package in which I've discovered this bug.

Comment From: enjoy-binbin

Looks like GCC generates a positive number for when *= 1000 on overflow. So, 'if check' below wouldn't catch it.

I have some times, i can add a test to cover it, improve the test