On Tue, May 02, 2023 at 04:14:58PM +0400, Sergey Kandaurov wrote:
>
> > On 1 May 2023, at 19:26, Sergey Kandaurov <pluk...@nginx.com> wrote:
> >
> > # HG changeset patch
> > # User Sergey Kandaurov <pluk...@nginx.com>
> > # Date 1682954724 -14400
> > # Mon May 01 19:25:24 2023 +0400
> > # Branch quic
> > # Node ID b10aa30b15a802870eb23716ce3937a1085c4c98
> > # Parent 8aa3363bc83d4354b3142e3972cce5c0ef523539
> > QUIC: improved split frames error handling.
> >
> > Do not update frame data chain on ngx_quic_read_buffer() error.
> > It may be used later as part of error handling, which envolves
> > writing CONNECTION_CLOSE and pending frames, including the one
> > with the corrupted chain pointer. Since ngx_quic_read_buffer()
> > returns the original chain, there is no point in updating it,
> > so this was simplified.
> >
> > diff --git a/src/event/quic/ngx_event_quic_frames.c
> > b/src/event/quic/ngx_event_quic_frames.c
> > --- a/src/event/quic/ngx_event_quic_frames.c
> > +++ b/src/event/quic/ngx_event_quic_frames.c
> > @@ -359,8 +359,7 @@ ngx_quic_split_frame(ngx_connection_t *c
> > ngx_memzero(&qb, sizeof(ngx_quic_buffer_t));
> > qb.chain = f->data;
> >
> > - f->data = ngx_quic_read_buffer(c, &qb, of->length);
> > - if (f->data == NGX_CHAIN_ERROR) {
> > + if (ngx_quic_read_buffer(c, &qb, of->length) == NGX_CHAIN_ERROR) {
> > return NGX_ERROR;
> > }
> >
>
> Below is an updated version after discussion with Roman.
> It uses conservative approach to maintain API and not rely on that the
> passed output chain pointer will not be modified. Theoretically it can
> if the limit inside ngx_quic_read_buffer() is zero on the first iteration
> so that the passed chain pointer will be zeroed. Another possibility is
> ngx_quic_split_chain() potentially may be rewritten to insert a new
> element in ngx_quic_split_chain() first.
>
> # HG changeset patch
> # User Sergey Kandaurov <pluk...@nginx.com>
> # Date 1683029654 -14400
> # Tue May 02 16:14:14 2023 +0400
> # Branch quic
> # Node ID 491623c24eb30876f60196e6ca99e5284de43cd5
> # Parent 8aa3363bc83d4354b3142e3972cce5c0ef523539
> QUIC: fixed split frames error handling.
>
> Do not corrupt frame data chain pointer on ngx_quic_read_buffer() error.
> The error leads to closing a QUIC connection where the frame may be used
> as part of the QUIC connection tear down, which envolves writing pending
> frames, including this one.
>
> diff --git a/src/event/quic/ngx_event_quic_frames.c
> b/src/event/quic/ngx_event_quic_frames.c
> --- a/src/event/quic/ngx_event_quic_frames.c
> +++ b/src/event/quic/ngx_event_quic_frames.c
> @@ -319,6 +319,7 @@ ngx_int_t
> ngx_quic_split_frame(ngx_connection_t *c, ngx_quic_frame_t *f, size_t len)
> {
> size_t shrink;
> + ngx_chain_t *out;
> ngx_quic_frame_t *nf;
> ngx_quic_buffer_t qb;
> ngx_quic_ordered_frame_t *of, *onf;
> @@ -359,11 +360,13 @@ ngx_quic_split_frame(ngx_connection_t *c
> ngx_memzero(&qb, sizeof(ngx_quic_buffer_t));
> qb.chain = f->data;
>
> - f->data = ngx_quic_read_buffer(c, &qb, of->length);
> - if (f->data == NGX_CHAIN_ERROR) {
> + out = ngx_quic_read_buffer(c, &qb, of->length);
> + if (out == NGX_CHAIN_ERROR) {
> return NGX_ERROR;
> }
>
> + f->data = out;
> +
> nf = ngx_quic_alloc_frame(c);
> if (nf == NULL) {
> return NGX_ERROR;
>
>
> --
> Sergey Kandaurov
> _______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel
Both this and the patch #1 look good.
--
Roman Arutyunyan
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
