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

[Sergey Kandaurov]

> 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