[QUIC] padding of Initial packets

[Vladimir Homutov]

On Tue, Feb 08, 2022 at 03:42:54PM +0300, Vladimir Homutov wrote:
> On Tue, Feb 08, 2022 at 02:10:04PM +0300, Andrey Kolyshkin wrote:
> > Hello.
> >
> > This patch is strange.
> > 1. ngx_quic_revert_send can set to ctx an uninitialized value from
> > preserved_pnum. (example if min > len and i = 0, only 0 element is filled
> > in preserved_pnum but restored all)
> > 2. ngx_quic_revert_send will restored pnum for ctx that have already called
> > ngx_quic_output_packet and the packet with this pnum will be queued.
> > (example if min > len and i = 1)
>
> thank you for noticing.
> indeed, this needs to be fixed. we don't want to restore contexts we
> didn't yet touch.


The suggested fix is below. Also, while investigating the issue
thoroughly, we found that it is also possible to run into negative ctx->inflight
when discarding context. This is  addressed by a second patch.

# HG changeset patch
# User Vladimir Homutov <vl at nginx.com>
# Date 1644411201 -10800
#      Wed Feb 09 15:53:21 2022 +0300
# Branch quic
# Node ID a4fb28741e19af426228e64b8d2c02ed3950b538
# Parent  dde5cb0205ef8c2a2a3255e7bd369a9c644f2049
QUIC: fixed output context restoring.

The cd8018bc81a5 fixed unintended send of non-padded initial packets,
but failed to restore context properly: only processed contexts need
to be restored.  As a consequence, a packet number could be restored
from uninitialized value.

diff --git a/src/event/quic/ngx_event_quic_output.c b/src/event/quic/ngx_event_quic_output.c
--- a/src/event/quic/ngx_event_quic_output.c
+++ b/src/event/quic/ngx_event_quic_output.c
@@ -165,7 +165,7 @@ ngx_quic_create_datagrams(ngx_connection
             if (min > len) {
                 /* padding can't be applied - avoid sending the packet */

-                for (i = 0; i < NGX_QUIC_SEND_CTX_LAST; i++) {
+                while (i-- > 0) {
                     ctx = &qc->send_ctx[i];
                     ngx_quic_revert_send(c, ctx, preserved_pnum[i]);
                 }


# HG changeset patch
# User Vladimir Homutov <vl at nginx.com>
# Date 1644411102 -10800
#      Wed Feb 09 15:51:42 2022 +0300
# Branch quic
# Node ID 2e27c45e2edb2c9540b211040d314b1748865820
# Parent  a4fb28741e19af426228e64b8d2c02ed3950b538
QUIC: fixed in-flight bytes accounting.

Initially, frames are genereated and stored in ctx->frames.
Next, ngx_quic_output() collects frames to be sent in in ctx->sending.
On failure, ngx_quic_revert_sned() returns frames into ctx->frames.

On success, the ngx_quic_commit_send() moves ack-eliciting frames into
ctx->sent and frees non-ack-eliciting frames.
This function also updates in-flight bytes counter, so only actually sent
frames are accounted.

The counter is decremented in the following cases:
 - acknowledgment is received
 - packet was declared lost
 - we are discarding context completely

In each of this cases frame is removed from ctx->sent queue and in-flight
counter is accordingly decremented.

The patch fixes the case of discarding context - only removing frames
from ctx->sent must be followed by in-flight bytes counter decrement,
otherwise cg->in_flight could experience type underflow.

The issue appeared in b1676cd64dc9.

diff --git a/src/event/quic/ngx_event_quic.c b/src/event/quic/ngx_event_quic.c
--- a/src/event/quic/ngx_event_quic.c
+++ b/src/event/quic/ngx_event_quic.c
@@ -1092,7 +1092,6 @@ ngx_quic_discard_ctx(ngx_connection_t *c
         ngx_queue_remove(q);

         f = ngx_queue_data(q, ngx_quic_frame_t, queue);
-        ngx_quic_congestion_ack(c, f);
         ngx_quic_free_frame(c, f);
     }