[PATCH 2 of 2] QUIC: improved split frames error handling

[Roman Arutyunyan]

On Tue, May 02, 2023 at 04:14:58PM +0400, Sergey Kandaurov wrote:
> 
> > On 1 May 2023, at 19:26, Sergey Kandaurov <pluknet at nginx.com> wrote:
> > 
> > # HG changeset patch
> > # User Sergey Kandaurov <pluknet at 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 <pluknet at 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 at nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel

Both this and the patch #1 look good.

--
Roman Arutyunyan