Hi Willy,
On Tue, Feb 13, 2018 at 08:05:44PM +0100, Willy Tarreau wrote:
> Hi Olivier,
> Such type of construct tends to scare me (probably because I'm not reading
> the whole code). It means we're supposed to set an error by default unless
> we pass by a specific path. I fear that we'll get future issues (possibly
> as the result of further fixes for unrelated stuff).
>
> Also I'm really not fond of the fact that the out_error label isn't used
> anymore to set the error, but sometimes to set it, sometimes to clear it.
>
What about what's attached, instead ?
> Why instead not handle it almost similarly to the way it was before the
> patch that removed the code ? I'm seeing that in the past we used to only
> differenciate between read0 and error. But if we're failing on SYSCALL
> with ret previously set to zero, an EOF was observed, so that's akin to
> a read0 (this information is lost in the recent code since ret is
> overwritten). May I suggest that we change all this block to a switch/case
> instead ? It would more or less give something like this :
>
> if (ret > 0) {
> blah;
> }
> else switch (SSL_get_error(ctx, ret)) {
> case SSL_ERROR_WANT_WRITE: blah; break;
> case SSL_ERROR_WANT_READ: blah; break;
> case SSL_ERROR_ZERO_RETURN: goto read0;
> case SSL_ERROR_SYSCALL:
> if (!ret)
> goto read0;
> else if (errno && errno != EAGAIN)
> goto out_error;
> else /* valid cases, connection establishment, etc */
> goto leave;
> default:
> goto out_error;
> }
>
> It's just a suggestion, it can be done by storing the result of
> SSL_get_error() anywhere else, but definitely we need to make a clear
> distinction between all these cases and for now the distinction between
> the read0, completed connection and other errors is lost.
>
Because we can't rely on ret == 0 being meaningful. The manpage for
OpenSSL 1.1.1 states :
Old documentation indicated a difference between 0 and -1, and that -1 was
retryable. You should instead call SSL_get_error() to find out if it's
retryable.
And the switch would lead to more goto, as we need to break from outside the
loop :)
> I'm also seeing that the whole block could be rearranged so that ret <= 0
> is handled first. That would remove some gotos and would leave the main
> part after all error handling.
>
I'm not sure I get that part. I don't mind one way or another, but I don't
understand how it would remove gotos.
> BTW this makes me realize that your inverted condition above seems wrong
> (|| instead of &&).
>
Oops, that is true, those things are too complicated.
Regards,
Olivier
>From fca75937f4f2b1927f5c82c137cba292c82dac53 Mon Sep 17 00:00:00 2001
From: Olivier Houchard <[email protected]>
Date: Tue, 13 Feb 2018 15:17:23 +0100
Subject: [PATCH] MINOR/BUG: ssl: Don't always treat SSL_ERROR_SYSCALL as
unrecovarable.
SSL_read() might return <= 0, and SSL_get_erro() return SSL_ERROR_SYSCALL,
without meaning the connection is gone. Before flagging the conection
as in error, check the errno value.
This should be backported to 1.8.
---
src/ssl_sock.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index aee3cd965..b24db079d 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -5434,6 +5434,13 @@ static int ssl_sock_to_buf(struct connection *conn,
struct buffer *buf, int coun
break;
} else if (ret == SSL_ERROR_ZERO_RETURN)
goto read0;
+ /* For SSL_ERROR_SYSCALL, make sure the error is
+ * unrecoverable before flagging the connection as
+ * in error.
+ */
+ if (ret == SSL_ERROR_SYSCALL && (!errno || errno ==
+ EAGAIN))
+ goto clear_ssl_error;
/* otherwise it's a real error */
goto out_error;
}
@@ -5448,11 +5455,12 @@ static int ssl_sock_to_buf(struct connection *conn,
struct buffer *buf, int coun
conn_sock_read0(conn);
goto leave;
out_error:
+ conn->flags |= CO_FL_ERROR;
+clear_ssl_error:
/* Clear openssl global errors stack */
ssl_sock_dump_errors(conn);
ERR_clear_error();
- conn->flags |= CO_FL_ERROR;
goto leave;
}
--
2.14.3
