semihosting: fix memory leak and double free

Resolve two problems that occurred when working with semihosting service
through multiple connection cycles (connect-disconnect-reconnect):

1) Double free:
    When the same service handles multiple connections sequentially,
    the same memory gets freed repeatedly, because function
    'semihosting_service_connection_closed_handler()' incorrectly frees
    service->priv->name on every connection closure.

2) Memory leak:
    Function 'free_services()' misses service->priv->name cleanup for
    semihosting redirection. Memory remains allocated after service
    destruction.

The solution introduces a new 'dtor()' field in the service structure
that is called exactly once during free_service() execution.

To reproduce the issue, you can do the following:
    1. openocd -f target.cfg -c init -c 'arm semihosting enable' -c
    'arm semihosting_redirect tcp 4445'

    # in another terminal
    2. nc localhost 4445
    3. Ctr+C
    4. nc localhost 4445
    5. Ctr+C

Change-Id: I0dc8021cc3e21c5af619c71a1821a1afe9bffe78
Signed-off-by: Kulyatskaya Alexandra <a.kulyatskaya@syntacore.com>
Reviewed-on: https://review.openocd.org/c/openocd/+/9196
Tested-by: jenkins
Reviewed-by: Evgeniy Naydanov <eugnay@gmail.com>
Reviewed-by: Antonio Borneo <borneo.antonio@gmail.com>
This commit is contained in:
Kulyatskaya Alexandra
2025-10-06 23:19:56 +03:00
committed by Antonio Borneo
parent 55e9160509
commit 5ff384be08
3 changed files with 17 additions and 19 deletions
+8 -12
View File
@@ -161,6 +161,7 @@ static int remove_connection(struct service *service, struct connection *connect
/* find connection */ /* find connection */
while ((c = *p)) { while ((c = *p)) {
if (c->fd == connection->fd) { if (c->fd == connection->fd) {
if (service->connection_closed)
service->connection_closed(c); service->connection_closed(c);
if (service->type == CONNECTION_TCP) if (service->type == CONNECTION_TCP)
close_socket(c->fd); close_socket(c->fd);
@@ -190,8 +191,13 @@ static int remove_connection(struct service *service, struct connection *connect
static void free_service(struct service *c) static void free_service(struct service *c)
{ {
if (c->type == CONNECTION_PIPE && c->fd != -1)
close(c->fd);
if (c->service_dtor)
c->service_dtor(c);
free(c->name); free(c->name);
free(c->port); free(c->port);
free(c->priv);
free(c); free(c);
} }
@@ -214,6 +220,7 @@ int add_service(const struct service_driver *driver, const char *port,
c->input = driver->input_handler; c->input = driver->input_handler;
c->connection_closed = driver->connection_closed_handler; c->connection_closed = driver->connection_closed_handler;
c->keep_client_alive = driver->keep_client_alive_handler; c->keep_client_alive = driver->keep_client_alive_handler;
c->service_dtor = driver->service_dtor_handler;
c->priv = priv; c->priv = priv;
c->next = NULL; c->next = NULL;
long portnumber; long portnumber;
@@ -390,18 +397,7 @@ static int remove_services(void)
struct service *next = c->next; struct service *next = c->next;
remove_connections(c); remove_connections(c);
free_service(c);
free(c->name);
if (c->type == CONNECTION_PIPE) {
if (c->fd != -1)
close(c->fd);
}
free(c->port);
free(c->priv);
/* delete service */
free(c);
/* remember the last service for unlinking */ /* remember the last service for unlinking */
c = next; c = next;
} }
+2
View File
@@ -58,6 +58,7 @@ struct service_driver {
int (*new_connection_handler)(struct connection *connection); int (*new_connection_handler)(struct connection *connection);
/** callback to handle incoming data */ /** callback to handle incoming data */
int (*input_handler)(struct connection *connection); int (*input_handler)(struct connection *connection);
void (*service_dtor_handler)(struct service *service);
/** callback to tear down the connection */ /** callback to tear down the connection */
int (*connection_closed_handler)(struct connection *connection); int (*connection_closed_handler)(struct connection *connection);
/** called periodically to send keep-alive messages on the connection */ /** called periodically to send keep-alive messages on the connection */
@@ -76,6 +77,7 @@ struct service {
int (*new_connection_during_keep_alive)(struct connection *connection); int (*new_connection_during_keep_alive)(struct connection *connection);
int (*new_connection)(struct connection *connection); int (*new_connection)(struct connection *connection);
int (*input)(struct connection *connection); int (*input)(struct connection *connection);
void (*service_dtor)(struct service *service);
int (*connection_closed)(struct connection *connection); int (*connection_closed)(struct connection *connection);
void (*keep_client_alive)(struct connection *connection); void (*keep_client_alive)(struct connection *connection);
void *priv; void *priv;
+6 -6
View File
@@ -1800,13 +1800,12 @@ static int semihosting_service_input_handler(struct connection *connection)
return ERROR_OK; return ERROR_OK;
} }
static int semihosting_service_connection_closed_handler(struct connection *connection) static void semihosting_service_dtor_handler(struct service *service)
{ {
struct semihosting_tcp_service *service = connection->service->priv; struct semihosting_tcp_service *service_priv = service->priv;
if (service) if (service_priv)
free(service->name); free(service_priv->name);
return ERROR_OK;
} }
static void semihosting_tcp_close_cnx(struct semihosting *semihosting) static void semihosting_tcp_close_cnx(struct semihosting *semihosting)
@@ -1825,7 +1824,8 @@ static const struct service_driver semihosting_service_driver = {
.new_connection_during_keep_alive_handler = NULL, .new_connection_during_keep_alive_handler = NULL,
.new_connection_handler = semihosting_service_new_connection_handler, .new_connection_handler = semihosting_service_new_connection_handler,
.input_handler = semihosting_service_input_handler, .input_handler = semihosting_service_input_handler,
.connection_closed_handler = semihosting_service_connection_closed_handler, .service_dtor_handler = semihosting_service_dtor_handler,
.connection_closed_handler = NULL,
.keep_client_alive_handler = NULL, .keep_client_alive_handler = NULL,
}; };