Hi Olivier,

On Tue, Feb 13, 2018 at 06:07:36PM +0100, Olivier Houchard wrote:
> Hi Emmanuel,
> 
> On Tue, Feb 13, 2018 at 05:40:00PM +0100, Emmanuel Hocdet wrote:
> > Hi Olivier
> > 
> > > Le 13 févr. 2018 à 15:27, Olivier Houchard <ohouch...@haproxy.com> a 
> > > écrit :
> > > 
> > > Thanks a lot for the detailed analyze, and sorry for the late answer.
> > > You're probably right, SSL_ERROR_SYSCALL shouldn't be treated as an
> > > unrecoverable error.
> > > So, what you basically did was something equivalent to the patch attached 
> > > ?
> > > 
> > > Thanks a lot !
> > > 
> > > Olivier
> > > <0001-MINOR-BUG-ssl-Don-t-always-treat-SSL_ERROR_SYSCALL-a.patch>
> > 
> > 
> > 'ret' can be unassigned
> > errno must be check earlier.
> 
> I (mostly) disagree.
> There are 3 goto out_error, and on the only one that mattered, ret and
> errno should be fine.
> However, I rewrote the patch to make it clearer what the intent is.
> Do you like it better ?
> 
> Cheers,
> 
> Olivier

> >From 8de2963753a33ca948beebcdd70c5f46bd01e4cf 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_Raad() 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 | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/src/ssl_sock.c b/src/ssl_sock.c
> index aee3cd965..83e9b86e4 100644
> --- a/src/ssl_sock.c
> +++ b/src/ssl_sock.c
> @@ -5313,6 +5313,7 @@ reneg_ok:
>  static int ssl_sock_to_buf(struct connection *conn, struct buffer *buf, int 
> count)
>  {
>       int ret, done = 0;
> +     int set_error_flag = 1;
>       int try;
>  
>       conn_refresh_polling_flags(conn);
> @@ -5434,6 +5435,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))
> +                             set_error_flag = 0;
>                       /* otherwise it's a real error */
>                       goto out_error;
>               }
> @@ -5452,7 +5460,8 @@ static int ssl_sock_to_buf(struct connection *conn, 
> struct buffer *buf, int coun
>       ssl_sock_dump_errors(conn);
>       ERR_clear_error();
>  
> -     conn->flags |= CO_FL_ERROR;
> +     if (set_error_flag)
> +             conn->flags |= CO_FL_ERROR;
>       goto leave;
>  }

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.

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.

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.

BTW this makes me realize that your inverted condition above seems wrong
(|| instead of &&).

Cheers,
Willy

Reply via email to