Skip to content

PHP: fix memory leaks and resource management issues #174

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

Open
prateek-kumar-improving wants to merge 2 commits into
base: main
Choose a base branch
from
+90 −42
Open

PHP: fix memory leaks and resource management issues #174

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
826563e
php: fix memory leaks and resource management issues
prateek-kumar-improving Mar 26, 2026
80dfa38
Merge branch 'main' into php/fix-memory-issues
prateek-kumar-improving Mar 27, 2026
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
78 changes: 41 additions & 37 deletions command_response.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,42 @@ uint8_t* create_route_bytes_from_route(cluster_route_t* route, size_t* route_byt
}

/* Execute a command and handle common error checking */
/* Helper to free route-related allocations */
static void cleanup_route(uint8_t* route_bytes, cluster_route_t* route) {
if (route_bytes) {
efree(route_bytes);
}
if (route->type == ROUTE_TYPE_KEY && route->data.key_route.key_allocated) {
efree(route->data.key_route.key);
Copy link
Copy Markdown
Collaborator

@jduo jduo Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this set route->data.key_route.key to NULL so it doesn't get double-freed? Do we need to clean up anything else on route, or clean up route itself?

}
}

/* Validate parameters before FFI call, returns 1 if valid, 0 otherwise */
static int validate_command_params(const void* glide_client,
unsigned long arg_count,
const uintptr_t* args,
const unsigned long* args_len) {
if (!glide_client) {
VALKEY_LOG_ERROR("parameter_validation", "glide_client is NULL");
return 0;
}

if (arg_count > 0) {
if (!args) {
VALKEY_LOG_ERROR_FMT(
"parameter_validation", "args is NULL but arg_count is %lu", arg_count);
return 0;
}
if (!args_len) {
VALKEY_LOG_ERROR_FMT(
"parameter_validation", "args_len is NULL but arg_count is %lu", arg_count);
return 0;
}
}

return 1;
}

CommandResult* execute_command_with_route(const void* glide_client,
enum RequestType command_type,
unsigned long arg_count,
Expand All @@ -263,41 +299,16 @@ CommandResult* execute_command_with_route(const void* glide_client,
uint8_t* route_bytes = create_route_bytes_from_route(&route, &route_bytes_len);
if (!route_bytes) {
VALKEY_LOG_ERROR("route_processing", "Failed to create route bytes");
/* Free dynamically allocated key if needed before returning */
if (route.type == ROUTE_TYPE_KEY && route.data.key_route.key_allocated) {
efree(route.data.key_route.key);
}
cleanup_route(NULL, &route);
return NULL;
}

/* Validate all parameters before FFI call */
if (!glide_client) {
VALKEY_LOG_ERROR("parameter_validation", "glide_client is NULL");
if (!validate_command_params(glide_client, arg_count, args, args_len)) {
cleanup_route(route_bytes, &route);
return NULL;
}

if (arg_count > 0) {
if (!args) {
VALKEY_LOG_ERROR_FMT(
"parameter_validation", "args is NULL but arg_count is %lu", arg_count);
return NULL;
}
if (!args_len) {
VALKEY_LOG_ERROR_FMT(
"parameter_validation", "args_len is NULL but arg_count is %lu", arg_count);
return NULL;
}
}

if (route_bytes_len > 0) {
if (!route_bytes) {
VALKEY_LOG_ERROR_FMT("parameter_validation",
"route_bytes is NULL but route_bytes_len is %zu",
route_bytes_len);
return NULL;
}
}

/* Create OTEL span for tracing */
uint64_t span_ptr = valkey_glide_create_span(command_type);

Expand All @@ -316,15 +327,8 @@ CommandResult* execute_command_with_route(const void* glide_client,
/* Cleanup span */
valkey_glide_drop_span(span_ptr);

/* Free route bytes */
if (route_bytes) {
efree(route_bytes);
}

/* Free dynamically allocated key if needed */
if (route.type == ROUTE_TYPE_KEY && route.data.key_route.key_allocated) {
efree(route.data.key_route.key);
}
/* Free route allocations */
cleanup_route(route_bytes, &route);

/* Validate result before returning */
if (!result) {
Expand Down
34 changes: 33 additions & 1 deletion valkey_glide.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,6 +491,10 @@ int valkey_glide_build_client_config_base(valkey_glide_php_common_constructor_pa
}

_initialize_open_telemetry(params, is_cluster);
if (EG(exception)) {
valkey_glide_cleanup_client_config(config);
return FAILURE;
}
return SUCCESS;
}

Expand Down Expand Up @@ -568,6 +572,27 @@ ZEND_GET_MODULE(valkey_glide)
void free_valkey_glide_object(zend_object* object) {
valkey_glide_object* valkey_glide = VALKEY_GLIDE_PHP_GET_OBJECT(valkey_glide_object, object);

/* Free buffered batch commands if object is destroyed mid-batch */
Copy link
Copy Markdown

@jeremyprime jeremyprime Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the buffered commands also be cleaned up when close is called? This could be moved to a helper function and called here and in close.

This also applies to valkey_glide_cluster.c file.

if (valkey_glide->buffered_commands) {
size_t i, j;
for (i = 0; i < valkey_glide->command_count; i++) {
struct batch_command* cmd = &valkey_glide->buffered_commands[i];
Copy link
Copy Markdown
Collaborator

@jduo jduo Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to free cmd as well? Or is buffered_commands allocated as one big array of batch_commands?

if (cmd->args) {
for (j = 0; j < cmd->arg_count; j++) {
if (cmd->args[j]) {
efree(cmd->args[j]);
}
}
efree(cmd->args);
}
if (cmd->arg_lengths) {
efree(cmd->arg_lengths);
}
}
efree(valkey_glide->buffered_commands);
valkey_glide->buffered_commands = NULL;
}

/* Free the Valkey Glide client if it exists */
if (valkey_glide->glide_client) {
close_glide_client(valkey_glide->glide_client);
Expand Down Expand Up @@ -1004,7 +1029,14 @@ PHP_METHOD(ValkeyGlide, __destruct) {
/* {{{ proto boolean ValkeyGlide::close()
*/
PHP_METHOD(ValkeyGlide, close) {
/* TODO: Implement ValkeyGlide close */
valkey_glide_object* valkey_glide =
VALKEY_GLIDE_PHP_ZVAL_GET_OBJECT(valkey_glide_object, getThis());

if (valkey_glide->glide_client) {
close_glide_client(valkey_glide->glide_client);
valkey_glide->glide_client = NULL;
}

RETURN_TRUE;
}

Expand Down
8 changes: 8 additions & 0 deletions valkey_glide_cluster.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,14 @@ static zend_function_entry valkey_glide_cluster_methods[] = {

/* {{{ proto bool ValkeyGlideCluster::close() */
PHP_METHOD(ValkeyGlideCluster, close) {
valkey_glide_object* valkey_glide =
VALKEY_GLIDE_PHP_ZVAL_GET_OBJECT(valkey_glide_object, getThis());

if (valkey_glide->glide_client) {
close_glide_client(valkey_glide->glide_client);
valkey_glide->glide_client = NULL;
}

RETURN_TRUE;
}

Expand Down
12 changes: 8 additions & 4 deletions valkey_glide_str_commands.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,8 +454,10 @@ int execute_sort_command(zval* object, int argc, zval* return_value, zend_class_
/* Free the argument arrays */
efree(args);
efree(args_len);
efree(offset_str);
efree(count_str);
if (offset_str)
Copy link
Copy Markdown
Collaborator

@jduo jduo Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't efree(NULL) a no-op? Why add these if statements for just these two?

efree(offset_str);
if (count_str)
efree(count_str);


/* Process the result */
Expand Down Expand Up @@ -545,8 +547,10 @@ int execute_sort_ro_command(zval* object, int argc, zval* return_value, zend_cla
/* Free the argument arrays */
efree(args);
efree(args_len);
efree(offset_str);
efree(count_str);
if (offset_str)
efree(offset_str);
if (count_str)
efree(count_str);


int ret_val = 0;
Expand Down
Loading