EXPIRE, EXPIREAT, SETEX, GETEX: Return error when expire time overflows (#8287)

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

* `SETEX`, `SET EX`, `GETEX` etc would have already error on negative value,
but now they would also error on overflows (i.e. when the input was positive but
after the manipulation it becomes negative, which would have passed before)
* `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 (except the case of changing sign when `basetime` is added)

Signed-off-by: Gnanesh <gnaneshkunal@outlook.com>
Co-authored-by: Oran Agra <oran@redislabs.com>

src/expire.c

@@ -506,10 +506,15 @@ void expireGenericCommand(client *c, long long basetime, int unit) {
 
     if (getLongLongFromObjectOrReply(c, param, &when, NULL) != C_OK)
         return;
-
+    int negative_when = when < 0;
     if (unit == UNIT_SECONDS) when *= 1000;
     when += basetime;
-
+    if (((when < 0) && !negative_when) || ((when-basetime > 0) && negative_when)) {
+        /* EXPIRE allows negative numbers, but we can at least detect an
+         * overflow by either unit conversion or basetime addition. */
+        addReplyErrorFormat(c, "invalid expire time in %s", c->cmd->name);
+        return;
+    }
     /* No key, return zero. */
     if (lookupKeyWrite(c->db,key) == NULL) {
         addReply(c,shared.czero);

src/t_string.c

@@ -73,16 +73,25 @@ static int checkStringLength(client *c, long long size) {
 #define OBJ_PERSIST (1<<8)         /* Set if we need to remove the ttl */
 
 void setGenericCommand(client *c, int flags, robj *key, robj *val, robj *expire, int unit, robj *ok_reply, robj *abort_reply) {
-    long long milliseconds = 0; /* initialized to avoid any harmness warning */
+    long long milliseconds = 0, when = 0; /* initialized to avoid any harmness warning */
 
     if (expire) {
         if (getLongLongFromObjectOrReply(c, expire, &milliseconds, NULL) != C_OK)
             return;
-        if (milliseconds <= 0) {
+        if (milliseconds <= 0 || (unit == UNIT_SECONDS && milliseconds >= LLONG_MAX / 1000)) {
+            /* Negative value provided or multiplication is gonna overflow. */
             addReplyErrorFormat(c, "invalid expire time in %s", c->cmd->name);
             return;
         }
         if (unit == UNIT_SECONDS) milliseconds *= 1000;
+        when = milliseconds;
+        if ((flags & OBJ_PX) || (flags & OBJ_EX))
+            when += mstime();
+        if (when <= 0) {
+            /* Overflow detected. */
+            addReplyErrorFormat(c, "invalid expire time in %s", c->cmd->name);
+            return;
+        }
     }
 
     if ((flags & OBJ_SET_NX && lookupKeyWrite(c->db,key) != NULL) ||
@@ -100,14 +109,7 @@ void setGenericCommand(client *c, int flags, robj *key, robj *val, robj *expire,
     server.dirty++;
     notifyKeyspaceEvent(NOTIFY_STRING,"set",key,c->db->id);
     if (expire) {
-        robj *exp = shared.pxat;
+        setExpire(c,c->db,key,when);
-
-        if ((flags & OBJ_PX) || (flags & OBJ_EX)) {
-            setExpire(c,c->db,key,milliseconds + mstime());
-            exp = shared.px;
-        } else {
-            setExpire(c,c->db,key,milliseconds);
-        }
         notifyKeyspaceEvent(NOTIFY_GENERIC,"expire",key,c->db->id);
 
         /* Propagate as SET Key Value PXAT millisecond-timestamp if there is EXAT/PXAT or
@@ -119,6 +121,7 @@ void setGenericCommand(client *c, int flags, robj *key, robj *val, robj *expire,
          * Additional care is required while modifying the argument order. AOF relies on the
          * exp argument being at index 3. (see feedAppendOnlyFile)
          * */
+        robj *exp = (flags & OBJ_PXAT) || (flags & OBJ_EXAT) ? shared.pxat : shared.px;
         robj *millisecondObj = createStringObjectFromLongLong(milliseconds);
         rewriteClientCommandVector(c,5,shared.set,key,val,exp,millisecondObj);
         decrRefCount(millisecondObj);
@@ -335,17 +338,26 @@ void getexCommand(client *c) {
         return;
     }
 
-    long long milliseconds = 0;
+    long long milliseconds = 0, when = 0;
 
     /* Validate the expiration time value first */
     if (expire) {
         if (getLongLongFromObjectOrReply(c, expire, &milliseconds, NULL) != C_OK)
             return;
-        if (milliseconds <= 0) {
+        if (milliseconds <= 0 || (unit == UNIT_SECONDS && milliseconds >= LLONG_MAX / 1000)) {
+            /* Negative value provided or multiplication is gonna overflow. */
             addReplyErrorFormat(c, "invalid expire time in %s", c->cmd->name);
             return;
         }
         if (unit == UNIT_SECONDS) milliseconds *= 1000;
+        when = milliseconds;
+        if ((flags & OBJ_PX) || (flags & OBJ_EX))
+            when += mstime();
+        if (when <= 0) {
+            /* Overflow detected. */
+            addReplyErrorFormat(c, "invalid expire time in %s", c->cmd->name);
+            return;
+        }
     }
 
     /* We need to do this before we expire the key or delete it */
@@ -365,14 +377,9 @@ void getexCommand(client *c) {
         notifyKeyspaceEvent(NOTIFY_GENERIC, "del", c->argv[1], c->db->id);
         server.dirty++;
     } else if (expire) {
-        robj *exp = shared.pexpireat;
+        setExpire(c,c->db,c->argv[1],when);
-        if ((flags & OBJ_PX) || (flags & OBJ_EX)) {
+        /* Propagate */
-            setExpire(c,c->db,c->argv[1],milliseconds + mstime());
+        robj *exp = (flags & OBJ_PXAT) || (flags & OBJ_EXAT) ? shared.pexpireat : shared.pexpire;
-            exp = shared.pexpire;
-        } else {
-            setExpire(c,c->db,c->argv[1],milliseconds);
-        }
-
         robj* millisecondObj = createStringObjectFromLongLong(milliseconds);
         rewriteClientCommandVector(c,3,exp,c->argv[1],millisecondObj);
         decrRefCount(millisecondObj);

tests/unit/expire.tcl

@@ -209,6 +209,58 @@ start_server {tags {"expire"}} {
         set e
     } {*not an integer*}
 
+    test {SET with EX with big integer should report an error} {
+        catch {r set foo bar EX 10000000000000000} e
+        set e
+    } {ERR invalid expire time in set}
+
+    test {SET with EX with smallest integer should report an error} {
+        catch {r SET foo bar EX -9999999999999999} e
+        set e
+    } {ERR invalid expire time in set}
+
+    test {GETEX with big integer should report an error} {
+        r set foo bar
+        catch {r GETEX foo EX 10000000000000000} e
+        set e
+    } {ERR invalid expire time in getex}
+
+    test {GETEX with smallest integer should report an error} {
+        r set foo bar
+        catch {r GETEX foo EX -9999999999999999} e
+        set e
+    } {ERR invalid expire time in getex}
+
+    test {EXPIRE with big integer overflows when converted to milliseconds} {
+        r set foo bar
+        catch {r EXPIRE foo 10000000000000000} e
+        set e
+    } {ERR invalid expire time in expire}
+
+    test {PEXPIRE with big integer overflow when basetime is added} {
+        r set foo bar
+        catch {r PEXPIRE foo 9223372036854770000} e
+        set e
+    } {ERR invalid expire time in pexpire}
+
+    test {EXPIRE with big negative integer} {
+        r set foo bar
+        catch {r EXPIRE foo -9999999999999999} e
+        assert_match {ERR invalid expire time in expire} $e
+        r ttl foo
+    } {-1}
+
+    test {PEXPIREAT with big integer works} {
+        r set foo bar
+        r PEXPIREAT foo 9223372036854770000
+    } {1}
+
+    test {PEXPIREAT with big negative integer works} {
+        r set foo bar
+        r PEXPIREAT foo -9223372036854770000
+        r ttl foo
+    } {-2}
+
     test {EXPIRE and SET/GETEX EX/PX/EXAT/PXAT option, TTL should not be reset after loadaof} {
         # This test makes sure that expire times are propagated as absolute
         # times to the AOF file and not as relative time, so that when the AOF