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