Skip to content

Commit

Permalink
HTTPCLIENT-2352: Improved flow control window handling to prevent ove…
Browse files Browse the repository at this point in the history
…rflow/underflow

- Updated `updateWindow` method to cap the flow control window at `Integer.MAX_VALUE` and `Integer.MIN_VALUE`, avoiding `ArithmeticException` during updates.
- Modified `maximizeConnWindow` to account for the capped behavior, ensuring seamless operation within flow control limits.
  • Loading branch information
arturobernalg committed Dec 20, 2024
1 parent b435a0b commit 412615b
Show file tree
Hide file tree
Showing 2 changed files with 91 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -198,12 +198,15 @@ abstract H2StreamHandler createLocallyInitiatedStream(
HttpProcessor httpProcessor,
BasicHttpConnectionMetrics connMetrics) throws IOException;

private int updateWindow(final AtomicInteger window, final int delta) throws ArithmeticException {
private int updateWindow(final AtomicInteger window, final int delta) {
for (;;) {
final int current = window.get();
final long newValue = (long) current + delta;
if (Math.abs(newValue) > 0x7fffffffL) {
throw new ArithmeticException("Update causes flow control window to exceed " + Integer.MAX_VALUE);
long newValue = (long) current + delta;
// Cap the new value if it would exceed Integer.MAX_VALUE or go below Integer.MIN_VALUE
if (newValue > Integer.MAX_VALUE) {
newValue = Integer.MAX_VALUE;
} else if (newValue < Integer.MIN_VALUE) {
newValue = Integer.MIN_VALUE;
}
if (window.compareAndSet(current, (int) newValue)) {
return (int) newValue;
Expand Down Expand Up @@ -1040,7 +1043,7 @@ private void consumeDataFrame(final RawFrame frame, final H2Stream stream) throw
}

private void maximizeConnWindow(final int connWinSize) throws IOException {
final int delta = Integer.MAX_VALUE - connWinSize;
final int delta = Integer.MAX_VALUE - 1 - connWinSize; // Use Integer.MAX_VALUE - 1 for a small buffer
if (delta > 0) {
final RawFrame windowUpdateFrame = frameFactory.createWindowUpdate(0, delta);
commitFrame(windowUpdateFrame);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import java.nio.ByteBuffer;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.atomic.AtomicInteger;

import org.apache.hc.core5.function.Supplier;
import org.apache.hc.core5.http.Header;
Expand Down Expand Up @@ -270,5 +271,87 @@ void testInputHeaderContinuationFrame() throws IOException, HttpException {
Mockito.verify(streamHandler).consumeHeader(headersCaptor.capture(), ArgumentMatchers.eq(false));
Assertions.assertFalse(headersCaptor.getValue().isEmpty());
}


@Test
void testUpdateWindowBehaviorComparison() {
AtomicInteger window = new AtomicInteger(1); // Start with window at 1

// Test with new implementation where overflow should be capped at Integer.MAX_VALUE
int resultNew = updateWindowNew(window, Integer.MAX_VALUE);
System.out.println("New implementation: Current window size after attempted overflow: " + resultNew);
Assertions.assertEquals(Integer.MAX_VALUE, resultNew, "New implementation should cap at Integer.MAX_VALUE");

// Reset for old implementation test
window = new AtomicInteger(1);

// Test with old implementation where an overflow should throw an ArithmeticException
try {
updateWindowOld(window, Integer.MAX_VALUE);
Assertions.fail("Old implementation should throw ArithmeticException for overflow");
} catch (final ArithmeticException e) {
System.out.println("Old implementation: ArithmeticException caught: " + e.getMessage());
}

// Test non-overflow scenario for both implementations
window = new AtomicInteger(1);
final int resultNewSafe = updateWindowNew(window, Integer.MAX_VALUE / 2 - 1);
System.out.println("New safe update result: " + resultNewSafe);
window = new AtomicInteger(1); // Reset for old test
final int resultOldSafe = updateWindowOld(window, Integer.MAX_VALUE / 2 - 1);
System.out.println("Old safe update result: " + resultOldSafe);

Assertions.assertEquals(Integer.MAX_VALUE / 2, resultNewSafe, "New implementation safe update");
Assertions.assertEquals(Integer.MAX_VALUE / 2, resultOldSafe, "Old implementation safe update");
// Test capping behavior from below Integer.MAX_VALUE with new implementation
window = new AtomicInteger(Integer.MAX_VALUE - 1);
resultNew = updateWindowNew(window, 2);
System.out.println("New implementation: Current window size after attempted overflow from below: " + resultNew);
Assertions.assertEquals(Integer.MAX_VALUE, resultNew, "New implementation should cap at Integer.MAX_VALUE from below");

// Test overflow from below with old implementation
window = new AtomicInteger(Integer.MAX_VALUE - 1);
try {
updateWindowOld(window, 2);
Assertions.fail("Old implementation should throw ArithmeticException for overflow from below");
} catch (final ArithmeticException e) {
System.out.println("Old implementation: ArithmeticException caught for overflow from below: " + e.getMessage());
}
}

// New implementation of updateWindow method with capping
private int updateWindowNew(final AtomicInteger window, final int delta) {
for (;;) {
final int current = window.get();
long newValue = (long) current + delta;

if (newValue > Integer.MAX_VALUE) {
newValue = Integer.MAX_VALUE;
} else if (newValue < Integer.MIN_VALUE) {
newValue = Integer.MIN_VALUE;
}

if (window.compareAndSet(current, (int) newValue)) {
return (int) newValue;
}
}
}

// Old implementation of updateWindow method which throws exception on overflow
private int updateWindowOld(final AtomicInteger window, final int delta) throws ArithmeticException {
for (;;) {
final int current = window.get();
System.out.println("Old - Current: " + current + ", Delta: " + delta);
final long newValue = (long) current + delta;
System.out.println("Old - New value before check: " + newValue);
if (Math.abs(newValue) > 0x7fffffffL) {
throw new ArithmeticException("Update causes flow control window to exceed " + Integer.MAX_VALUE);
}
if (window.compareAndSet(current, (int) newValue)) {
System.out.println("Old - New value set: " + (int) newValue);
return (int) newValue;
}
}
}
}

0 comments on commit 412615b

Please sign in to comment.