From 732e466a6a593c8ead1cecfddba0ca0fc1e49d35 Mon Sep 17 00:00:00 2001 From: michael-grunder Date: Wed, 21 Feb 2024 10:11:35 -0800 Subject: [PATCH] Improve warning when we encounter an invalid EXPIRY in SET We actually had two different bits of logic to handle EXPIRY values in the `SET` command. One for the legacy `SET` -> `SETEX` mapping and another for the newer `SET foo bar EX `. Additionally the error message could be confusing. Passing 3.1415 for an `EX` expiry would fail as we didn't allow floats. This commit consolidates expiry parsing to our existing helper function as well as improves the `php_error_docref` warning in the event that the user passes invalid data. The warning will now tell the user the type they tried to pass as an EXPIRY to make it easier to track down what's going wrong. Fixes #2448 --- redis_commands.c | 40 +++++++++++++++++++--------------------- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/redis_commands.c b/redis_commands.c index 906eb23..df72638 100644 --- a/redis_commands.c +++ b/redis_commands.c @@ -2291,14 +2291,19 @@ static int redis_try_get_expiry(zval *zv, zend_long *lval) { int redis_set_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock, char **cmd, int *cmd_len, short *slot, void **ctx) { - smart_string cmdstr = {0}; - zval *z_value, *z_opts=NULL; char *key = NULL, *exp_type = NULL, *set_type = NULL; - long exp_set = 0, keep_ttl = 0; + zval *z_value, *z_opts=NULL; + smart_string cmdstr = {0}; zend_long expire = -1; zend_bool get = 0; + long keep_ttl = 0; size_t key_len; + #define setExpiryWarning(zv) \ + php_error_docref(NULL, E_WARNING, "%s passed as EXPIRY is invalid " \ + "(must be an int, float, or numeric string >= 1)", \ + zend_zval_type_name((zv))) + // Make sure the function is being called correctly if (zend_parse_parameters(ZEND_NUM_ARGS(), "sz|z", &key, &key_len, &z_value, &z_opts) == FAILURE) @@ -2306,6 +2311,7 @@ int redis_set_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock, return FAILURE; } + // Check for an options array if (z_opts && Z_TYPE_P(z_opts) == IS_ARRAY) { HashTable *kt = Z_ARRVAL_P(z_opts); @@ -2321,17 +2327,12 @@ int redis_set_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock, zend_string_equals_literal_ci(zkey, "EXAT") || zend_string_equals_literal_ci(zkey, "PXAT")) ) { - exp_set = 1; - - /* Set expire type */ - exp_type = ZSTR_VAL(zkey); - - /* Try to extract timeout */ - if (Z_TYPE_P(v) == IS_LONG) { - expire = Z_LVAL_P(v); - } else if (Z_TYPE_P(v) == IS_STRING) { - expire = atol(Z_STRVAL_P(v)); + if (redis_try_get_expiry(v, &expire) == FAILURE || expire < 1) { + setExpiryWarning(v); + return FAILURE; } + + exp_type = ZSTR_VAL(zkey); } else if (Z_TYPE_P(v) == IS_STRING) { if (zend_string_equals_literal_ci(Z_STR_P(v), "KEEPTTL")) { keep_ttl = 1; @@ -2345,19 +2346,14 @@ int redis_set_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock, } } ZEND_HASH_FOREACH_END(); } else if (z_opts && Z_TYPE_P(z_opts) != IS_NULL) { - if (redis_try_get_expiry(z_opts, &expire) == FAILURE) { - php_error_docref(NULL, E_WARNING, "Expire must be a long, double, or a numeric string"); + if (redis_try_get_expiry(z_opts, &expire) == FAILURE || expire < 1) { + setExpiryWarning(z_opts); return FAILURE; } - - exp_set = 1; } /* Protect the user from syntax errors but give them some info about what's wrong */ - if (exp_set && expire < 1) { - php_error_docref(NULL, E_WARNING, "EXPIRE can't be < 1"); - return FAILURE; - } else if (exp_type && keep_ttl) { + if (exp_type && keep_ttl) { php_error_docref(NULL, E_WARNING, "KEEPTTL can't be combined with EX or PX option"); return FAILURE; } @@ -2396,6 +2392,8 @@ int redis_set_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock, *cmd_len = cmdstr.len; return SUCCESS; + + #undef setExpiryWarning } /* MGET */