8263031: HttpClient throws Exception if it receives a Push Promise that is too large #7696

c-cleary · 2022-03-04T14:42:40Z

Problem
When a Continuation Frame is received by the httpclient using HTTP/2 after a Push Promise frame (can happen if the amount of headers to be sent in a single Push Promise frame exceeds the maximum frame size, so a Continuation frame is required), the following exception occurs:

java.io.IOException: no statuscode in response
at java.net.http/jdk.internal.net.http.HttpClientImpl.send(HttpClientImpl.java:565)
at java.net.http/jdk.internal.net.http.HttpClientFacade.send(HttpClientFacade.java:119)
...

This exception occurs because there is no existing flow in jdk/internal/net/http/Http2Connection.java which accounts for the case where a PushPromiseFrame is received with the END_HEADERS flag set to 0x0. When this occurs, the only acceptable frame/s (as multiple continuations are also acceptable) that can be received by the client on the same stream is a continuation frame.

Fix
To ensure correct behavior, the following changes were made to jdk/internal/net/http/Http2Connection.java.

The existing method handlePushPromise() was modified so that if the END_HEADERS flag is unset (flags equal to 0x0), then a record used to track the state of the Push Promise containing a shared HeaderDecoder and the received PushPromiseFrame is initialised.
When the subsequent ContinuationFrame is received in processFrame(), the method handlePushContinuation() is called instead of the default flow resulting in stream.incoming(frame) being called (the source of the incorrect behaviour originally).
In handlePushContinuation(), the shared decoder is used to decode the received ContinuationFrame headers and if the END_HEADERS flag is set (flags equal to 0x4), the HttpHeaders object for the Push Promise as a whole is constructed which serves to combine the headers from both the PushPromiseFrame and the ContinuationFrame.

A regression test was included which verifies that the exception is not thrown and that the headers arrive correctly.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8263031: HttpClient throws Exception if it receives a Push Promise that is too large

Reviewers

Daniel Fuchs (@dfuch - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/7696/head:pull/7696
$ git checkout pull/7696

Update a local copy of the PR:
$ git checkout pull/7696
$ git pull https://git.openjdk.java.net/jdk pull/7696/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7696

View PR using the GUI difftool:
$ git pr show -t 7696

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/7696.diff

…at is too large

bridgekeeper · 2022-03-04T14:44:18Z

👋 Welcome back ccleary! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2022-03-04T14:46:42Z

@c-cleary The following label will be automatically applied to this pull request:

net

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2022-03-04T14:52:31Z

Webrevs

c-cleary · 2022-03-04T15:17:11Z

I see that a couple of imports got changed by my IDE, will address that shortly

c-cleary · 2022-03-07T08:20:04Z

I see that a couple of imports got changed by my IDE, will address that shortly

Now resolved

c-cleary · 2022-03-07T11:38:17Z

test/jdk/java/net/httpclient/http2/PushPromiseContinuation.java

+            Map<String, List<String>> map = new HashMap<>();
+            map.put("x-promise", List.of(mainPromiseBody));
+            HttpHeaders headers = HttpHeaders.of(map, ACCEPT_ALL);
+            exchange.serverPush(uri, headers, is);


Could create all of the headers here instead of in Http2LPPTestExchangeImpl, might improve readability of test.

dfuch · 2022-03-07T12:08:49Z

src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java

+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import java.io.EOFException;
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.net.InetSocketAddress;
+import java.net.URI;
+import java.net.http.HttpClient;
+import java.net.http.HttpHeaders;
+import java.nio.ByteBuffer;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.Flow;
+import java.util.function.Function;
+import java.util.function.Supplier;
+


Could we keep the original alphabetical ordering and have the java.* imports come before the jdk.* imports?

Done, this was just an IDE hiccup

src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java

dfuch · 2022-03-07T12:11:05Z

src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java

+                if (frame instanceof ContinuationFrame cf) {
+                    handlePushContinuation(stream, cf);
+                } else {
+                    // TODO: Maybe say what kind of frame was received instead


We should either do it or remove the TODO. @Michael-Mc-Mahon what would you suggest here?

dfuch · 2022-03-07T12:17:53Z

src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java

+            // stream is a Continuation frame
+            if (pcs != null) {
+                if (frame instanceof ContinuationFrame cf) {
+                    handlePushContinuation(stream, cf);


IOException should probably be caught and transformed in protocol error here to?
In case of protocol error when handling push promise (or their continuation) - should pcs be reset to null?
Maybe it doesn't matter since we're closing the connection anyway.

In my opinion it would probably be safest to set pcs to null here yes.

dfuch · 2022-03-07T12:22:22Z

test/jdk/java/net/httpclient/http2/PushPromiseContinuation.java

+ * @run testng/othervm PushPromiseContinuation
+ */


It would be good to add an @summary tag to explain the purpose of this test

test/jdk/java/net/httpclient/http2/PushPromiseContinuation.java

dfuch · 2022-03-07T12:27:01Z

test/jdk/java/net/httpclient/http2/PushPromiseContinuation.java

+            System.err.println("Server: Push sent");
+        }
+    }
+}


missing newline at end of file?

dfuch · 2022-03-07T12:28:29Z

test/jdk/java/net/httpclient/http2/PushPromiseContinuation.java

+            OutgoingPushPromise pp = new OutgoingPushPromise(streamid, uri, pushPromiseHeaders, content);
+            // Indicates to the client that a continuation should be expected
+            pp.setFlag(0x0);
+
+            // Create the continuation frame
+            List<ByteBuffer> encodedHeaders = conn.encodeHeaders(continuationHeaders);
+            ContinuationFrame cf = new ContinuationFrame(streamid, HeaderFrame.END_HEADERS, encodedHeaders);
+
+            try {


It would be good to have a test-case that creates 2 continuation frames too.

Good idea yes, to check that the repeat continuation still behaves as expected. Should hopefully be straight forward to create another test case.

On this issue, there is a case where a faulty server might send an indefinite number of Continuations (maybe the server never sets an END_HEADERS flag). Should a safe guard for the Push Promise with Continuation/s case be put in place to prevent the faulty scenario?

It would be impossible to detect, but if the server is faulty and "forget" to set the END_HEADERS flag, then we will detect that because the next frame we receive won't be the ContinuationFrame we expect.

dfuch · 2022-03-07T12:31:41Z

src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java

    }

+    private record PushContinuationState(HeaderDecoder pushContDecoder, PushPromiseFrame pushContFrame) {}
+    private volatile PushContinuationState pcs;


I'd prefer to have a longer name than pcs - pushContinuationState would make it less mysterious at the place where it's used. Also it could be good to move these declarations (linew 854 and 855) higher in the file, where other instance variables are declared.

Changed to pushContinuationState, its a long name but definitely clearer

dfuch · 2022-03-15T15:16:03Z

src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java

                    // TODO: Maybe say what kind of frame was received instead
-                    protocolError(ErrorFrame.PROTOCOL_ERROR, "Expected Continuation frame but received another type");
+                    pushContinuationState = null;
+                    protocolError(ErrorFrame.PROTOCOL_ERROR, "Expected a Continuation frame but received " + frame);


In all other places in this method we have return; just after a call to protocolError, except in the two places where your changes added one. For consistency you should probably add this return; statement, even if it's not strictly needed. It would avoid having to have to analyze the whole structure of the nested if - then - else to figure out that it's actually not needed.

dfuch · 2022-03-15T15:22:04Z

src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java

    private <T> void handlePushContinuation(Stream<T> parent, ContinuationFrame cf)
            throws IOException {
-        decodeHeaders(cf, pcs.pushContDecoder);
+        decodeHeaders(cf, pushContinuationState.pushContDecoder);


I suggest declaring a local variable here to avoid reading pushContinuationState more than once.
Something like:

var pcs = pushContinuationState;

then use pcs wherever needed in that method.

dfuch · 2022-03-15T15:26:01Z

test/jdk/java/net/httpclient/http2/PushPromiseContinuation.java


    static HttpHeaders testHeaders;
    static HttpHeadersBuilder testHeadersBuilder;
+    static int continuationCount;


Since these three static variables are set by one thread and read by another - they should all be volatile.

This reverts commit 5c1c0f5.

dfuch

Thanks for making these changes! Still a few comments...

dfuch · 2022-03-22T10:53:17Z

src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java


 package jdk.internal.net.http;

+import jdk.internal.net.http.HttpConnection.HttpPublisher;


Could this line be pushed down to where other jdk.* imports are made?

dfuch · 2022-03-22T10:57:35Z

test/jdk/java/net/httpclient/http2/PushPromiseContinuation.java

+        HttpRequest hreq = HttpRequest.newBuilder(uri).version(HttpClient.Version.HTTP_2).GET().build();
+        CompletableFuture<HttpResponse<String>> cf =
+                client.sendAsync(hreq, HttpResponse.BodyHandlers.ofString(UTF_8), pph);
+        cf.join();


I'd suggest checking that we received the expected response code + body too

Further changes made to check response codes and bodies of both the response and push promise.

dfuch · 2022-03-24T18:21:45Z

src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java

+                        handlePushContinuation(stream, cf);
+                    } catch (UncheckedIOException e) {
+                        debug.log("Error handling Push Promise with Continuation: " + e.getMessage(), e);
+                        protocolError(ResetFrame.PROTOCOL_ERROR, e.getMessage());


I believe it would more clear to use ErrorFrame.PROTOCOL_ERROR or GoAwayFrame.PROTOCOL_ERRROR here and in the other calls to protocolError below, since we're not resetting the stream here but closing the whole connection with a GoAwayFrame.

Oh yes, good point. I think ErrorFrame.PROTOCOL_ERROR would be the most appropriate here. I'll amend the change accordingly.

However, I'll look into the specification further for the other cases and see if they need be changed as well. Though closing the whole connection with GoAwayFrame seems correct

I changed three more occurences to ErrorFrame.PROTOCOL_ERROR, its easier to understand and lines up more clearly with specification

Make sure to run tier1, tier2 before integrating. Drop me a note when you are ready I will sponsor the change.

openjdk · 2022-03-28T11:30:16Z

@c-cleary This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8263031: HttpClient throws Exception if it receives a Push Promise that is too large

Reviewed-by: dfuchs

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 425 new commits pushed to the master branch:

8567266: 8283683: Make ThreadLocalRandom a final class
f4eaa16: 8283728: jdk.hotspot.agent: Wrong location for RISCV64ThreadContext.java
cdef087: 8283727: P11KeyGenerator has import statement with two semicolons after JDK-8267319
7f12537: 8283558: Parallel: Pass PSIsAliveClosure to ReferenceProcessor constructor
66f1da1: 8281222: ciTypeFlow::profiled_count fails "assert(0 <= i && i < _len) failed: illegal index"
c2c0cb2: 8282668: HotSpot Style Guide should permit unrestricted unions
b0daf70: 8263134: HotSpot Style Guide should disallow inheriting constructors
c587b29: 8283720: ProblemList java/time/test/java/time/TestZoneOffset.java
d5f9059: 8283695: [AIX] Build failure due to name conflict in test_arguments.cpp
f520b4f: 8283668: Update IllegalFormatException to use sealed classes
... and 415 more: https://git.openjdk.java.net/jdk/compare/bc6148407e629bd99fa5a8577ebd90320610f349...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@dfuch) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

c-cleary · 2022-03-28T12:26:28Z

/integrate

openjdk · 2022-03-28T12:27:25Z

@c-cleary
Your change (at version b6d34d8) is now ready to be sponsored by a Committer.

dfuch · 2022-04-07T08:41:04Z

/sponsor

openjdk · 2022-04-07T08:42:47Z

Going to push as commit 4d2cd26.
Since your change was applied there have been 564 commits pushed to the master branch:

61fcf2f: 8284389: Improve stability of GHA Pre-submit testing by caching cygwin installer
d5cd4a3: 8283387: [macos] a11y : Screen magnifier does not show selected Tab
5a21397: 8284387: Fix formatting of doc comments in jdk.javadoc
4451257: 8284437: Building from different users/workspace is not always deterministic
dd4a1bb: 8284299: Handle inheritDoc misuse more gracefully
46ce2ef: 8277517: Bump minimum boot jdk to JDK 18
77388ea: 8284368: Remove finalizer method in jdk.crypto.cryptoki
8e4fab0: 8284303: runtime/Thread/AsyncExceptionTest.java timed out
3cd3a83: 8284167: Make internal javac exceptions stackless
a385142: 8177107: Reduce memory footprint of java.lang.reflect.Constructor/Method
... and 554 more: https://git.openjdk.java.net/jdk/compare/bc6148407e629bd99fa5a8577ebd90320610f349...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-04-07T08:43:03Z

@dfuch @c-cleary Pushed as commit 4d2cd26.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

c-cleary added 4 commits February 23, 2022 14:31

8263031: HttpClient throws Exception if it receives a Push Promise th…

d6240c9

…at is too large

8263031: Update test name and cleanup

94b8df9

8263031: Setting END_HEADERS flag where appropriate

210e9a7

8263031: Created new flow for Push Promises followed by Continuations

527cfac

c-cleary marked this pull request as ready for review March 4, 2022 14:46

openjdk bot added net net-dev@openjdk.org rfr Pull request is ready for review labels Mar 4, 2022

8263031: Tidied up import statements

664a110

c-cleary commented Mar 7, 2022

View reviewed changes

dfuch reviewed Mar 7, 2022

View reviewed changes

c-cleary added 2 commits March 15, 2022 14:42

8263031: Added test for multiple Continuation Frames

1c0de3b

8263031: Cleanup of changes in Http2Connection

c14f599

dfuch reviewed Mar 15, 2022

View reviewed changes

c-cleary added 5 commits March 15, 2022 15:30

8263031: Correct ordering of imports

c2ae1e5

Test cleanup, added third case

5c1c0f5

Revert "Test cleanup, added third case"

04db16e

This reverts commit 5c1c0f5.

8263031: Test cleanup, added third case

640992b

8263031: Added return statements and pcs var

c0543da

dfuch reviewed Mar 22, 2022

View reviewed changes

c-cleary added 2 commits March 22, 2022 15:25

8263031: Removed duplicate import

31e3941

8263031: further verification of push promise and response

55e2d99

dfuch reviewed Mar 24, 2022

View reviewed changes

8263031: changed to ErrorFrame.PROTOCOL_ERROR

b6d34d8

dfuch approved these changes Mar 28, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 28, 2022

openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 28, 2022

openjdk bot added the integrated Pull request has been integrated label Apr 7, 2022

openjdk bot closed this Apr 7, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Apr 7, 2022


		package jdk.internal.net.http;

		import jdk.internal.net.http.HttpConnection.HttpPublisher;

8263031: HttpClient throws Exception if it receives a Push Promise that is too large #7696

8263031: HttpClient throws Exception if it receives a Push Promise that is too large #7696

Uh oh!

Conversation

c-cleary commented Mar 4, 2022 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Mar 4, 2022

Uh oh!

openjdk bot commented Mar 4, 2022

Uh oh!

mlbridge bot commented Mar 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

c-cleary commented Mar 4, 2022

Uh oh!

c-cleary commented Mar 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

c-cleary Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfuch Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

c-cleary commented Mar 4, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 4, 2022 •

edited

Loading

c-cleary Mar 24, 2022 •

edited

Loading

dfuch Mar 24, 2022 •

edited

Loading

openjdk bot commented Mar 28, 2022 •

edited

Loading