Skip to content

Commit

Permalink
Validate the set, mset, setrange and similar refuse to create empty k…
Browse files Browse the repository at this point in the history 
…eys (#201)

This PR introduces some constraints on the minimum length of keys,
patterns and short strings handled by the Redis command parser to ensure
that if an argument of that type is zero-length will be rejected.

Tests are introduced as well to validate the rejection.

On a side, the PR also changes some code to take into account the
returned value of module_redis_connection_error_message_printf_*

Closes #199
  • Loading branch information
@danielealbano
danielealbano authored Aug 29, 2022
1 parent 6ce1c1d commit 93246b1
Show file tree
Hide file tree
Showing 5 changed files with 63 additions and 28 deletions.
3 changes: 1 addition & 2 deletions src/module/redis/command/module_redis_command_mset.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,7 @@ MODULE_REDIS_COMMAND_FUNCPTR_COMMAND_END(mset) {
key_value->key.value.length,
key_value->value.value.chunk_sequence,
STORAGE_DB_ENTRY_NO_EXPIRY)) {
module_redis_connection_error_message_printf_noncritical(connection_context, "ERR mset failed");
return true;
return module_redis_connection_error_message_printf_noncritical(connection_context, "ERR mset failed");
}

// Mark both the key and the chunk_sequence as NULL as the storage db now owns them, we don't want them to be
Expand Down
6 changes: 2 additions & 4 deletions src/module/redis/command/module_redis_command_msetnx.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,7 @@ MODULE_REDIS_COMMAND_FUNCPTR_COMMAND_END(msetnx) {
key_value->key.value.length,
&rmw_statuses[index],
&entry_index))) {
module_redis_connection_error_message_printf_noncritical(connection_context, "ERR msetnx failed");
return_res = true;
return_res = module_redis_connection_error_message_printf_noncritical(connection_context, "ERR msetnx failed");
can_commit_update = false;
error_found = true;
break;
Expand Down Expand Up @@ -102,8 +101,7 @@ MODULE_REDIS_COMMAND_FUNCPTR_COMMAND_END(msetnx) {
&rmw_statuses[index],
key_value->value.value.chunk_sequence,
STORAGE_DB_ENTRY_NO_EXPIRY))) {
module_redis_connection_error_message_printf_noncritical(connection_context, "ERR msetnx failed");
return_res = true;
return_res = module_redis_connection_error_message_printf_noncritical(connection_context, "ERR msetnx failed");
error_found = true;
break;
}
Expand Down
44 changes: 26 additions & 18 deletions src/module/redis/module_redis_command.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -281,21 +281,19 @@ bool module_redis_command_process_argument_begin(
connection_context,
expected_argument,
argument_length)) {
module_redis_connection_error_message_printf_noncritical(
return module_redis_connection_error_message_printf_noncritical(
connection_context,
"ERR The %s length has exceeded the allowed size of '%u'",
expected_argument->type == MODULE_REDIS_COMMAND_ARGUMENT_TYPE_KEY ? "key" : "pattern",
connection_context->network_channel->module_config->redis->max_key_length);
return true;
}

if (expected_argument->type == MODULE_REDIS_COMMAND_ARGUMENT_TYPE_LONG_STRING) {
if (!storage_db_chunk_sequence_is_size_allowed(argument_length)) {
module_redis_connection_error_message_printf_noncritical(
return module_redis_connection_error_message_printf_noncritical(
connection_context,
"ERR The argument length has exceeded the allowed size of '%lu'",
storage_db_chunk_sequence_allowed_max_size());
return true;
}

command_parser_context->current_argument.member_context_addr =
Expand Down Expand Up @@ -477,13 +475,12 @@ bool module_redis_command_process_argument_full(
base_addr);

if (has_token) {
module_redis_connection_error_message_printf_noncritical(
return module_redis_connection_error_message_printf_noncritical(
connection_context,
"ERR the command '%s' doesn't support both the parameters '%s' and '%s' set at the same time",
connection_context->command.info->string,
oneof_token_entry->token,
token_entry->token);
return true;
}
}

Expand All @@ -501,12 +498,11 @@ bool module_redis_command_process_argument_full(
base_addr);

if (has_token && !token_entry->argument->has_multiple_token) {
module_redis_connection_error_message_printf_noncritical(
return module_redis_connection_error_message_printf_noncritical(
connection_context,
"ERR the parameter '%s' has already been specified for the command '%s'",
token_entry->token,
connection_context->command.info->string);
return true;
}
}

Expand Down Expand Up @@ -537,10 +533,9 @@ bool module_redis_command_process_argument_full(
if (guessed_argument == NULL) {
// Although the error message can be much smarter, as was being in a previous iteration, to match the Redis
// behaviour the error message has been changed simply to "syntax error".
module_redis_connection_error_message_printf_noncritical(
return module_redis_connection_error_message_printf_noncritical(
connection_context,
"ERR syntax error");
return true;
}

// If the tokens have been checked and the guessed argument has a token, it can be set straight to true and the
Expand Down Expand Up @@ -589,12 +584,27 @@ bool module_redis_command_process_argument_full(
connection_context,
guessed_argument,
chunk_length)) {
module_redis_connection_error_message_printf_noncritical(
return module_redis_connection_error_message_printf_noncritical(
connection_context,
"ERR The %s length has exceeded the allowed size of '%u'",
expected_argument->type == MODULE_REDIS_COMMAND_ARGUMENT_TYPE_KEY ? "key" : "pattern",
expected_argument->type == MODULE_REDIS_COMMAND_ARGUMENT_TYPE_KEY
? "key"
: (expected_argument->type == MODULE_REDIS_COMMAND_ARGUMENT_TYPE_PATTERN
? "pattern"
: "short string"),
connection_context->network_channel->module_config->redis->max_key_length);
return true;
}

if (unlikely(chunk_length == 0)) {
return module_redis_connection_error_message_printf_noncritical(
connection_context,
"ERR the %s '%s' has length '0', not allowed",
expected_argument->type == MODULE_REDIS_COMMAND_ARGUMENT_TYPE_KEY
? "key"
: (expected_argument->type == MODULE_REDIS_COMMAND_ARGUMENT_TYPE_PATTERN
? "pattern"
: "short string"),
guessed_argument->name);
}

string_value = slab_allocator_mem_alloc(chunk_length);
Expand All @@ -604,7 +614,7 @@ bool module_redis_command_process_argument_full(
return false;
}

strncpy(string_value, chunk_data, chunk_length);
memcpy(string_value, chunk_data, chunk_length);

if (guessed_argument->type == MODULE_REDIS_COMMAND_ARGUMENT_TYPE_KEY) {
module_redis_key_t *key = base_addr;
Expand All @@ -628,11 +638,10 @@ bool module_redis_command_process_argument_full(
*integer_value = strtoll(chunk_data, &integer_value_end_ptr, 10);

if (errno == ERANGE || integer_value_end_ptr != chunk_data + chunk_length) {
module_redis_connection_error_message_printf_noncritical(
return module_redis_connection_error_message_printf_noncritical(
connection_context,
"ERR value for argument '%s' is not an integer or out of range",
guessed_argument->name);
return true;
}
break;

Expand All @@ -641,11 +650,10 @@ bool module_redis_command_process_argument_full(
*double_value = strtod(chunk_data, &double_value_end_ptr);

if (errno == ERANGE || double_value_end_ptr != chunk_data + chunk_length) {
module_redis_connection_error_message_printf_noncritical(
return module_redis_connection_error_message_printf_noncritical(
connection_context,
"ERR value for argument '%s' is not a double or out of range",
guessed_argument->name);
return true;
}
break;

Expand Down
17 changes: 13 additions & 4 deletions src/module/redis/module_redis_connection.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,8 @@ bool module_redis_connection_error_message_vprintf_internal(
bool override_previous_error,
char *error_message,
va_list args) {
bool return_res = false;

if (!override_previous_error) {
// Can't set an error message on top of an existing one, something is wrong if that happens
assert(connection_context->error.message == NULL);
Expand All @@ -130,7 +132,7 @@ bool module_redis_connection_error_message_vprintf_internal(

if (error_message_with_args == NULL) {
LOG_E(TAG, "Unable to allocate <%lu> bytes for the command error message", error_message_with_args_length + 1);
return false;
goto end;
}

if (vsnprintf(
Expand All @@ -140,14 +142,21 @@ bool module_redis_connection_error_message_vprintf_internal(
args) < 0) {
LOG_E(TAG, "Failed to format string <%s> with the arguments requested", error_message);
LOG_E_OS_ERROR(TAG);

return false;
goto end;
}

connection_context->error.message = error_message_with_args;

return_res = true;

end:

connection_context->command.skip = true;
if (!return_res) {
connection_context->terminate_connection = true;
}

return true;
return return_res;
}

bool module_redis_connection_error_message_printf_noncritical(
Expand Down
21 changes: 21 additions & 0 deletions tests/unit_tests/modules/redis/test-program-redis-commands.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -526,6 +526,27 @@ TEST_CASE("program.c-redis-commands", "[program-redis-commands]") {
REQUIRE(strncmp(buffer_recv, expected_error, strlen(expected_error)) == 0);
}

SECTION("Zero length - Key") {
REQUIRE(send_recv_resp_command_text(
client_fd,
std::vector<std::string>{"SET", "", "b_value"},
"-ERR the key 'key' has length '0', not allowed\r\n"));
}

SECTION("Zero length - Pattern") {
REQUIRE(send_recv_resp_command_text(
client_fd,
std::vector<std::string>{"SORT", "a_key", "BY", ""},
"-ERR the pattern 'by_pattern' has length '0', not allowed\r\n"));
}

SECTION("Zero length - Short String") {
REQUIRE(send_recv_resp_command_text(
client_fd,
std::vector<std::string>{"PING", ""},
"-ERR the short string 'message' has length '0', not allowed\r\n"));
}

SECTION("Max command arguments") {
off_t buffer_send_offset = 0;
char expected_error[256] = { 0 };
Expand Down

0 comments on commit 93246b1

Please sign in to comment.