From 061b7a6fee0c96d53d47553cd05f96791df5e677 Mon Sep 17 00:00:00 2001 From: Pierre Tardy Date: Tue, 12 May 2026 14:45:10 +0200 Subject: [PATCH] fix: proper fd lifecycle management to eliminate fdsan crash Root cause: The DLT_CONNECTION_GATEWAY connections share a receiver pointer aliased into DltGatewayConnection.client.receiver. When dlt_connection_destroy() closed the fd, the gateway's client.sock retained the stale fd number. If the kernel reused that fd number (e.g., for a FILE*), subsequent gateway send() calls would trigger Android fdsan abort. Changes: - dlt_connection_destroy: Do NOT close fd for GATEWAY connections (they don't own it). Only detach the receiver pointer. - dlt_gateway_close_connection: New function that properly closes client.sock AND invalidates client.receiver.fd at all disconnect points. - Deferred destruction: Connections are now marked PENDING_DESTROY instead of being freed immediately. A sweep phase after the event loop iteration safely destroys them, preventing use-after-free when callbacks trigger their own connection's removal. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Pierre Tardy --- src/daemon/dlt_daemon_connection.c | 29 +++++++++- src/daemon/dlt_daemon_connection_types.h | 3 +- src/daemon/dlt_daemon_event_handler.c | 73 +++++++++++++++++++++--- src/gateway/dlt_gateway.c | 25 ++++++++ src/gateway/dlt_gateway.h | 10 ++++ 5 files changed, 128 insertions(+), 12 deletions(-) diff --git a/src/daemon/dlt_daemon_connection.c b/src/daemon/dlt_daemon_connection.c index 456623cdb..e9c2f9385 100644 --- a/src/daemon/dlt_daemon_connection.c +++ b/src/daemon/dlt_daemon_connection.c @@ -345,13 +345,38 @@ void *dlt_connection_get_callback(DltConnection *con) * Ownership of the connection is given during the registration to * the DltEventHandler. * + * For DLT_CONNECTION_GATEWAY connections, the fd is owned by the gateway's + * DltClient structure and must NOT be closed here. The gateway module is + * responsible for closing its own socket via dlt_client_cleanup(). + * * @param to_destroy Connection to be destroyed. */ void dlt_connection_destroy(DltConnection *to_destroy) { + if (to_destroy == NULL) + return; + to_destroy->id = 0; - close(to_destroy->receiver->fd); - dlt_connection_destroy_receiver(to_destroy); + + if (to_destroy->receiver != NULL) { + int fd = to_destroy->receiver->fd; + + if (to_destroy->type == DLT_CONNECTION_GATEWAY) { + /* Gateway connections: the fd is owned by DltGatewayConnection.client. + * Do NOT close it here — the gateway manages its own socket lifecycle. + * Just detach our reference. */ + to_destroy->receiver = NULL; + } else { + /* All other connection types: we own the fd, close it. */ + if (fd >= 0) { + close(fd); + to_destroy->receiver->fd = -1; + } + + dlt_connection_destroy_receiver(to_destroy); + } + } + free(to_destroy); } diff --git a/src/daemon/dlt_daemon_connection_types.h b/src/daemon/dlt_daemon_connection_types.h index 5f197ca6b..51767f4f4 100644 --- a/src/daemon/dlt_daemon_connection_types.h +++ b/src/daemon/dlt_daemon_connection_types.h @@ -34,7 +34,8 @@ typedef enum { INACTIVE, /* Connection is inactive, excluded from poll handling */ ACTIVE, /* Connection is actively handled by poll */ DEACTIVATE,/* Request for deactivation of the connection */ - ACTIVATE /* Request for activation of the connection */ + ACTIVATE, /* Request for activation of the connection */ + PENDING_DESTROY /* Marked for deferred destruction after event loop iteration */ } DltConnectionStatus; typedef enum { diff --git a/src/daemon/dlt_daemon_event_handler.c b/src/daemon/dlt_daemon_event_handler.c index 57763585c..d7e1337be 100644 --- a/src/daemon/dlt_daemon_event_handler.c +++ b/src/daemon/dlt_daemon_event_handler.c @@ -55,6 +55,9 @@ #define DLT_EV_MASK_REJECTED (POLLERR | POLLNVAL) +/* Forward declarations for internal functions */ +DLT_STATIC int dlt_daemon_remove_connection(DltEventHandler *ev, + DltConnection *to_remove); /** @brief Initialize a pollfd structure * * That ensures that no event will be mis-watched. @@ -169,12 +172,51 @@ static void dlt_event_handler_disable_fd(DltEventHandler *ev, int fd) } } +/** @brief Sweep connections marked PENDING_DESTROY. + * + * Called after the event loop iteration completes to safely destroy + * connections that were marked during callback processing. This prevents + * use-after-free when a callback triggers its own connection's removal. + * + * @param ev The event handler structure. + * @param daemon_local Structure containing needed information. + */ +static void dlt_event_handler_sweep_pending_destroy(DltEventHandler *ev, + DltDaemonLocal *daemon_local) +{ + DltConnection *cur = NULL; + DltConnection *next = NULL; + + (void)daemon_local; + + if (ev == NULL) + return; + + cur = ev->connections; + + while (cur != NULL) { + next = cur->next; + + if (cur->status == PENDING_DESTROY) { + /* Remove from linked list and destroy. + * Counter was already decremented in unregister_connection(). */ + dlt_daemon_remove_connection(ev, cur); + } + + cur = next; + } +} + /** @brief Catch and process incoming events. * * This function waits for events on all connections. Once an event raise, * the callback for the specific connection is called, or the connection is * destroyed if a hangup occurs. * + * Connections that need to be destroyed during event processing are marked + * PENDING_DESTROY and swept after the iteration completes. This prevents + * use-after-free when a callback triggers its own connection's destruction. + * * @param daemon Structure to be passed to the callback. * @param daemon_local Structure containing needed information. * @param pEvent Event handler structure. @@ -217,7 +259,7 @@ int dlt_daemon_handle_event(DltEventHandler *pEvent, con = dlt_event_handler_find_connection(pEvent, pEvent->pfd[i].fd); - if (con && con->receiver) { + if (con && con->receiver && (con->status != PENDING_DESTROY)) { type = con->type; fd = con->receiver->fd; } @@ -276,6 +318,9 @@ int dlt_daemon_handle_event(DltEventHandler *pEvent, #endif } + /* Sweep connections marked for deferred destruction */ + dlt_event_handler_sweep_pending_destroy(pEvent, daemon_local); + return 0; } @@ -439,6 +484,9 @@ int dlt_connection_check_activate(DltEventHandler *evhdl, con->status = ACTIVE; } + break; + case PENDING_DESTROY: + /* Already marked for destruction — deactivation is a no-op */ break; default: dlt_vlog(LOG_ERR, "Unknown connection status: %u\n", con->status); @@ -491,13 +539,12 @@ int dlt_event_handler_register_connection(DltEventHandler *evhdl, ACTIVATE); } -/** @brief Unregisters a connection from the event handler and destroys it. +/** @brief Unregisters a connection from the event handler. * - * We first look for the connection to be unregistered, delete the event - * corresponding and then destroy the connection. - * If the connection is of type DLT_CONNECTION_CLIENT_MSG_TCP, we decrease - * the daemon_local->client_connections counter. TODO: Move this counter inside - * the event handler structure. + * The connection is deactivated from poll and marked PENDING_DESTROY for + * deferred cleanup. Actual destruction happens in the sweep phase after + * the event loop iteration completes. This is safe to call from within + * event callbacks without causing use-after-free. * * @param evhdl The event handler structure where the connection list is. * @param daemon_local Structure containing needed information. @@ -522,6 +569,11 @@ int dlt_event_handler_unregister_connection(DltEventHandler *evhdl, return -1; } + /* Already pending destruction — nothing to do */ + if (temp->status == PENDING_DESTROY) + return 0; + + /* Decrement client counter immediately (callers may check it right after) */ if ((temp->type == DLT_CONNECTION_CLIENT_MSG_TCP) || (temp->type == DLT_CONNECTION_CLIENT_MSG_SERIAL)) { daemon_local->client_connections--; @@ -532,11 +584,14 @@ int dlt_event_handler_unregister_connection(DltEventHandler *evhdl, } } + /* Remove from poll immediately to stop receiving events */ if (dlt_connection_check_activate(evhdl, temp, DEACTIVATE) < 0) dlt_log(LOG_ERR, "Unable to unregister event.\n"); - /* Cannot fail as far as dlt_daemon_find_connection succeed */ - return dlt_daemon_remove_connection(evhdl, temp); + /* Mark for deferred destruction — the sweep phase handles the rest */ + temp->status = PENDING_DESTROY; + + return 0; } diff --git a/src/gateway/dlt_gateway.c b/src/gateway/dlt_gateway.c index d3aba52bb..30c94e8ee 100644 --- a/src/gateway/dlt_gateway.c +++ b/src/gateway/dlt_gateway.c @@ -29,6 +29,7 @@ #include #include #include +#include #include #include #include @@ -1068,6 +1069,21 @@ DltReceiver *dlt_gateway_get_connection_receiver(DltGateway *gateway, int fd) return NULL; } +void dlt_gateway_close_connection(DltGatewayConnection *con) +{ + if (con == NULL) + return; + + if (con->client.sock >= 0) { + close(con->client.sock); + con->client.sock = -1; + } + + /* Ensure the receiver's fd copy is also invalidated. + * These are normally the same value, but be explicit. */ + con->client.receiver.fd = -1; +} + /** * Parse GET_LOG_INFO * @@ -1345,6 +1361,9 @@ DltReturnValue dlt_gateway_process_passive_node_messages(DltDaemon *daemon, daemon_local, receiver->fd) != 0) dlt_log(LOG_ERR, "Remove passive node Connection failed\n"); + + /* Close the gateway's own socket and invalidate both fd copies */ + dlt_gateway_close_connection(con); } return DLT_RETURN_OK; @@ -1448,6 +1467,9 @@ DltReturnValue dlt_gateway_process_passive_node_messages(DltDaemon *daemon, != 0) dlt_log(LOG_ERR, "Remove passive node Connection failed\n"); + /* Close the gateway's own socket and invalidate both fd copies */ + dlt_gateway_close_connection(con); + dlt_log(LOG_WARNING, "Disconnect from passive node due to invalid ECUid\n"); @@ -1750,6 +1772,9 @@ DltReturnValue dlt_gateway_process_on_demand_request(DltGateway *gateway, con->client.sock) != 0) dlt_log(LOG_ERR, "Remove passive node event handler connection failed\n"); + + /* Close the gateway's own socket and invalidate both fd copies */ + dlt_gateway_close_connection(con); } else { dlt_log(LOG_ERR, "Unknown command (connection_status)\n"); diff --git a/src/gateway/dlt_gateway.h b/src/gateway/dlt_gateway.h index 8a7c7c6f8..9b2ad7a0e 100644 --- a/src/gateway/dlt_gateway.h +++ b/src/gateway/dlt_gateway.h @@ -232,5 +232,15 @@ DltGatewayConnection *dlt_gateway_get_connection(DltGateway *g, DltGatewayConnection *dlt_gateway_get_connection_v2(DltGateway *g, char *ecu, int verbose); +/** + * Close the gateway connection's socket and invalidate all fd references. + * + * This properly shuts down the socket owned by the DltClient embedded in the + * gateway connection, and ensures both client.sock and client.receiver.fd are + * set to -1 to prevent any stale fd usage. + * + * @param con DltGatewayConnection to close + */ +void dlt_gateway_close_connection(DltGatewayConnection *con); #endif