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;
> > }
> >
> > 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;
> > }
> >
> > 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.

> > }
> >
> > 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;
> > }
> >
> _______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel

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