[PATCH] QUIC openssl compat mode error handling

[Roman Arutyunyan]

Hi,

> On 22 Sep 2023, at 19:58, Vladimir Homutov <vl at inspert.ru> wrote:
> 
> 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.

I assume, send_alert() would return an error on a fatal condition, not just a local error.

> 
>> 
>> --
>> Roman Arutyunyan
> 
>> # HG changeset patch
>> # User Vladimir Khomutov <vl at inspert.ru <mailto: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!

Committed, thanks.

----
Roman Arutyunyan
arut at nginx.com




-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20230925/23d54e24/attachment.htm>