From: Emeric Brun <ebrun@haproxy.com>
Date: Fri, 1 Jul 2022 15:36:50 +0000 (+0200)
Subject: BUG/MEDIUM: ssl/fd: unexpected fd close using async engine
X-Git-Tag: v2.7-dev2~161
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=7d392a592d5341df43086a2a48f34658b0ba432f;p=thirdparty%2Fhaproxy.git

BUG/MEDIUM: ssl/fd: unexpected fd close using async engine

Before 2.3, after an async crypto processing or on session close, the engine
async file's descriptors were removed from the fdtab but not closed because
it is the engine which has created the file descriptor, and it is responsible
for closing it. In 2.3 the fd_remove() call was replaced by fd_stop_both()
which stops the polling but does not remove the fd from the fdtab and the
fd remains indefinitively in the fdtab.

A simple replacement by fd_delete() is not a valid fix because fd_delete()
removes the fd from the fdtab but also closes the fd. And the fd will be
closed twice: by the haproxy's core and by the engine itself.

Instead, let's set FD_DISOWN on the FD before calling fd_delete() which will
take care of not closing it.

This patch must be backported on branches >= 2.3, and it relies on this
previous patch:

   MINOR: fd: add a new FD_DISOWN flag to prevent from closing a deleted FD

As mentioned in the patch above, a different flag will be needed in 2.3.
---

diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index e955a46cda..3c89aaa8d2 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -776,8 +776,14 @@ void ssl_async_fd_free(int fd)
 	}
 
 	SSL_get_all_async_fds(ssl, all_fd, &num_all_fds);
-	for (i=0 ; i < num_all_fds ; i++)
-		fd_stop_both(all_fd[i]);
+	for (i=0 ; i < num_all_fds ; i++) {
+		/* We want to remove the fd from the fdtab
+		 * but we flag it to disown because the
+		 * close is performed by the engine itself
+		 */
+		fdtab[all_fd[i]].state |= FD_DISOWN;
+		fd_delete(all_fd[i]);
+	}
 
 	/* Now we can safely call SSL_free, no more pending job in engines */
 	SSL_free(ssl);
@@ -807,8 +813,14 @@ static inline void ssl_async_process_fds(struct ssl_sock_ctx *ctx)
 	SSL_get_changed_async_fds(ssl, add_fd, &num_add_fds, del_fd, &num_del_fds);
 
 	/* We remove unused fds from the fdtab */
-	for (i=0 ; i < num_del_fds ; i++)
-		fd_stop_both(del_fd[i]);
+	for (i=0 ; i < num_del_fds ; i++) {
+		/* We want to remove the fd from the fdtab
+		 * but we flag it to disown because the
+		 * close is performed by the engine itself
+		 */
+		fdtab[del_fd[i]].state |= FD_DISOWN;
+		fd_delete(del_fd[i]);
+	}
 
 	/* We add new fds to the fdtab */
 	for (i=0 ; i < num_add_fds ; i++) {
@@ -6796,8 +6808,14 @@ void ssl_sock_close(struct connection *conn, void *xprt_ctx) {
 			 * because the fd is  owned by the engine.
 			 * the engine is responsible to close
 			 */
-			for (i=0 ; i < num_all_fds ; i++)
-				fd_stop_both(all_fd[i]);
+			for (i=0 ; i < num_all_fds ; i++) {
+				/* We want to remove the fd from the fdtab
+				 * but we flag it to disown because the
+				 * close is performed by the engine itself
+				 */
+				fdtab[all_fd[i]].state |= FD_DISOWN;
+				fd_delete(all_fd[i]);
+			}
 		}
 #endif
 		SSL_free(ctx->ssl);