Skip to content

Commit

Permalink
[#10065] YSQL: Fix memory leaks in Ysql Connection Manager
Browse files Browse the repository at this point in the history
Summary:
This diff fixes memory leaks in Ysql Connection Manager which were caused due to following reasons:

  # In upstream odyssey io object of a client is never deallocated when client is free, so it is explicitly made to free.
  # In connection manager while reading the packet from socket during authentication (in yb_auth_passthrough.c), a machine msg object is created which is not set free after it's purpose, so explicitly making it in this diff.
  # Memory allocated to store the name of guc variable (in var.h) is never released, it is fixed by doing automatic allocation on stack so it is released automatically as it goes out of scope.

Jira: DB-8241

Test Plan:
  # All Ysql Connection Manager Tests are passing.

  # Ensured any long running app in yb-stress-test doesn't causes ysql conn mgr's memory to grow continuously while app is running, rather after some point of time it gets stabilized.

Reviewers: janand, nkumar, rbarigidad

Reviewed By: janand, rbarigidad

Subscribers: rbarigidad, nkumar, mihnea, yql

Differential Revision: https://phorge.dev.yugabyte.com/D33190
  • Loading branch information
Manav Kumar committed Apr 22, 2024
1 parent 687d25f commit 811e578
Show file tree
Hide file tree
Showing 6 changed files with 41 additions and 6 deletions.
4 changes: 4 additions & 0 deletions src/odyssey/sources/auth_query.c
Expand Up @@ -204,6 +204,10 @@ int od_auth_query(od_client_t *client, char *peer)
/* detach and unroute */
od_router_detach(router, auth_client);
od_router_unroute(router, auth_client);
if (auth_client->io.io) {
machine_close(auth_client->io.io);
machine_io_free(auth_client->io.io);
}
od_client_free(auth_client);
return OK_RESPONSE;
}
4 changes: 4 additions & 0 deletions src/odyssey/sources/client.h
Expand Up @@ -150,6 +150,10 @@ static inline void od_client_free(od_client_t *client)
if (client->prep_stmt_ids) {
od_hashmap_free(client->prep_stmt_ids);
}
if (client->vars.vars) {
free(client->vars.vars);
client->vars.vars = NULL;
}
free(client);
}

Expand Down
19 changes: 19 additions & 0 deletions src/odyssey/sources/frontend.c
Expand Up @@ -2106,6 +2106,8 @@ int yb_clean_shmem(od_client_t *client, od_server_t *server)
server, "Got a packet of type: %s",
kiwi_be_type_to_string(type));

machine_msg_free(msg);

if (type == KIWI_BE_READY_FOR_QUERY) {
return 0;
} else if (type == KIWI_BE_ERROR_RESPONSE) {
Expand Down Expand Up @@ -2473,6 +2475,11 @@ int yb_execute_on_control_connection(od_client_t *client,
control_conn_client, NULL,
"failed to route internal client for control connection: %s",
od_router_status_to_str(status));

if (control_conn_client->io.io) {
machine_close(control_conn_client->io.io);
machine_io_free(control_conn_client->io.io);
}
od_client_free(control_conn_client);
goto failed_to_acquire_control_connection;
}
Expand All @@ -2486,6 +2493,10 @@ int yb_execute_on_control_connection(od_client_t *client,
"failed to attach internal client for control connection to route: %s",
od_router_status_to_str(status));
od_router_unroute(router, control_conn_client);
if (control_conn_client->io.io) {
machine_close(control_conn_client->io.io);
machine_io_free(control_conn_client->io.io);
}
od_client_free(control_conn_client);
goto failed_to_acquire_control_connection;
}
Expand All @@ -2509,6 +2520,10 @@ int yb_execute_on_control_connection(od_client_t *client,
od_io_error(&server->io));
od_router_close(router, control_conn_client);
od_router_unroute(router, control_conn_client);
if (control_conn_client->io.io) {
machine_close(control_conn_client->io.io);
machine_io_free(control_conn_client->io.io);
}
od_client_free(control_conn_client);
goto failed_to_acquire_control_connection;
}
Expand All @@ -2519,6 +2534,10 @@ int yb_execute_on_control_connection(od_client_t *client,
/* detach and unroute */
od_router_detach(router, control_conn_client);
od_router_unroute(router, control_conn_client);
if (control_conn_client->io.io) {
machine_close(control_conn_client->io.io);
machine_io_free(control_conn_client->io.io);
}
od_client_free(control_conn_client);

if (rc == -1)
Expand Down
4 changes: 4 additions & 0 deletions src/odyssey/sources/storage.c
Expand Up @@ -362,6 +362,10 @@ void od_storage_watchdog_watch(void *arg)
od_debug(&instance->logger, "watchdog", watchdog_client,
NULL,
"deallocating obsolete storage watchdog");
if (watchdog_client->io.io) {
machine_close(watchdog_client->io.io);
machine_io_free(watchdog_client->io.io);
}
od_client_free(watchdog_client);
od_storage_watchdog_free(watchdog);
return;
Expand Down
8 changes: 6 additions & 2 deletions src/odyssey/sources/yb_auth_passthrough.c
Expand Up @@ -442,12 +442,13 @@ int yb_auth_frontend_passthrough(od_client_t *client, od_server_t *server)
"Unable to allocate the shared memory segment");
return -1;
}

machine_msg_free(msg);
return rc_auth;

case YB_KIWI_BE_FATAL_FOR_LOGICAL_CONNECTION:
yb_handle_fatalforlogicalconnection_pkt(client, server);
rc_auth = -1;
machine_msg_free(msg);
continue;

case KIWI_BE_NOTICE_RESPONSE:
Expand All @@ -460,9 +461,10 @@ int yb_auth_frontend_passthrough(od_client_t *client, od_server_t *server)
rc = od_write(&client->io, msg);
if (rc < 0)
rc_auth = -1;

machine_msg_free(msg);
continue;
}
machine_msg_free(msg);

break;

Expand All @@ -473,8 +475,10 @@ int yb_auth_frontend_passthrough(od_client_t *client, od_server_t *server)

case KIWI_BE_ERROR_RESPONSE:
/* Physical connection is broken, no need to wait for readyForQuery pkt */
machine_msg_free(msg);
server->offline = 1;
default:
machine_msg_free(msg);
return -1;
}
}
Expand Down
8 changes: 4 additions & 4 deletions src/odyssey/third_party/kiwi/kiwi/var.h
Expand Up @@ -8,6 +8,7 @@
*/

#define KIWI_MAX_VAR_SIZE 128
#define KIWI_MAX_NAME_SIZE 64

typedef struct kiwi_var kiwi_var_t;
typedef struct kiwi_vars kiwi_vars_t;
Expand Down Expand Up @@ -45,7 +46,7 @@ struct kiwi_var {
#ifdef YB_GUC_SUPPORT_VIA_SHMEM
kiwi_var_type_t type;
#endif
char *name;
char name[KIWI_MAX_NAME_SIZE];
int name_len;
char value[KIWI_MAX_VAR_SIZE];
int value_len;
Expand All @@ -67,9 +68,9 @@ static inline void kiwi_var_init(kiwi_var_t *var, char *name, int name_len)
var->name = name;
#else
if (name_len == 0)
var->name = NULL;
var->name[0] = '\0';
else
var->name = strdup(name);
memcpy(var->name, name, name_len);
#endif
var->name_len = name_len;
var->value_len = 0;
Expand Down Expand Up @@ -136,7 +137,6 @@ static inline void yb_kiwi_var_push(kiwi_vars_t *vars, char *name, int name_len,
vars->vars = realloc(vars->vars, vars->size * sizeof(kiwi_var_t));

kiwi_var_t *var = &vars->vars[vars->size - 1];
var->name = (char *)malloc(name_len * sizeof(char));
memcpy(var->name, name, name_len);
var->name_len = name_len;
memcpy(var->value, value, value_len);
Expand Down

0 comments on commit 811e578

Please sign in to comment.