You can not select more than 25 topics
Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.
87 lines
3.3 KiB
87 lines
3.3 KiB
From 06a0fb4102523a7b38b90983b11bb08d6d69aea1 Mon Sep 17 00:00:00 2001
|
|
From: Olivier Houchard <cognet@ci0.org>
|
|
Date: Sat, 27 Jan 2024 22:58:29 +0100
|
|
Subject: [PATCH] BUG/MAJOR: ssl_sock: Always clear retry flags in read/write
|
|
functions
|
|
MIME-Version: 1.0
|
|
Content-Type: text/plain; charset=UTF-8
|
|
Content-Transfer-Encoding: 8bit
|
|
|
|
It has been found that under some rare error circumstances,
|
|
SSL_do_handshake() could return with SSL_ERROR_WANT_READ without
|
|
even trying to call the read function, causing permanent wakeups
|
|
that prevent the process from sleeping.
|
|
|
|
It was established that this only happens if the retry flags are
|
|
not systematically cleared in both directions upon any I/O attempt,
|
|
but, given the lack of documentation on this topic, it is hard to
|
|
say if this rather strange behavior is expected or not, otherwise
|
|
why wouldn't the library always clear the flags by itself before
|
|
proceeding?
|
|
|
|
In addition, this only seems to affect OpenSSL 1.1.0 and above,
|
|
and does not affect wolfSSL nor aws-lc.
|
|
|
|
A bisection on haproxy showed that this issue was first triggered by
|
|
commit a8955d57ed ("MEDIUM: ssl: provide our own BIO."), which means
|
|
that OpenSSL's socket BIO does not have this problem. And this one
|
|
does always clear the flags before proceeding. So let's just proceed
|
|
the same way. It was verified that it properly fixes the problem,
|
|
does not affect other implementations, and doesn't cause any freeze
|
|
nor spurious wakeups either.
|
|
|
|
Many thanks to Valentín Gutiérrez for providing a network capture
|
|
showing the incident as well as a reproducer. This is GH issue #2403.
|
|
|
|
This patch needs to be backported to all versions that include the
|
|
commit above, i.e. as far as 2.0.
|
|
|
|
(cherry picked from commit 1ad19917213fac57ee37e581b0ef137e36c6309d)
|
|
Signed-off-by: Willy Tarreau <w@1wt.eu>
|
|
(cherry picked from commit bef2bc4cb6f4fa942d3659f25770cbfc137327b2)
|
|
Signed-off-by: Willy Tarreau <w@1wt.eu>
|
|
(cherry picked from commit a0b31bda308bccd987c15007a5384b602fcd7415)
|
|
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
|
|
(cherry picked from commit 571f5ebb056f533a8dac0d9948d0a3cecaeeda26)
|
|
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
|
|
(cherry picked from commit a067ce17f89b9b98ccc669521e0f859f5f62b3dd)
|
|
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
|
|
(cherry picked from commit d292e56c7e70eff215dd37b3e9e53c36499de867)
|
|
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
|
|
---
|
|
src/ssl_sock.c | 8 ++++----
|
|
1 file changed, 4 insertions(+), 4 deletions(-)
|
|
|
|
diff --git a/src/ssl_sock.c b/src/ssl_sock.c
|
|
index 9e7e7369d744..ef34fb61d1dd 100644
|
|
--- a/src/ssl_sock.c
|
|
+++ b/src/ssl_sock.c
|
|
@@ -158,11 +158,11 @@ static int ha_ssl_write(BIO *h, const char *buf, int num)
|
|
tmpbuf.data = num;
|
|
tmpbuf.head = 0;
|
|
ret = ctx->xprt->snd_buf(ctx->conn, ctx->xprt_ctx, &tmpbuf, num, 0);
|
|
+ BIO_clear_retry_flags(h);
|
|
if (ret == 0 && !(ctx->conn->flags & (CO_FL_ERROR | CO_FL_SOCK_WR_SH))) {
|
|
BIO_set_retry_write(h);
|
|
ret = -1;
|
|
- } else if (ret == 0)
|
|
- BIO_clear_retry_flags(h);
|
|
+ }
|
|
return ret;
|
|
}
|
|
|
|
@@ -190,11 +190,11 @@ static int ha_ssl_read(BIO *h, char *buf, int size)
|
|
tmpbuf.data = 0;
|
|
tmpbuf.head = 0;
|
|
ret = ctx->xprt->rcv_buf(ctx->conn, ctx->xprt_ctx, &tmpbuf, size, 0);
|
|
+ BIO_clear_retry_flags(h);
|
|
if (ret == 0 && !(ctx->conn->flags & (CO_FL_ERROR | CO_FL_SOCK_RD_SH))) {
|
|
BIO_set_retry_read(h);
|
|
ret = -1;
|
|
- } else if (ret == 0)
|
|
- BIO_clear_retry_flags(h);
|
|
+ }
|
|
|
|
return ret;
|
|
}
|