From 9a2c6ba4e7be4121044a1ef048e953333535caf5 Mon Sep 17 00:00:00 2001 From: "debing.sun" Date: Tue, 4 Jun 2024 20:35:26 +0800 Subject: [PATCH] Prevent negative expire parameter in HEXPIRE/HEXPIREAT/HPEXPIRE/HPEXPIREAT commands (#13310) 1. Don't allow HEXPIRE/HEXPIREAT/HPEXPIRE/HPEXPIREAT command expire parameters is negative 2. Remove a dead code reported from Coverity. when `unit` is not `UNIT_SECONDS`, the second `if (expire > (long long) EB_EXPIRE_TIME_MAX)` will be dead code. ```c # t_hash.c 2988 /* Check expire overflow */ cond_at_most: Condition expire > 281474976710655LL, taking false branch. Now the value of expire is at most 281474976710655. 2989 if (expire > (long long) EB_EXPIRE_TIME_MAX) { 2990 addReplyErrorExpireTime(c); 2991 return; 2992 } 2994 if (unit == UNIT_SECONDS) { 2995 if (expire > (long long) EB_EXPIRE_TIME_MAX / 1000) { 2996 addReplyErrorExpireTime(c); 2997 return; 2998 } 2999 expire *= 1000; 3000 } else { at_most: At condition expire > 281474976710655LL, the value of expire must be at most 281474976710655. dead_error_condition: The condition expire > 281474976710655LL cannot be true. 3001 if (expire > (long long) EB_EXPIRE_TIME_MAX) { CID 494223: (#1 of 1): Logically dead code (DEADCODE) dead_error_begin: Execution cannot reach this statement: addReplyErrorExpireTime(c);. 3002 addReplyErrorExpireTime(c); 3003 return; 3004 } 3005 } ``` --------- Co-authored-by: Ozan Tezcan --- src/t_hash.c | 18 ++++++------------ tests/unit/type/hash-field-expire.tcl | 15 ++++++++++++++- 2 files changed, 20 insertions(+), 13 deletions(-) diff --git a/src/t_hash.c b/src/t_hash.c index 0463e09b4..efb489265 100644 --- a/src/t_hash.c +++ b/src/t_hash.c @@ -2869,7 +2869,7 @@ static void httlGenericCommand(client *c, const char *cmd, long long basetime, i /* Verify `numFields` is consistent with number of arguments */ if (numFields > (c->argc - numFieldsAt - 1)) { - addReplyError(c, "Parameter `numFileds` is more than number of arguments"); + addReplyError(c, "Parameter `numFields` is more than number of arguments"); return; } @@ -2983,10 +2983,8 @@ static void hexpireGenericCommand(client *c, const char *cmd, long long basetime /* Read the expiry time from command */ if (getLongLongFromObjectOrReply(c, expireArg, &expire, NULL) != C_OK) return; - - /* Check expire overflow */ - if (expire > (long long) EB_EXPIRE_TIME_MAX) { - addReplyErrorExpireTime(c); + if (expire < 0) { + addReplyError(c,"invalid expire time, must be >= 0"); return; } @@ -2996,13 +2994,9 @@ static void hexpireGenericCommand(client *c, const char *cmd, long long basetime return; } expire *= 1000; - } else { - if (expire > (long long) EB_EXPIRE_TIME_MAX) { - addReplyErrorExpireTime(c); - return; - } } + /* Ensure that the final absolute Unix timestamp does not exceed EB_EXPIRE_TIME_MAX. */ if (expire > (long long) EB_EXPIRE_TIME_MAX - basetime) { addReplyErrorExpireTime(c); return; @@ -3033,7 +3027,7 @@ static void hexpireGenericCommand(client *c, const char *cmd, long long basetime /* Verify `numFields` is consistent with number of arguments */ if (numFields > (c->argc - numFieldsAt - 1)) { - addReplyError(c, "Parameter `numFileds` is more than number of arguments"); + addReplyError(c, "Parameter `numFields` is more than number of arguments"); return; } @@ -3130,7 +3124,7 @@ void hpersistCommand(client *c) { /* Verify `numFields` is consistent with number of arguments */ if (numFields > (c->argc - numFieldsAt - 1)) { - addReplyError(c, "Parameter `numFileds` is more than number of arguments"); + addReplyError(c, "Parameter `numFields` is more than number of arguments"); return; } diff --git a/tests/unit/type/hash-field-expire.tcl b/tests/unit/type/hash-field-expire.tcl index ef5217068..1c9084861 100644 --- a/tests/unit/type/hash-field-expire.tcl +++ b/tests/unit/type/hash-field-expire.tcl @@ -128,6 +128,19 @@ start_server {tags {"external:skip needs:debug"}} { r readraw 0 } + test "HEXPIRE/HEXPIREAT/HPEXPIRE/HPEXPIREAT - Verify that the expire time does not overflow" { + r del myhash + r hset myhash f1 v1 + # The expire time can't be negative. + assert_error {ERR invalid expire time, must be >= 0} {r HEXPIRE myhash -1 FIELDS 1 f1} + assert_error {ERR invalid expire time, must be >= 0} {r HEXPIRE myhash -9223372036854775808 FIELDS 1 f1} + # The expire time can't be greater than the EB_EXPIRE_TIME_MAX + assert_error {ERR invalid expire time in 'hexpire' command} {r HEXPIRE myhash [expr (1<<48) / 1000] FIELDS 1 f1} + assert_error {ERR invalid expire time in 'hexpireat' command} {r HEXPIREAT myhash [expr (1<<48) / 1000 + [clock seconds] + 100] FIELDS 1 f1} + assert_error {ERR invalid expire time in 'hpexpire' command} {r HPEXPIRE myhash [expr (1<<48)] FIELDS 1 f1} + assert_error {ERR invalid expire time in 'hpexpireat' command} {r HPEXPIREAT myhash [expr (1<<48) + [clock milliseconds] + 100] FIELDS 1 f1} + } + test "HPEXPIRE(AT) - Test 'NX' flag ($type)" { r del myhash r hset myhash field1 value1 field2 value2 field3 value3 @@ -192,7 +205,7 @@ start_server {tags {"external:skip needs:debug"}} { r del myhash r hset myhash f1 v1 assert_error {*Parameter `numFields` should be greater than 0} {r hpexpire myhash 1000 NX FIELDS 0 f1 f2 f3} - assert_error {*Parameter `numFileds` is more than number of arguments} {r hpexpire myhash 1000 NX FIELDS 4 f1 f2 f3} + assert_error {*Parameter `numFields` is more than number of arguments} {r hpexpire myhash 1000 NX FIELDS 4 f1 f2 f3} } test "HPEXPIRE - parameter expire-time near limit of 2^48 ($type)" {