[PATCH] QUIC openssl compat mode error handling

[Vladimir Homutov]

On Fri, Sep 22, 2023 at 07:30:50PM +0400, Roman Arutyunyan wrote:
> Hi Vladimir,
>
> On Fri, Sep 22, 2023 at 03:44:08PM +0300, Vladimir Homutov via nginx-devel wrote:
> > # HG changeset patch
> > # User Vladimir Khomutov <vl at inspert.ru>
> > # Date 1695386443 -10800
> > #      Fri Sep 22 15:40:43 2023 +0300
> > # Node ID 974ba23e68909ba708616410aa77074213d4d1e5
> > # Parent  5741eddf82e826766cd0f5ec7c6fe383145ca581
> > QUIC: handle add_handhshake_data() callback errors in compat.
> >
> > The error may be triggered by incorrect transport parameter sent by client.
> > The expected behaviour in this case is to close connection complaining
> > about incorrect parameter.  Currently the connection just times out.
> >
> > diff --git a/src/event/quic/ngx_event_quic_openssl_compat.c b/src/event/quic/ngx_event_quic_openssl_compat.c
> > --- a/src/event/quic/ngx_event_quic_openssl_compat.c
> > +++ b/src/event/quic/ngx_event_quic_openssl_compat.c
> > @@ -408,7 +408,10 @@ ngx_quic_compat_message_callback(int wri
> >                         "quic compat tx %s len:%uz ",
> >                         ngx_quic_level_name(level), len);
> >
> > -        (void) com->method->add_handshake_data(ssl, level, buf, len);
> > +        if (com->method->add_handshake_data(ssl, level, buf, len) != 1) {
> > +            ngx_post_event(&qc->close, &ngx_posted_events);
> > +            return;
> > +        }
> >
> >          break;
>
> Thanks for the patch.  Indeed, it's a simple way to handle errors in callbacks.
> I'd also handle the error in send_alert(), even though we don't generate any
> errors in it now.

Yes, although I was not sure if we need to close connection if we failed
to send alert (but probably if we are sending it, everything is already
bad enough). In either case, handling both cases similarly looks
as a way to go.

>
> --
> Roman Arutyunyan

> # HG changeset patch
> # User Vladimir Khomutov <vl at inspert.ru>
> # Date 1695396237 -14400
> #      Fri Sep 22 19:23:57 2023 +0400
> # Node ID 3db945fda515014d220151046d02f3960bcfca0a
> # Parent  32b5aaebcca51854de6e1f8a40798edb13662edb
> QUIC: handle callback errors in compat.
>
> The error may be triggered in add_handhshake_data() by incorrect transport
> parameter sent by client.  The expected behaviour in this case is to close
> connection complaining about incorrect parameter.  Currently the connection
> just times out.
>
> diff --git a/src/event/quic/ngx_event_quic_openssl_compat.c b/src/event/quic/ngx_event_quic_openssl_compat.c
> --- a/src/event/quic/ngx_event_quic_openssl_compat.c
> +++ b/src/event/quic/ngx_event_quic_openssl_compat.c
> @@ -408,7 +408,9 @@ ngx_quic_compat_message_callback(int wri
>                         "quic compat tx %s len:%uz ",
>                         ngx_quic_level_name(level), len);
>
> -        (void) com->method->add_handshake_data(ssl, level, buf, len);
> +        if (com->method->add_handshake_data(ssl, level, buf, len) != 1) {
> +            goto failed;
> +        }
>
>          break;
>
> @@ -420,11 +422,19 @@ ngx_quic_compat_message_callback(int wri
>                             "quic compat %s alert:%ui len:%uz ",
>                             ngx_quic_level_name(level), alert, len);
>
> -            (void) com->method->send_alert(ssl, level, alert);
> +            if (com->method->send_alert(ssl, level, alert) != 1) {
> +                goto failed;
> +            }
>          }
>
>          break;
>      }
> +
> +    return;
> +
> +failed:
> +
> +    ngx_post_event(&qc->close, &ngx_posted_events);
>  }
>

Looks good!