> On 9 Nov 2021, at 10:32, Roman Arutyunyan <arut@nginx.com> wrote:
>
> On Mon, Nov 08, 2021 at 07:25:43PM +0300, Sergey Kandaurov wrote:
>> On Mon, Oct 18, 2021 at 03:48:28PM +0300, Roman Arutyunyan wrote:
>>> # HG changeset patch
>>> # User Roman Arutyunyan <arut@nginx.com>
>>> # Date 1634559753 -10800
>>> # Mon Oct 18 15:22:33 2021 +0300
>>> # Branch quic
>>> # Node ID 8739f475583031399879ef0af2eb5af76008449e
>>> # Parent 404de224517e33f685613d6425dcdb3c8ef5b97e
>>> HTTP/3: adjusted QUIC connection finalization.
>>>
>>> When an HTTP/3 function returns an error in context of a QUIC stream, it's
>>> this function's responsibility now to finalize the entire QUIC connection
>>> with the right code, if required. Previously, QUIC connection finalization
>>> could be done both outside and inside such functions. The new rule follows
>>> a similar rule for logging, leads to cleaner code, and allows to provide more
>>> details about the error.
>>>
>>> While here, a few error cases are no longer treated as fatal and QUIC connection
>>> is no longer finalized in these cases. A few other cases now lead to
>>> stream reset instead of connection finalization.
>>>
>>> diff --git a/src/http/v3/ngx_http_v3.c b/src/http/v3/ngx_http_v3.c
>>> --- a/src/http/v3/ngx_http_v3.c
>>> +++ b/src/http/v3/ngx_http_v3.c
>>> @@ -33,7 +33,7 @@ ngx_http_v3_init_session(ngx_connection_
>>>
>>> h3c = ngx_pcalloc(pc->pool, sizeof(ngx_http_v3_session_t));
>>> if (h3c == NULL) {
>>> - return NGX_ERROR;
>>> + goto failed;
>>> }
>>>
>>> h3c->max_push_id = (uint64_t) -1;
>>> @@ -49,7 +49,7 @@ ngx_http_v3_init_session(ngx_connection_
>>>
>>> cln = ngx_pool_cleanup_add(pc->pool, 0);
>>> if (cln == NULL) {
>>> - return NGX_ERROR;
>>> + goto failed;
>>> }
>>>
>>> cln->handler = ngx_http_v3_cleanup_session;
>>> @@ -58,6 +58,14 @@ ngx_http_v3_init_session(ngx_connection_
>>> hc->v3_session = h3c;
>>>
>>> return ngx_http_v3_send_settings(c);
>>> +
>>> +failed:
>>> +
>>> + ngx_log_error(NGX_LOG_ERR, c->log, 0, "failed to create http3 session");
>>> +
>>> + ngx_http_v3_finalize_connection(c, NGX_HTTP_V3_ERR_INTERNAL_ERROR,
>>> + "failed to create http3 session");
>>> + return NGX_ERROR;
>>> }
>>>
>>>
>>> diff --git a/src/http/v3/ngx_http_v3_request.c b/src/http/v3/ngx_http_v3_request.c
>>> --- a/src/http/v3/ngx_http_v3_request.c
>>> +++ b/src/http/v3/ngx_http_v3_request.c
>>> @@ -65,8 +65,6 @@ ngx_http_v3_init(ngx_connection_t *c)
>>> ngx_http_core_srv_conf_t *cscf;
>>>
>>> if (ngx_http_v3_init_session(c) != NGX_OK) {
>>> - ngx_http_v3_finalize_connection(c, NGX_HTTP_V3_ERR_INTERNAL_ERROR,
>>> - "internal error");
>>> ngx_http_close_connection(c);
>>> return;
>>> }
>>> @@ -110,8 +108,6 @@ ngx_http_v3_init(ngx_connection_t *c)
>>> h3c->goaway = 1;
>>>
>>> if (ngx_http_v3_send_goaway(c, (n + 1) << 2) != NGX_OK) {
>>> - ngx_http_v3_finalize_connection(c, NGX_HTTP_V3_ERR_INTERNAL_ERROR,
>>> - "goaway error");
>>> ngx_http_close_connection(c);
>>> return;
>>> }
>>> @@ -287,15 +283,14 @@ ngx_http_v3_process_request(ngx_event_t
>>> rc = ngx_http_v3_parse_headers(c, st, b);
>>>
>>> if (rc > 0) {
>>> - ngx_http_v3_finalize_connection(c, rc,
>>> - "could not parse request headers");
>>> + ngx_quic_reset_stream(c, rc);
>>> + ngx_log_error(NGX_LOG_ERR, r->connection->log, 0,
>>> + "client sent invalid header");
>>> ngx_http_finalize_request(r, NGX_HTTP_BAD_REQUEST);
>>> break;
>>> }

Note that ngx_http_v3_parse_headers() returned > 0 will reset a stream,
then it will try again, now as part of closing a (fake) http connection.
This still works due to a wev->error check in ngx_quic_reset_stream().

>>>
>>> if (rc == NGX_ERROR) {
>>> - ngx_http_v3_finalize_connection(c, NGX_HTTP_V3_ERR_INTERNAL_ERROR,
>>> - "internal error");
>>> ngx_http_close_request(r, NGX_HTTP_INTERNAL_SERVER_ERROR);
>>> break;
>>> }
>>> @@ -1167,17 +1162,13 @@ ngx_http_v3_request_body_filter(ngx_http
>>> }
>>>
>>> if (rc > 0) {
>>> - ngx_http_v3_finalize_connection(r->connection, rc,
>>> - "client sent invalid body");
>>> + ngx_quic_reset_stream(r->connection, rc);
>>> ngx_log_error(NGX_LOG_ERR, r->connection->log, 0,
>>> "client sent invalid body");
>>> return NGX_HTTP_BAD_REQUEST;
>>> }
>>>
>>> if (rc == NGX_ERROR) {
>>> - ngx_http_v3_finalize_connection(r->connection,
>>> - NGX_HTTP_V3_ERR_INTERNAL_ERROR,
>>> - "internal error");
>>> return NGX_HTTP_INTERNAL_SERVER_ERROR;
>>> }
>>>

Note that NGX_ERROR conversion to emitting a special response on request
finalization looks contrary to just resetting a stream (above) on less
fatal logical errors, with rc > 0 (I see only H3_FRAME_UNEXPECTED there).
Not sure about that. OTOH, handling NGX_ERROR this way seems to follow
our others HTTP protocol version implementations.

>>> diff --git a/src/http/v3/ngx_http_v3_streams.c b/src/http/v3/ngx_http_v3_streams.c
>>> --- a/src/http/v3/ngx_http_v3_streams.c
>>> +++ b/src/http/v3/ngx_http_v3_streams.c
>>> @@ -283,7 +283,7 @@ ngx_http_v3_create_push_stream(ngx_conne
>>>
>>> sc = ngx_quic_open_stream(c, 0);
>>> if (sc == NULL) {
>>> - return NULL;
>>> + goto failed;
>>> }
>>>
>>> p = buf;
>>> @@ -318,7 +318,13 @@ ngx_http_v3_create_push_stream(ngx_conne
>>>
>>> failed:
>>>
>>> - ngx_http_v3_close_uni_stream(sc);
>>> + ngx_log_error(NGX_LOG_ERR, c->log, 0, "failed to create push stream");
>>> +
>>> + ngx_http_v3_finalize_connection(c, NGX_HTTP_V3_ERR_STREAM_CREATION_ERROR,
>>> + "failed to create push stream");
>>> + if (sc) {
>>> + ngx_http_v3_close_uni_stream(sc);
>>> + }
>>>
>>> return NULL;
>>> }
>>> @@ -368,7 +374,7 @@ ngx_http_v3_get_uni_stream(ngx_connectio
>>>
>>> sc = ngx_quic_open_stream(c, 0);
>>> if (sc == NULL) {
>>> - return NULL;
>>> + goto failed;
>>> }
>>>
>>> sc->quic->cancelable = 1;
>>> @@ -405,7 +411,13 @@ ngx_http_v3_get_uni_stream(ngx_connectio
>>>
>>> failed:
>>>
>>> - ngx_http_v3_close_uni_stream(sc);
>>> + ngx_log_error(NGX_LOG_ERR, c->log, 0, "failed to create server stream");
>>> +
>>> + ngx_http_v3_finalize_connection(c, NGX_HTTP_V3_ERR_STREAM_CREATION_ERROR,
>>> + "failed to create server stream");
>>> + if (sc) {
>>> + ngx_http_v3_close_uni_stream(sc);
>>> + }
>>>
>>> return NULL;
>>> }
>>> @@ -424,7 +436,7 @@ ngx_http_v3_send_settings(ngx_connection
>>>
>>> cc = ngx_http_v3_get_uni_stream(c, NGX_HTTP_V3_STREAM_CONTROL);
>>> if (cc == NULL) {
>>> - return NGX_DECLINED;
>>> + return NGX_ERROR;
>>> }
>>>
>>> h3scf = ngx_http_v3_get_module_srv_conf(c, ngx_http_v3_module);
>>> @@ -457,6 +469,10 @@ ngx_http_v3_send_settings(ngx_connection
>>>
>>> failed:
>>>
>>> + ngx_log_error(NGX_LOG_ERR, c->log, 0, "failed to send settings");
>>> +
>>> + ngx_http_v3_finalize_connection(c, NGX_HTTP_V3_ERR_EXCESSIVE_LOAD,
>>> + "failed to send settings");
>>> ngx_http_v3_close_uni_stream(cc);
>>>
>>> return NGX_ERROR;
>>> @@ -475,7 +491,7 @@ ngx_http_v3_send_goaway(ngx_connection_t
>>>
>>> cc = ngx_http_v3_get_uni_stream(c, NGX_HTTP_V3_STREAM_CONTROL);
>>> if (cc == NULL) {
>>> - return NGX_DECLINED;
>>> + return NGX_ERROR;
>>
>> This case now misses sending CONNECTION_CLOSE (no further stream processing),
>> Instead, the last allowed stream connection is now closed while initializing
>> with sending FIN with off:0 in its cleanup function which confuses clients
>> due to H3_FRAME_UNEXPECTED. It probably needs to be converted to goto failed
>> with conditional uni stream close.
>>
>> Same in ngx_http_v3_send_settings() around ngx_http_v3_get_uni_stream(), but
>> there it just leads to control stream silent closure with no visible effects
>> on the wire. Yet, with teh 2nd patch applied that updates stream reset logic,
>> the error causes to emit RESET_STREAM(H3_INTERNAL_ERROR), which is a protocol
>> violation when operating on a critical (control) stream.
>
> Can you elaborate on these? The ngx_http_v3_get_uni_stream() function now
> closes the QUIC connection by calling ngx_http_v3_finalize_connection() in case
> of error. So its callers don't need to do this.

Okay, somehow I've missed that.
It looks good.

>
>>> }
>>>
>>> n = ngx_http_v3_encode_varlen_int(NULL, id);
>>> @@ -495,6 +511,10 @@ ngx_http_v3_send_goaway(ngx_connection_t
>>>
>>> failed:
>>>
>>> + ngx_log_error(NGX_LOG_ERR, c->log, 0, "failed to send goaway");
>>> +
>>> + ngx_http_v3_finalize_connection(c, NGX_HTTP_V3_ERR_EXCESSIVE_LOAD,
>>> + "failed to send goaway");
>>> ngx_http_v3_close_uni_stream(cc);
>>>
>>> return NGX_ERROR;
>>> @@ -510,7 +530,7 @@ ngx_http_v3_send_ack_section(ngx_connect
>>> ngx_http_v3_session_t *h3c;
>>>
>>> ngx_log_debug1(NGX_LOG_DEBUG_HTTP, c->log, 0,
>>> - "http3 client ack section %ui", stream_id);
>>> + "http3 send section acknowledgement %ui", stream_id);
>>>
>>> dc = ngx_http_v3_get_uni_stream(c, NGX_HTTP_V3_STREAM_DECODER);
>>> if (dc == NULL) {
>>> @@ -524,11 +544,21 @@ ngx_http_v3_send_ack_section(ngx_connect
>>> h3c->total_bytes += n;
>>>
>>> if (dc->send(dc, buf, n) != (ssize_t) n) {
>>> - ngx_http_v3_close_uni_stream(dc);
>>> - return NGX_ERROR;
>>> + goto failed;
>>> }
>>>
>>> return NGX_OK;
>>> +
>>> +failed:
>>> +
>>> + ngx_log_error(NGX_LOG_ERR, c->log, 0,
>>> + "failed to send section acknowledgement");
>>> +
>>> + ngx_http_v3_finalize_connection(c, NGX_HTTP_V3_ERR_EXCESSIVE_LOAD,
>>> + "failed to send section acknowledgement");
>>> + ngx_http_v3_close_uni_stream(dc);
>>> +
>>> + return NGX_ERROR;
>>> }
>>>
>>>
>>> @@ -541,7 +571,7 @@ ngx_http_v3_send_cancel_stream(ngx_conne
>>> ngx_http_v3_session_t *h3c;
>>>
>>> ngx_log_debug1(NGX_LOG_DEBUG_HTTP, c->log, 0,
>>> - "http3 client cancel stream %ui", stream_id);
>>> + "http3 send stream cancellation %ui", stream_id);
>>>
>>> dc = ngx_http_v3_get_uni_stream(c, NGX_HTTP_V3_STREAM_DECODER);
>>> if (dc == NULL) {
>>> @@ -555,11 +585,20 @@ ngx_http_v3_send_cancel_stream(ngx_conne
>>> h3c->total_bytes += n;
>>>
>>> if (dc->send(dc, buf, n) != (ssize_t) n) {
>>> - ngx_http_v3_close_uni_stream(dc);
>>> - return NGX_ERROR;
>>> + goto failed;
>>> }
>>>
>>> return NGX_OK;
>>> +
>>> +failed:
>>> +
>>> + ngx_log_error(NGX_LOG_ERR, c->log, 0, "failed to send stream cancellation");
>>> +
>>> + ngx_http_v3_finalize_connection(c, NGX_HTTP_V3_ERR_EXCESSIVE_LOAD,
>>> + "failed to send stream cancellation");
>>> + ngx_http_v3_close_uni_stream(dc);
>>> +
>>> + return NGX_ERROR;
>>> }
>>>
>>>
>>> @@ -572,7 +611,7 @@ ngx_http_v3_send_inc_insert_count(ngx_co
>>> ngx_http_v3_session_t *h3c;
>>>
>>> ngx_log_debug1(NGX_LOG_DEBUG_HTTP, c->log, 0,
>>> - "http3 client increment insert count %ui", inc);
>>> + "http3 send insert count increment %ui", inc);
>>>
>>> dc = ngx_http_v3_get_uni_stream(c, NGX_HTTP_V3_STREAM_DECODER);
>>> if (dc == NULL) {
>>> @@ -586,11 +625,21 @@ ngx_http_v3_send_inc_insert_count(ngx_co
>>> h3c->total_bytes += n;
>>>
>>> if (dc->send(dc, buf, n) != (ssize_t) n) {
>>> - ngx_http_v3_close_uni_stream(dc);
>>> - return NGX_ERROR;
>>> + goto failed;
>>> }
>>>
>>> return NGX_OK;
>>> +
>>> +failed:
>>> +
>>> + ngx_log_error(NGX_LOG_ERR, c->log, 0,
>>> + "failed to send insert count increment");
>>> +
>>> + ngx_http_v3_finalize_connection(c, NGX_HTTP_V3_ERR_EXCESSIVE_LOAD,
>>> + "failed to send insert count increment");
>>> + ngx_http_v3_close_uni_stream(dc);
>>> +
>>> + return NGX_ERROR;
>>> }
>>>
>>>
>>> diff --git a/src/http/v3/ngx_http_v3_tables.c b/src/http/v3/ngx_http_v3_tables.c
>>> --- a/src/http/v3/ngx_http_v3_tables.c
>>> +++ b/src/http/v3/ngx_http_v3_tables.c
>>> @@ -589,6 +589,10 @@ ngx_http_v3_check_insert_count(ngx_conne
>>> if (h3c->nblocked == h3scf->max_blocked_streams) {
>>> ngx_log_error(NGX_LOG_INFO, c->log, 0,
>>> "client exceeded http3_max_blocked_streams limit");
>>> +
>>> + ngx_http_v3_finalize_connection(c,
>>> + NGX_HTTP_V3_ERR_DECOMPRESSION_FAILED,
>>> + "too many blocked streams");
>>> return NGX_HTTP_V3_ERR_DECOMPRESSION_FAILED;
>>> }
>>>

Similar to the very beginning comment above, after closing QUIC connection
there and handling decompression error as part of parsing headers, it still
tries to reset a stream to no avail (up to two times, see above).

--
Sergey Kandaurov

_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel