[PATCH 1 of 3] HTTP/3: adjusted QUIC connection finalization
Roman Arutyunyan
arut at nginx.com
Wed Nov 10 12:21:01 UTC 2021
On Tue, Nov 09, 2021 at 07:01:32PM +0300, Sergey Kandaurov wrote:
>
> > On 9 Nov 2021, at 10:32, Roman Arutyunyan <arut at 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 at 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().
Yes, resetting the stream will set wev->error, which will lead to errors
in send_chain() while sending HTTP 400 page. As a consequence, we'll try
to reset the stream again, which will lead to nothing because of duplicate
reset protection.
> >>>
> >>> 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);
Here the stream will be reset as part of closing the request because
ngx_http_close_request() does not send HTTP 500 unlike
ngx_http_finalize_request().
> >>> 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.
When we have an error code, we have to reset the stream, because there's no
other way to send that code to client (except closing the QUIC connection).
When we have no code, we can return HTTP 500 instead which may look
inconsistent.
However, when ngx_http_v3_parse_XXX() return NGX_ERROR, it's not because of
parsing (in this case we have an error code), but because of allocation etc.
This is similar to an allocation error in any other part of the code, including
common HTTP code where we just return HTTP 500. So I don't think we need to
reset the stream here.
Actually, ngx_http_v3_parse_data() does not currently return NGX_ERROR at all,
but this fact does not change the API.
> >>> 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).
Yes. But this does not seem to break anything.
--
Roman Arutyunyan
More information about the nginx-devel
mailing list