From 162196671274de045fbce8f307d479a3a5586f0f Mon Sep 17 00:00:00 2001 From: Gnanesh Date: Sun, 21 Feb 2021 12:39:54 +0530 Subject: [PATCH] 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 Co-authored-by: Oran Agra --- src/expire.c | 9 ++++++-- src/t_string.c | 51 ++++++++++++++++++++++++------------------ tests/unit/expire.tcl | 52 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 88 insertions(+), 24 deletions(-) diff --git a/src/expire.c b/src/expire.c index 0ce34aea8..982301542 100644 --- a/src/expire.c +++ b/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); diff --git a/src/t_string.c b/src/t_string.c index 6a3b256b8..cdf69110f 100644 --- a/src/t_string.c +++ b/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) { - addReplyErrorFormat(c,"invalid expire time in %s",c->cmd->name); + 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; - - if ((flags & OBJ_PX) || (flags & OBJ_EX)) { - setExpire(c,c->db,key,milliseconds + mstime()); - exp = shared.px; - } else { - setExpire(c,c->db,key,milliseconds); - } + setExpire(c,c->db,key,when); 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) { - addReplyErrorFormat(c,"invalid expire time in %s",c->cmd->name); + 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; - if ((flags & OBJ_PX) || (flags & OBJ_EX)) { - setExpire(c,c->db,c->argv[1],milliseconds + mstime()); - exp = shared.pexpire; - } else { - setExpire(c,c->db,c->argv[1],milliseconds); - } - + setExpire(c,c->db,c->argv[1],when); + /* Propagate */ + robj *exp = (flags & OBJ_PXAT) || (flags & OBJ_EXAT) ? shared.pexpireat : shared.pexpire; robj* millisecondObj = createStringObjectFromLongLong(milliseconds); rewriteClientCommandVector(c,3,exp,c->argv[1],millisecondObj); decrRefCount(millisecondObj); diff --git a/tests/unit/expire.tcl b/tests/unit/expire.tcl index 9bde4809f..900856848 100644 --- a/tests/unit/expire.tcl +++ b/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