This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 9.0.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 5cb76ac4ba94de4e2792dbd650ba1cd913e5ac49 Author: opencode <[email protected]> AuthorDate: Tue Oct 13 15:49:35 2026 +0200 Fix flow control over-allocation when releasing the HTTP/2 connection back log When the connection window increment was exhausted within the incremental streams at the urgency level where allocation was exhausted, releaseBackLog() distributed the increment using proportional shares that were never capped at the capacity still available, never decremented the running total by the grants that had already been made and never stopped once the capacity ran out. Every incremental stream at that level also received a share rounded up to at least one byte, and any non-i [...] Since allocations made on the back log path are the only record of the use of the connection flow control window, the connection could then send more data than the client's window permitted, which a compliant peer must treat as a connection error of type FLOW_CONTROL_ERROR. With N incremental streams in the back log, a one byte window update could fund N bytes of data. TestRfc9218.testPriority encoded this behaviour as the expected result. The distribution now uses a fixed allocation base so the proportional shares between the incremental streams are unchanged, non-incremental streams at the exhaustion level are allocated in full as they are at lower urgency levels, each grant is capped at the remaining capacity, the remaining capacity is decremented by what was actually granted and distribution stops once the capacity is exhausted. Streams left without an allocation stay in the back log and are considered again when th [...] TestRfc9218.testPriority updated to the corrected expectations: the one byte window update now produces exactly one one byte data frame and the eight kilobyte window update allocations are unchanged. --- .../apache/coyote/http2/Http2UpgradeHandler.java | 55 +++++++++++++++++++--- test/org/apache/coyote/http2/TestRfc9218.java | 30 ++++-------- webapps/docs/changelog.xml | 16 +++++++ 3 files changed, 75 insertions(+), 26 deletions(-) diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java b/java/org/apache/coyote/http2/Http2UpgradeHandler.java index 5ca268b8aa..c63bcebbaa 100644 --- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java +++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java @@ -1300,6 +1300,11 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH // Iteration 2. Allocate. // Reset for second iteration remaining = increment; + // Tracks the allocation available for the proportional + // distribution used for the incremental streams at the urgency + // level where the allocation was exhausted. The value is + // initialised when the first such stream is processed. + int allocationBase = 0; orderedStreamsIterator = orderedStreams.iterator(); while (orderedStreamsIterator.hasNext()) { Stream s = orderedStreamsIterator.next(); @@ -1330,13 +1335,48 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH break; } - // Cast to long during the intermediate calculation to avoid integer overflow in multiplication - int share = (int) ((long) s.getConnectionAllocationRequested() * remaining / - requestedAllocationForIncrementalStreams); - if (share == 0) { - share = 1; + int grant; + if (s.getIncremental()) { + if (allocationBase == 0) { + // First incremental stream at this level. The + // proportional distribution below always + // distributes the same base value, regardless of + // how much of that base has already been granted + // to the streams at this level. + allocationBase = remaining; + } + // Cast to long during the intermediate calculation to avoid integer overflow in multiplication + grant = (int) ((long) s.getConnectionAllocationRequested() * allocationBase / + requestedAllocationForIncrementalStreams); + if (grant == 0) { + grant = 1; + } + } else { + // The proportional distribution above is only + // defined for the incremental streams at this level. + // The non-incremental streams at this level were + // allocated in full during the first iteration (that + // is how the allocation reached the incremental + // streams) so allocate them in full here too. + grant = s.getConnectionAllocationRequested(); + } + if (grant > remaining) { + // Never allocate more than the window available. The + // allocations on this path are the only record of the + // use of the window the client made available. + // Allocating more than that would have the connection + // send more data than the client's flow control window + // permits, which a compliant client must treat as a + // connection error of type FLOW_CONTROL_ERROR. + grant = remaining; } - allocate(s, share); + // Note: the rounding up above combined with the cap + // here may exhaust the remaining allocation early. + // Streams left without an allocation remain in the back + // log and are considered again when the next window + // increment arrives. + int left = allocate(s, grant); + remaining -= (grant - left); result.add(s); if (s.getConnectionAllocationRequested() == 0) { // Fully allocated (unlikely but possible due to @@ -1344,6 +1384,9 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH orderedStreamsIterator.remove(); backLogStreams.remove(s); } + if (remaining < 1) { + break; + } } } } diff --git a/test/org/apache/coyote/http2/TestRfc9218.java b/test/org/apache/coyote/http2/TestRfc9218.java index a540dc7198..9e434dbd81 100644 --- a/test/org/apache/coyote/http2/TestRfc9218.java +++ b/test/org/apache/coyote/http2/TestRfc9218.java @@ -126,34 +126,24 @@ public class TestRfc9218 extends Http2TestBase { Assert.assertEquals(0, trace.length()); output.clearTrace(); - // 1 byte unallocated in connection window // 17 - 5267 body left // 19 - 7022 body left // 21 - 6144 body left - // Add 1 byte to the connection window. Due to rounding up, each stream should get 1 byte. + // Add 1 byte to the connection window. Rounding up means stream 17 takes the whole allocation. The other + // streams remain in the back log since allocating to them as well would exceed the window the client made + // available. sendWindowUpdate(0, 1); parser.readFrame(); - System.out.println(output.getTrace()); - parser.readFrame(); - System.out.println(output.getTrace()); - parser.readFrame(); trace = output.getTrace(); System.out.println(trace); - Assert.assertTrue(trace.contains("17-Body-1\n")); - trace = trace.replace("17-Body-1\n", ""); - Assert.assertTrue(trace.contains("19-Body-1\n")); - trace = trace.replace("19-Body-1\n", ""); - Assert.assertTrue(trace.contains("21-Body-1\n")); - trace = trace.replace("21-Body-1\n", ""); - Assert.assertEquals(0, trace.length()); + Assert.assertEquals("17-Body-1\n", trace); output.clearTrace(); - // 1 byte over allocated in connection window // 17 - 5266 body left - // 19 - 7021 body left - // 21 - 6143 body left + // 19 - 7022 body left + // 21 - 6144 body left // BZ 69614 - invalid priority update frames should be ignored sendInvalidPriorityUpdate(17); @@ -162,8 +152,8 @@ public class TestRfc9218 extends Http2TestBase { sendPriorityUpdate(17, 2, true); /* - * Add 8k to the connection window. Should clear the connection window over allocation and fully allocate 17 - * with the remainder split proportionally between 19 and 21. + * Add 8k to the connection window. Should fully allocate 17 with the remainder split proportionally between 19 + * and 21. */ sendWindowUpdate(0, 1024 * 8); // Use try/catch as third read has been failing on some tests runs @@ -189,8 +179,8 @@ public class TestRfc9218 extends Http2TestBase { trace = trace.replace("21-Body-1365\n", ""); Assert.assertEquals(0, trace.length()); - // 19 - 5641 body left - // 21 - 4778 body left + // 19 - 5462 body left + // 21 - 4779 body left // Add 16k to the connection window. Should fully allocate 19 and 21. sendWindowUpdate(0, 1024 * 16); diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index a95b1a4d0f..c11a903ad9 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -287,6 +287,22 @@ an error has been reported. A single failure to flush the output was previously reported to the listener multiple times. (remm) </fix> + <fix> + When releasing the HTTP/2 connection back log, cap the allocation to + each stream at the connection window capacity that remains to be + distributed, decrement that capacity by every allocation made and stop + distributing once it is exhausted. Previously, when the capacity was + exhausted by incremental streams, allocations could exceed the capacity + the client had made available, since every incremental stream received + a share rounded up to at least one byte and non-incremental streams at + the same urgency level received a share computed using a denominator + that only counted incremental streams. The connection could then send + more data than the client's flow control window allowed, which a + compliant client must treat as a connection error of type + <code>FLOW_CONTROL_ERROR</code>. Streams that are left without an + allocation remain in the back log and are considered again when the + next window increment arrives. (remm) + </fix> </changelog> </subsection> <subsection name="Web applications"> --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
