You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by "arturobernalg (via GitHub)" <gi...@apache.org> on 2024/12/20 21:19:45 UTC
[PR] HTTPCLIENT-2352: Improved flow control window handling to prevent overflow/underflow [httpcomponents-core]
arturobernalg opened a new pull request, #510:
URL: https://github.com/apache/httpcomponents-core/pull/510
This PR addresses issues related to the flow control window management in the HTTP/2 implementation of Apache HttpClient. Specifically, it ensures that flow control window updates do not cause `ArithmeticException` due to overflow or underflow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2352: Improved flow control window handling to prevent overflow/underflow [httpcomponents-core]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #510:
URL: https://github.com/apache/httpcomponents-core/pull/510#issuecomment-2558138168
> if (newValue > Integer.MAX_VALUE || newValue < Integer.MIN_VALUE) {
> throw new H2ConnectionException(H2Error.FLOW_CONTROL_ERROR,
> "Flow control window exceeded: " + newValue);
> }
@arturobernalg This is hardly any different to what we have now, slightly more readable and slightly less efficient.
I would like to see protocol logs with the exception and ideally a reproducer before any changes are made.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2352: Improved flow control window handling to prevent overflow/underflow [httpcomponents-core]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c closed pull request #510: HTTPCLIENT-2352: Improved flow control window handling to prevent overflow/underflow
URL: https://github.com/apache/httpcomponents-core/pull/510
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2352: Improved flow control window handling to prevent overflow/underflow [httpcomponents-core]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #510:
URL: https://github.com/apache/httpcomponents-core/pull/510#discussion_r1894412006
##########
httpcore5-h2/src/test/java/org/apache/hc/core5/http2/impl/nio/TestAbstractH2StreamMultiplexer.java:
##########
@@ -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);
Review Comment:
I don't think we should write to the console.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2352: Improved flow control window handling to prevent overflow/underflow [httpcomponents-core]
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on PR #510:
URL: https://github.com/apache/httpcomponents-core/pull/510#issuecomment-2558005058
> @arturobernalg I am sorry but this is really not a good idea. We cannot just disable the data window overflow check and hope for the best. HTTP/2 endpoints can make mistakes in calculating their data windows. We need to detect those mistakes and report them as protocol violations
@ok2c I see your point about not bypassing the overflow check. How about keeping the protocol violation handling by throwing an H2ConnectionException on overflow? This way, we still catch misbehaving endpoints without silently ignoring the issue.
```
private int updateWindow(final AtomicInteger window, final int delta) throws H2ConnectionException {
for (;;) {
final int current = window.get();
final long newValue = (long) current + delta;
if (newValue > Integer.MAX_VALUE || newValue < Integer.MIN_VALUE) {
throw new H2ConnectionException(H2Error.FLOW_CONTROL_ERROR,
"Flow control window exceeded: " + newValue);
}
if (window.compareAndSet(current, (int) newValue)) {
return (int) newValue;
}
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2352: Improved flow control window handling to prevent overflow/underflow [httpcomponents-core]
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on PR #510:
URL: https://github.com/apache/httpcomponents-core/pull/510#issuecomment-2558172065
> > ```
> > if (newValue > Integer.MAX_VALUE || newValue < Integer.MIN_VALUE) {
> > throw new H2ConnectionException(H2Error.FLOW_CONTROL_ERROR,
> > "Flow control window exceeded: " + newValue);
> > }
> > ```
>
> @arturobernalg This is hardly any different to what we have now, slightly more readable and slightly less efficient.
>
> I would like to see protocol logs with the exception and ideally a reproducer before any changes are made.
Fair enough @ok2c
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2352: Improved flow control window handling to prevent overflow/underflow [httpcomponents-core]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #510:
URL: https://github.com/apache/httpcomponents-core/pull/510#issuecomment-2557996198
@arturobernalg I am sorry but this is really not a good idea. We cannot just disable the data window overflow check and hope for the best. HTTP/2 endpoints can make mistakes in calculating their data windows. We need to detect those mistakes and report them as protocol violations
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2352: Improved flow control window handling to prevent overflow/underflow [httpcomponents-core]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #510:
URL: https://github.com/apache/httpcomponents-core/pull/510#issuecomment-2568962655
Superseded by #511
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org