From 412231eecd0af2d66937968ba9ef4d233725d859 Mon Sep 17 00:00:00 2001 From: Rob Shearman Date: Wed, 1 Jun 2022 19:41:08 +0100 Subject: [PATCH 1/2] whitelist: Use a watcher for control socket reading rather than blocking Performing a stream read_all call (which is a blocking read) from within the accept callback has the issue that if a whitelist client is still connected whilst a shutdown of the charon deamon is triggered then that shutdown won't complete gracefully due to the accept task never exiting. So fix shutting down gracefully by using the socket watcher rather than a blocking read upon connection accept. Fall back to a blocking read for partial messages to avoid the complexity associated (i.e. storing state) for incomplete reads, which shouldn't block and cause the original problem if the client only sends whole messages. --- .../plugins/whitelist/whitelist_control.c | 32 +++++++++++++++++-- 1 file changed, 29 insertions(+), 3 deletions(-) diff --git a/src/libcharon/plugins/whitelist/whitelist_control.c b/src/libcharon/plugins/whitelist/whitelist_control.c index e3a787ca7..c8dc7ef8d 100644 --- a/src/libcharon/plugins/whitelist/whitelist_control.c +++ b/src/libcharon/plugins/whitelist/whitelist_control.c @@ -95,13 +95,32 @@ static void list(private_whitelist_control_t *this, /** * Dispatch a received message */ -static bool on_accept(private_whitelist_control_t *this, stream_t *stream) +CALLBACK(on_read, bool, + private_whitelist_control_t *this, stream_t *stream) { identification_t *id; whitelist_msg_t msg; + ssize_t n; - while (stream->read_all(stream, &msg, sizeof(msg))) + while (TRUE) { + n = stream->read(stream, &msg, sizeof(msg), FALSE); + if (n <= 0) + { + if (errno == EWOULDBLOCK) + { + break; + } + return FALSE; + } + if (n < sizeof(msg)) + { + if (!stream->read_all(stream, ((char *)&msg) + n, sizeof(msg) - n)) + { + return FALSE; + } + } + msg.id[sizeof(msg.id) - 1] = 0; id = identification_create_from_string(msg.id); switch (ntohl(msg.type)) @@ -131,7 +150,14 @@ static bool on_accept(private_whitelist_control_t *this, stream_t *stream) id->destroy(id); } - return FALSE; + return TRUE; +} + +CALLBACK(on_accept, bool, + private_whitelist_control_t *this, stream_t *stream) +{ + stream->on_read(stream, on_read, this); + return TRUE; } METHOD(whitelist_control_t, destroy, void, From 85ebf6abd4412d241fd55bc076b60f0a87931540 Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Fri, 18 Jul 2025 12:07:45 +0200 Subject: [PATCH 2/2] whitelist: Add error handling to socket reads and fix a memory leak This now adds some state (basically a message buffer), but simplifies error handling as we don't have to handle two potential failure paths and could avoid some potential issues by still calling the blocking read_all(). It also fixes a memory leak when clients disconnect. --- .../plugins/whitelist/whitelist_control.c | 55 +++++++++++++------ 1 file changed, 37 insertions(+), 18 deletions(-) diff --git a/src/libcharon/plugins/whitelist/whitelist_control.c b/src/libcharon/plugins/whitelist/whitelist_control.c index c8dc7ef8d..4aec53aee 100644 --- a/src/libcharon/plugins/whitelist/whitelist_control.c +++ b/src/libcharon/plugins/whitelist/whitelist_control.c @@ -92,38 +92,51 @@ static void list(private_whitelist_control_t *this, stream->write_all(stream, &msg, sizeof(msg)); } +/** + * Information about a client connection. + */ +typedef struct { + private_whitelist_control_t *this; + whitelist_msg_t msg; + size_t read; +} whitelist_conn_t; + /** * Dispatch a received message */ CALLBACK(on_read, bool, - private_whitelist_control_t *this, stream_t *stream) + whitelist_conn_t *conn, stream_t *stream) { + private_whitelist_control_t *this = conn->this; identification_t *id; - whitelist_msg_t msg; - ssize_t n; + ssize_t len; while (TRUE) { - n = stream->read(stream, &msg, sizeof(msg), FALSE); - if (n <= 0) + while (conn->read < sizeof(conn->msg)) { - if (errno == EWOULDBLOCK) - { - break; - } - return FALSE; - } - if (n < sizeof(msg)) - { - if (!stream->read_all(stream, ((char *)&msg) + n, sizeof(msg) - n)) + len = stream->read(stream, (char*)&conn->msg + conn->read, + sizeof(conn->msg) - conn->read, FALSE); + if (len <= 0) { + if (errno == EWOULDBLOCK) + { + return TRUE; + } + if (len != 0) + { + DBG1(DBG_CFG, "whitelist socket error: %s", strerror(errno)); + } + stream->destroy(stream); + free(conn); return FALSE; } + conn->read += len; } - msg.id[sizeof(msg.id) - 1] = 0; - id = identification_create_from_string(msg.id); - switch (ntohl(msg.type)) + conn->msg.id[sizeof(conn->msg.id) - 1] = 0; + id = identification_create_from_string(conn->msg.id); + switch (ntohl(conn->msg.type)) { case WHITELIST_ADD: this->listener->add(this->listener, id); @@ -148,6 +161,7 @@ CALLBACK(on_read, bool, break; } id->destroy(id); + conn->read = 0; } return TRUE; @@ -156,7 +170,12 @@ CALLBACK(on_read, bool, CALLBACK(on_accept, bool, private_whitelist_control_t *this, stream_t *stream) { - stream->on_read(stream, on_read, this); + whitelist_conn_t *conn; + + INIT(conn, + .this = this, + ); + stream->on_read(stream, on_read, conn); return TRUE; }