Connection receive-flow-control over-count: bytes_consumed charged before recvstate records the bytes, not rolled back on concealed error

[ultragoose]

Summary

apply_stream_frame() charges the connection-level receive-flow-control counter
(conn->ingress.max_data.bytes_consumed) before quicly_recvstate_update() durably
records the bytes, and never rolls the charge back when the update returns a concealed,
non-fatal error. The counter drifts permanently ahead of the bytes actually in recvstate,
and after enough drift a later, perfectly valid frame trips a spurious
QUICLY_TRANSPORT_ERROR_FLOW_CONTROL (0x3) that tears down an otherwise healthy connection.

Confirmed on master (ed83c7c); the same code is present on kazuho/multipath-05.
Reproduced in production on a memory-constrained receiver under sustained multipath
cross-path reordering.

The three links

1. Charge is applied before the record -- lib/quicly.c, apply_stream_frame() (master):

/* flow control */
if (stream->stream_id >= 0) {
    uint64_t max_stream_data = frame->offset + frame->data.len;
    ...
    if (stream->recvstate.received.ranges[num_ranges - 1].end < max_stream_data) {
        uint64_t newly_received = max_stream_data - ranges[num_ranges - 1].end;
        if (bytes_consumed + newly_received > sender.max_committed)
            return QUICLY_TRANSPORT_ERROR_FLOW_CONTROL;
        bytes_consumed += newly_received;          /* <-- charged here (line ~2374) */
    }
}
...
size_t apply_len = frame->data.len;
if ((ret = quicly_recvstate_update(&stream->recvstate, frame->offset, &apply_len,
                                   frame->is_fin, stream->_recv_aux.max_ranges)) != 0)
    return ret;                                    /* <-- record here; charge NOT undone */

2. The update can return a concealed error with the bytes unrecorded -- lib/recvstate.c,
quicly_recvstate_update():

if ((ret = quicly_ranges_add(&state->received, off, off + *len)) != 0)
    return ret;                                    /* PTLS_ERROR_NO_MEMORY: range NOT added */
if (state->received.num_ranges > max_ranges)
    return QUICLY_ERROR_STATE_EXHAUSTION;          /* range already added here */

quicly_ranges_add() -> insert_at() returns PTLS_ERROR_NO_MEMORY when it cannot grow the
range array (malloc failure, lib/ranges.c:56) -- the received range is not recorded, but
the caller has already charged the bytes.

3. do_receive() swallows the error -- lib/quicly.c:

case PTLS_ERROR_NO_MEMORY:
case QUICLY_ERROR_STATE_EXHAUSTION:
case QUICLY_ERROR_PACKET_IGNORED:
    break;                 /* no initiate_close: connection survives, packet dropped */

The connection keeps running with bytes_consumed permanently ahead of the recorded data.

Consequence

When the peer retransmits the dropped region, ranges[last].end is still below
max_stream_data, so the same bytes are charged again. The over-count grows by ~1 path-MTU
per malloc failure and accumulates until bytes_consumed + newly_received > sender.max_committed
on a later, valid frame -> FLOW_CONTROL_ERROR -> connection torn down. Multipath reordering
makes this reachable in practice: the received-range array is routinely large (fragmented across
paths), so quicly_ranges_add growth allocations are frequent and a single failure is enough to
seed the drift.

Note QUICLY_ERROR_STATE_EXHAUSTION (recvstate.c, num_ranges > max_ranges) is not a
source of over-count: by that point quicly_ranges_add has already committed the range, so the
bytes are genuinely recorded. Only the NO_MEMORY path (range not added, charge stands)
over-counts. Any fix must therefore preserve the charge on the STATE_EXHAUSTION path.

Suggested fix

Keep the admission check before the update (reject an over-window frame without mutating state),
but apply the charge after quicly_recvstate_update(), from the durable advance of the
received high-water:

charge = (transfer_complete ? eos : ranges[last].end) - top_before

On NO_MEMORY the high-water does not advance -> nothing charged, and the retransmission by the
peer is charged exactly once. STATE_EXHAUSTION and all success paths advance the high-water ->
charged exactly once, reproducing the previous value on every non-error path.

Patch (applies cleanly to master and kazuho/multipath-05)

@@ apply_stream_frame @@
-    /* flow control */
+    /* Connection-level charge is applied AFTER quicly_recvstate_update() below, from the
+     * durable advance of the received high-water -- never before, so a concealed
+     * PTLS_ERROR_NO_MEMORY from quicly_ranges_add() (bytes not recorded) charges nothing,
+     * while QUICLY_ERROR_STATE_EXHAUSTION (range already committed) still charges once. */
+    uint64_t stream_top_before = 0;
     if (stream->stream_id >= 0) {
         uint64_t max_stream_data = frame->offset + frame->data.len;
-        if (stream->recvstate.received.ranges[stream->recvstate.received.num_ranges - 1].end < max_stream_data) {
-            uint64_t newly_received =
-                max_stream_data - stream->recvstate.received.ranges[stream->recvstate.received.num_ranges - 1].end;
+        stream_top_before = stream->recvstate.received.ranges[stream->recvstate.received.num_ranges - 1].end;
+        if (stream_top_before < max_stream_data) {
+            uint64_t newly_received = max_stream_data - stream_top_before;
             if (stream->conn->ingress.max_data.bytes_consumed + newly_received >
                 stream->conn->ingress.max_data.sender.max_committed)
                 return QUICLY_TRANSPORT_ERROR_FLOW_CONTROL;
-            stream->conn->ingress.max_data.bytes_consumed += newly_received;
         }
     }

     /* update recvbuf */
     size_t apply_len = frame->data.len;
-    if ((ret = quicly_recvstate_update(&stream->recvstate, frame->offset, &apply_len,
-                                       stream->_recv_aux.max_ranges)) != 0)
+    ret = quicly_recvstate_update(&stream->recvstate, frame->offset, &apply_len, frame->is_fin,
+                                  stream->_recv_aux.max_ranges);
+
+    /* charge for the bytes durably recorded above (advance of the received high-water) */
+    if (stream->stream_id >= 0) {
+        uint64_t stream_top_after = quicly_recvstate_transfer_complete(&stream->recvstate)
+                                        ? stream->recvstate.eos
+                                        : stream->recvstate.received.ranges[stream->recvstate.received.num_ranges - 1].end;
+        if (stream_top_after > stream_top_before) {
+            stream->conn->ingress.max_data.bytes_consumed += stream_top_after - stream_top_before;
+        }
+    }
+
+    if (ret != 0)
         return ret;

Happy to open a PR with this if you prefer.