From 03dce720baf91ff67eb82431f8d6ad24b4f4d657 Mon Sep 17 00:00:00 2001 From: Christoph Lipka Date: Mon, 7 Nov 2016 15:26:35 +0900 Subject: Event handling: Fix connection destroy bug It might happen that an event is part of the epoll event queue that belongs to a connection which was destroyed before the event is handled. Due to this, the event handling main loop might stop and the daemon exits. This misbehavior is fixed with this patch. Signed-off-by: Christoph Lipka --- src/daemon/dlt_daemon_connection.c | 4 ++++ src/daemon/dlt_daemon_event_handler.c | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/src/daemon/dlt_daemon_connection.c b/src/daemon/dlt_daemon_connection.c index 51f975d..b22f4b7 100644 --- a/src/daemon/dlt_daemon_connection.c +++ b/src/daemon/dlt_daemon_connection.c @@ -171,6 +171,7 @@ STATIC void dlt_connection_destroy_receiver(DltConnection *con) default: (void) dlt_receiver_free(con->receiver); free(con->receiver); + con->receiver = NULL; break; } } @@ -320,6 +321,9 @@ void dlt_connection_destroy(DltConnection *to_destroy) { close(to_destroy->receiver->fd); dlt_connection_destroy_receiver(to_destroy); + /* connection pointer might be in epoll queue and used even after destroying + * it. To make sure it is not used anymore, connection type is invalidated */ + to_destroy->type = DLT_CONNECTION_TYPE_MAX; free(to_destroy); } diff --git a/src/daemon/dlt_daemon_event_handler.c b/src/daemon/dlt_daemon_event_handler.c index 9ee3bc1..cfe61a4 100644 --- a/src/daemon/dlt_daemon_event_handler.c +++ b/src/daemon/dlt_daemon_event_handler.c @@ -137,6 +137,10 @@ int dlt_daemon_handle_event(DltEventHandler *pEvent, type = con->type; fd = con->receiver->fd; } + else /* connection might have been destroyed in the meanwhile */ + { + continue; + } /* First of all handle epoll error events * We only expect EPOLLIN or EPOLLOUT -- cgit v1.2.1 From 0ce6e68d8835b13aa6be52ffdf2d81e1170a3834 Mon Sep 17 00:00:00 2001 From: Frederic Berat Date: Tue, 17 Jan 2017 11:37:27 +0100 Subject: dlt-daemon: Fix use after free potential issue In dlt_daemon_send_all_multiple, if the connection was broken, we closed it before getting the next available connection. This must be avoided by having a temporary next pointer. The same kind of problem is valid for pointers coming from the epoll interface. The kernel can provide back connection pointer that are not valid any longer. Therefore, we need to use an ID instead of the pointer value to retrieve the connections. Signed-off-by: Frederic Berat Signed-off-by: Christoph Lipka --- src/daemon/dlt_daemon_client.c | 4 +++- src/daemon/dlt_daemon_connection.c | 11 +++++++++++ src/daemon/dlt_daemon_connection_types.h | 3 +++ src/daemon/dlt_daemon_event_handler.c | 31 +++++++++++++++++++++++++++++-- src/daemon/dlt_daemon_event_handler.h | 2 ++ 5 files changed, 48 insertions(+), 3 deletions(-) diff --git a/src/daemon/dlt_daemon_client.c b/src/daemon/dlt_daemon_client.c index e605bd5..84ffd5d 100644 --- a/src/daemon/dlt_daemon_client.c +++ b/src/daemon/dlt_daemon_client.c @@ -133,6 +133,8 @@ static int dlt_daemon_client_send_all_multiple(DltDaemon *daemon, { int ret = 0; DLT_DAEMON_SEM_LOCK(); + DltConnection *next = dlt_connection_get_next(temp->next, type_mask); + ret = dlt_connection_send_multiple(temp, data1, size1, @@ -166,7 +168,7 @@ static int dlt_daemon_client_send_all_multiple(DltDaemon *daemon, sent = 1; } - temp = dlt_connection_get_next(temp->next, type_mask); + temp = next; } /* for */ return sent; diff --git a/src/daemon/dlt_daemon_connection.c b/src/daemon/dlt_daemon_connection.c index b22f4b7..8d2aa67 100644 --- a/src/daemon/dlt_daemon_connection.c +++ b/src/daemon/dlt_daemon_connection.c @@ -47,6 +47,8 @@ #include "dlt_common.h" #include "dlt_gateway.h" +static DltConnectionId connectionId; + /** @brief Generic sending function. * * We manage different type of connection which have similar send/write @@ -319,6 +321,7 @@ void *dlt_connection_get_callback(DltConnection *con) */ void dlt_connection_destroy(DltConnection *to_destroy) { + to_destroy->id = 0; close(to_destroy->receiver->fd); dlt_connection_destroy_receiver(to_destroy); /* connection pointer might be in epoll queue and used even after destroying @@ -388,6 +391,14 @@ int dlt_connection_create(DltDaemonLocal *daemon_local, return -1; } + /* We are single threaded no need for protection. */ + temp->id = connectionId++; + if (!temp->id) + { + /* Skipping 0 */ + temp->id = connectionId++; + } + temp->type = type; temp->status = ACTIVE; diff --git a/src/daemon/dlt_daemon_connection_types.h b/src/daemon/dlt_daemon_connection_types.h index 1e1186b..8774378 100644 --- a/src/daemon/dlt_daemon_connection_types.h +++ b/src/daemon/dlt_daemon_connection_types.h @@ -67,10 +67,13 @@ typedef enum { #define DLT_CON_MASK_GATEWAY_TIMER (1 << DLT_CONNECTION_GATEWAY_TIMER) #define DLT_CON_MASK_ALL (0xffff) +typedef unsigned int DltConnectionId; + /* TODO: squash the DltReceiver structure in there * and remove any other duplicates of FDs */ typedef struct DltConnection { + DltConnectionId id; DltReceiver *receiver; /**< Receiver structure for this connection */ DltConnectionType type; /**< Represents what type of handle is this (like FIFO, serial, client, server) */ DltConnectionStatus status; /**< Status of connection */ diff --git a/src/daemon/dlt_daemon_event_handler.c b/src/daemon/dlt_daemon_event_handler.c index cfe61a4..9051f07 100644 --- a/src/daemon/dlt_daemon_event_handler.c +++ b/src/daemon/dlt_daemon_event_handler.c @@ -128,7 +128,9 @@ int dlt_daemon_handle_event(DltEventHandler *pEvent, for (i = 0 ; i < nfds ; i++) { struct epoll_event *ev = &pEvent->events[i]; - DltConnection *con = (DltConnection *)ev->data.ptr; + DltConnectionId id = (DltConnectionId)ev->data.ptr; + DltConnection *con = dlt_event_handler_find_connection_by_id(pEvent, + id); int fd = 0; DltConnectionType type = DLT_CONNECTION_TYPE_MAX; @@ -210,6 +212,31 @@ DltConnection *dlt_event_handler_find_connection(DltEventHandler *ev, return temp; } +/** @brief Find connection with a specific \a id in the connection list. + * + * There can be only one event per \a fd. We can then find a specific connection + * based on this \a fd. That allows to check if a specific \a fd has already been + * registered. + * + * @param ev The event handler structure where the list of connection is. + * @param id The identifier of the connection to be found. + * + * @return The found connection pointer, NULL otherwise. + */ +DltConnection *dlt_event_handler_find_connection_by_id(DltEventHandler *ev, + DltConnectionId id) +{ + + DltConnection *temp = ev->connections; + + while ((temp != NULL) && (temp->id != id)) + { + temp = temp->next; + } + + return temp; +} + /** @brief Remove a connection from the list and destroy it. * * This function will first look for the connection in the event handler list, @@ -351,7 +378,7 @@ int dlt_connection_check_activate(DltEventHandler *evhdl, { struct epoll_event ev; /* Content will be copied by the kernel */ ev.events = con->ev_mask; - ev.data.ptr = (void *)con; + ev.data.ptr = (void *)con->id; snprintf(local_str, DLT_DAEMON_TEXTBUFSIZE, diff --git a/src/daemon/dlt_daemon_event_handler.h b/src/daemon/dlt_daemon_event_handler.h index ea88da9..dc91b89 100644 --- a/src/daemon/dlt_daemon_event_handler.h +++ b/src/daemon/dlt_daemon_event_handler.h @@ -39,6 +39,8 @@ int dlt_daemon_prepare_event_handling(DltEventHandler *); int dlt_daemon_handle_event(DltEventHandler *, DltDaemon *, DltDaemonLocal *); +DltConnection *dlt_event_handler_find_connection_by_id(DltEventHandler *, + DltConnectionId); DltConnection *dlt_event_handler_find_connection(DltEventHandler *, int); -- cgit v1.2.1