Skip to content

fix: harden hashtable value cleanup #81

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
AlexJuca merged 1 commit into from
Apr 27, 2026
Merged

fix: harden hashtable value cleanup #81

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ endif()
# Add test executable
add_executable(test_counter tests/test_counter.c src/counter.c)
add_executable(test_string_utils tests/test_string_utils.c src/string_utils.c)
add_executable(test_hashtable tests/test_hashtable.c src/core/hashtable.c)
add_executable(test_response_writer tests/test_response_writer.c src/client.c src/commands/common/command_registry.c)
add_executable(test_server_lifecycle tests/test_server_lifecycle.c src/server_lifecycle.c src/client.c src/core/list.c src/core/hashtable.c)
add_executable(test_server_config tests/test_server_config.c src/config.c src/numeric_parse.c)
Expand All @@ -72,20 +73,23 @@ target_compile_definitions(test_server_config PRIVATE SERVER)
target_compile_definitions(test_integration PRIVATE SERVER)
fkvs_configure_target(test_counter)
fkvs_configure_target(test_string_utils)
fkvs_configure_target(test_hashtable)
fkvs_configure_target(test_response_writer)
fkvs_configure_target(test_server_lifecycle)
fkvs_configure_target(test_server_config)
fkvs_configure_target(test_server_limits)
fkvs_configure_target(test_integration)
target_compile_options(test_counter PRIVATE -UNDEBUG)
target_compile_options(test_string_utils PRIVATE -UNDEBUG)
target_compile_options(test_hashtable PRIVATE -UNDEBUG)
target_compile_options(test_response_writer PRIVATE -UNDEBUG)
target_compile_options(test_server_lifecycle PRIVATE -UNDEBUG)
target_compile_options(test_server_config PRIVATE -UNDEBUG)
target_compile_options(test_server_limits PRIVATE -UNDEBUG)
target_compile_options(test_integration PRIVATE -UNDEBUG)
target_link_libraries(test_counter)
target_link_libraries(test_string_utils)
target_link_libraries(test_hashtable)
target_link_libraries(test_response_writer)
target_link_libraries(test_server_lifecycle)
target_link_libraries(test_server_config)
Expand All @@ -96,6 +100,7 @@ target_link_libraries(test_integration)
enable_testing()
add_test(NAME CounterTest COMMAND test_counter)
add_test(NAME StringUtilsTest COMMAND test_string_utils)
add_test(NAME HashtableTest COMMAND test_hashtable)
add_test(NAME ResponseWriterTest COMMAND test_response_writer)
add_test(NAME ServerLifecycleTest COMMAND test_server_lifecycle)
add_test(NAME ServerConfigTest COMMAND test_server_config)
Expand Down
77 changes: 34 additions & 43 deletions src/commands/server/server_command_handlers.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,6 @@
static hashtable_t *table = NULL;
static hashtable_t *expires = NULL;

static void free_value_copy(value_entry_t *value)
{
if (!value)
return;

free(value->ptr);
free(value);
}

static bool check_and_expire(const unsigned char *key, size_t key_len)
{
if (is_expired(expires, key, key_len)) {
Expand Down Expand Up @@ -198,8 +189,8 @@ void handle_set_command(client_t *client, unsigned char *buffer, size_t bytes_re
value_len, value_encoding)) {
send_error(client);
fprintf(stderr, "Unable to store SET value\n");
free_value_copy(old_value);
free_value_copy(old_expiry);
free_value_entry(old_value);
free_value_entry(old_expiry);
free(data);
return;
}
Expand All @@ -222,8 +213,8 @@ void handle_set_command(client_t *client, unsigned char *buffer, size_t bytes_re
} else {
delete_value(expires, &buffer[pos_key], key_len);
}
free_value_copy(old_value);
free_value_copy(old_expiry);
free_value_entry(old_value);
free_value_entry(old_expiry);
free(data);
return;
}
Expand All @@ -233,8 +224,8 @@ void handle_set_command(client_t *client, unsigned char *buffer, size_t bytes_re
}

send_reply(client, &buffer[pos_value], value_len);
free_value_copy(old_value);
free_value_copy(old_expiry);
free_value_entry(old_value);
free_value_entry(old_expiry);
free(data);
}

Expand Down Expand Up @@ -268,7 +259,7 @@ void handle_get_command(client_t *client, unsigned char *buffer, size_t bytes_re
if (!resp_buffer) {
send_error(client);
perror("malloc failed");
free_value_copy(value);
free_value_entry(value);
return;
}

Expand All @@ -277,7 +268,7 @@ void handle_get_command(client_t *client, unsigned char *buffer, size_t bytes_re
resp_buffer[value_len] = '\0';
send_reply(client, resp_buffer, value_len);
free(resp_buffer);
free_value_copy(value);
free_value_entry(value);
} else {
send_error(client);
}
Expand Down Expand Up @@ -339,7 +330,7 @@ void handle_incr_command(client_t *client, unsigned char *buffer,
if (value->encoding != VALUE_ENTRY_TYPE_INT) {
fprintf(stderr, "Stored value is not an integer.\n");
send_error(client);
free_value_copy(value);
free_value_entry(value);
return;
}

Expand All @@ -349,7 +340,7 @@ void handle_incr_command(client_t *client, unsigned char *buffer,
current == INT64_MAX) {
fprintf(stderr, "Stored integer is out of range.\n");
send_error(client);
free_value_copy(value);
free_value_entry(value);
return;
}
const int64_t sum = current + 1;
Expand All @@ -361,7 +352,7 @@ void handle_incr_command(client_t *client, unsigned char *buffer,
char *reply = int64_to_string(sum);
if (!reply) {
send_error(client);
free_value_copy(value);
free_value_entry(value);
return;
}
const size_t reply_len = strlen(reply);
Expand All @@ -371,13 +362,13 @@ void handle_incr_command(client_t *client, unsigned char *buffer,
fprintf(stderr, "Unable to set incremented value.\n");
send_error(client);
free(reply);
free_value_copy(value);
free_value_entry(value);
return;
}

send_reply(client, (const unsigned char *)reply, reply_len);
free(reply);
free_value_copy(value);
free_value_entry(value);
}

void handle_incr_by_command(client_t *client, unsigned char *buffer,
Expand Down Expand Up @@ -455,7 +446,7 @@ void handle_incr_by_command(client_t *client, unsigned char *buffer,
if (old_value->encoding != VALUE_ENTRY_TYPE_INT) {
fprintf(stderr, "Stored value is not an integer.\n");
send_error(client);
free_value_copy(old_value);
free_value_entry(old_value);
free(incr_str);
return;
}
Expand All @@ -470,7 +461,7 @@ void handle_incr_by_command(client_t *client, unsigned char *buffer,
(increment < 0 && current < INT64_MIN - increment)) {
fprintf(stderr, "Integer increment is out of range.\n");
send_error(client);
free_value_copy(old_value);
free_value_entry(old_value);
free(incr_str);
return;
}
Expand All @@ -483,7 +474,7 @@ void handle_incr_by_command(client_t *client, unsigned char *buffer,
char *result = int64_to_string(sum);
if (!result) {
send_error(client);
free_value_copy(old_value);
free_value_entry(old_value);
free(incr_str);
return;
}
Expand All @@ -493,14 +484,14 @@ void handle_incr_by_command(client_t *client, unsigned char *buffer,
result_len, VALUE_ENTRY_TYPE_INT)) {
fprintf(stderr, "Unable to set incremented value.\n");
send_error(client);
free_value_copy(old_value);
free_value_entry(old_value);
free(incr_str);
free(result);
return;
}

send_reply(client, (unsigned char *)result, result_len);
free_value_copy(old_value);
free_value_entry(old_value);
free(incr_str);
free(result);
}
Expand Down Expand Up @@ -581,7 +572,7 @@ void handle_decr_by_command(client_t *client, unsigned char *buffer,
if (old_value->encoding != VALUE_ENTRY_TYPE_INT) {
fprintf(stderr, "Stored value is not an integer.\n");
send_error(client);
free_value_copy(old_value);
free_value_entry(old_value);
free(decr_str);
return;
}
Expand All @@ -596,7 +587,7 @@ void handle_decr_by_command(client_t *client, unsigned char *buffer,
(decrement < 0 && current > INT64_MAX + decrement)) {
fprintf(stderr, "Integer decrement is out of range.\n");
send_error(client);
free_value_copy(old_value);
free_value_entry(old_value);
free(decr_str);
return;
}
Expand All @@ -609,7 +600,7 @@ void handle_decr_by_command(client_t *client, unsigned char *buffer,
char *result = int64_to_string(result_val);
if (!result) {
send_error(client);
free_value_copy(old_value);
free_value_entry(old_value);
free(decr_str);
return;
}
Expand All @@ -619,14 +610,14 @@ void handle_decr_by_command(client_t *client, unsigned char *buffer,
result_len, VALUE_ENTRY_TYPE_INT)) {
fprintf(stderr, "Unable to set decremented value.\n");
send_error(client);
free_value_copy(old_value);
free_value_entry(old_value);
free(decr_str);
free(result);
return;
}

send_reply(client, (unsigned char *)result, result_len);
free_value_copy(old_value);
free_value_entry(old_value);
free(decr_str);
free(result);
}
Expand Down Expand Up @@ -762,7 +753,7 @@ void handle_decr_command(client_t *client, unsigned char *buffer,
if (value->encoding != VALUE_ENTRY_TYPE_INT) {
fprintf(stderr, "Stored value is not an integer.\n");
send_error(client);
free_value_copy(value);
free_value_entry(value);
return;
}

Expand All @@ -772,15 +763,15 @@ void handle_decr_command(client_t *client, unsigned char *buffer,
current == INT64_MIN) {
fprintf(stderr, "Stored integer is out of range.\n");
send_error(client);
free_value_copy(value);
free_value_entry(value);
return;
}
const int64_t decrement = current - 1;

char *result_str = int64_to_string(decrement);
if (!result_str) {
send_error(client);
free_value_copy(value);
free_value_entry(value);
return;
}
const size_t result_length = strlen(result_str);
Expand All @@ -789,13 +780,13 @@ void handle_decr_command(client_t *client, unsigned char *buffer,
result_length, VALUE_ENTRY_TYPE_INT)) {
fprintf(stderr, "Unable to set decremented value.\n");
send_error(client);
free_value_copy(value);
free_value_entry(value);
free(result_str);
return;
}

send_reply(client, (unsigned char *)result_str, result_length);
free_value_copy(value);
free_value_entry(value);
free(result_str);
}

Expand Down Expand Up @@ -872,12 +863,12 @@ void handle_expire_command(client_t *client, unsigned char *buffer,
send_error(client);
return;
}
free(val->ptr);
free(val);
free_value_entry(val);

// Parse seconds string
char sec_buf[32];
size_t copy_len = ttl_str_len < sizeof(sec_buf) - 1 ? ttl_str_len : sizeof(sec_buf) - 1;
size_t copy_len =
ttl_str_len < sizeof(sec_buf) - 1 ? ttl_str_len : sizeof(sec_buf) - 1;
memcpy(sec_buf, &buffer[pos_ttl], copy_len);
sec_buf[copy_len] = '\0';

Expand Down Expand Up @@ -927,16 +918,16 @@ void handle_ttl_command(client_t *client, unsigned char *buffer,
size_t val_len;
bool key_exists = get_value(table, &buffer[5], key_len, &val, &val_len);
if (key_exists) {
free(val->ptr);
free(val);
free_value_entry(val);
}

int64_t ttl;
if (!key_exists) {
ttl = -2;
} else {
ttl = get_ttl(expires, &buffer[5], key_len);
// get_ttl returns -2 if not in expires table; for existing key with no TTL, return -1
// get_ttl returns -2 if not in expires table; for existing key with
// no TTL, return -1.
if (ttl == -2)
ttl = -1;
}
Expand Down
Loading
Loading