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 <ohouch...@haproxy.com>
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

Reply via email to