Skip to content

Commit

Permalink
8242999: HTTP/2 client may not handle CONTINUATION frames correctly
Browse files Browse the repository at this point in the history 
Updated jdk.internal.net.http.Stream.incoming(Http2Frame frame) to handle continuation frame with END_HEADER flag

Backport-of: 184b433
  • Loading branch information
@TheRealMDoerr
TheRealMDoerr committed Jun 6, 2024
1 parent 48cf5a3 commit ee370ed
Show file tree
Hide file tree
Showing 2 changed files with 80 additions and 22 deletions.
8 changes: 4 additions & 4 deletions src/java.net.http/share/classes/jdk/internal/net/http/Stream.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -397,10 +397,10 @@ void incoming(Http2Frame frame) throws IOException {
if (hframe.endHeaders()) {
Log.logTrace("handling response (streamid={0})", streamid);
handleResponse();
if (hframe.getFlag(HeaderFrame.END_STREAM)) {
if (debug.on()) debug.log("handling END_STREAM: %d", streamid);
receiveDataFrame(new DataFrame(streamid, DataFrame.END_STREAM, List.of()));
}
}
if (hframe.getFlag(HeaderFrame.END_STREAM)) {
if (debug.on()) debug.log("handling END_STREAM: %d", streamid);
receiveDataFrame(new DataFrame(streamid, DataFrame.END_STREAM, List.of()));
}
} else if (frame instanceof DataFrame) {
receiveDataFrame((DataFrame)frame);
Expand Down
94 changes: 76 additions & 18 deletions test/jdk/java/net/httpclient/http2/ContinuationFrameTest.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2017, 2018, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2017, 2020, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -29,6 +29,7 @@
* java.net.http/jdk.internal.net.http.frame
* java.net.http/jdk.internal.net.http.hpack
* @library /test/lib server
* @compile ../ReferenceTracker.java
* @build Http2TestServer
* @build jdk.test.lib.net.SimpleSSLContext
* @run testng/othervm ContinuationFrameTest
Expand Down Expand Up @@ -72,6 +73,9 @@ public class ContinuationFrameTest {
Http2TestServer https2TestServer; // HTTP/2 ( h2 )
String http2URI;
String https2URI;
String noBodyhttp2URI;
String noBodyhttps2URI;
final ReferenceTracker TRACKER = ReferenceTracker.INSTANCE;

/**
* A function that returns a list of 1) a HEADERS frame ( with an empty
Expand All @@ -87,6 +91,23 @@ public class ContinuationFrameTest {
return List.of(hf, cf);
};

/**
* A function that returns a list of 1) a HEADERS frame with END_STREAM
* ( and with an empty payload ), and 2) two CONTINUATION frames,the first
* is empty and the second contains headers and the END_HEADERS flag
*/
static BiFunction<Integer,List<ByteBuffer>,List<Http2Frame>> twoContinuation =
(Integer streamid, List<ByteBuffer> encodedHeaders) -> {
List<ByteBuffer> empty = List.of(ByteBuffer.wrap(new byte[0]));
HeadersFrame hf = new HeadersFrame(streamid, HeaderFrame.END_STREAM, empty);
ContinuationFrame cf = new ContinuationFrame(streamid, 0,empty);
ContinuationFrame cf1 = new ContinuationFrame(streamid,
HeaderFrame.END_HEADERS,
encodedHeaders);

return List.of(hf, cf, cf1);
};

/**
* A function that returns a list of a HEADERS frame followed by a number of
* CONTINUATION frames. Each frame contains just a single byte of payload.
Expand All @@ -112,15 +133,20 @@ public class ContinuationFrameTest {
@DataProvider(name = "variants")
public Object[][] variants() {
return new Object[][] {
{ http2URI, false, oneContinuation },
{ https2URI, false, oneContinuation },
{ http2URI, true, oneContinuation },
{ https2URI, true, oneContinuation },

{ http2URI, false, byteAtATime },
{ https2URI, false, byteAtATime },
{ http2URI, true, byteAtATime },
{ https2URI, true, byteAtATime },
{ http2URI, false, oneContinuation },
{ https2URI, false, oneContinuation },
{ http2URI, true, oneContinuation },
{ https2URI, true, oneContinuation },

{ noBodyhttp2URI, false, twoContinuation },
{ noBodyhttp2URI, true, twoContinuation },
{ noBodyhttps2URI, false, twoContinuation },
{ noBodyhttps2URI, true, twoContinuation },

{ http2URI, false, byteAtATime },
{ https2URI, false, byteAtATime },
{ http2URI, true, byteAtATime },
{ https2URI, true, byteAtATime },
};
}

Expand All @@ -136,8 +162,13 @@ void test(String uri,

HttpClient client = null;
for (int i=0; i< ITERATION_COUNT; i++) {
if (!sameClient || client == null)
client = HttpClient.newBuilder().sslContext(sslContext).build();
if (!sameClient || client == null) {
client = HttpClient.newBuilder()
.proxy(HttpClient.Builder.NO_PROXY)
.sslContext(sslContext)
.build();
TRACKER.track(client);
}

HttpRequest request = HttpRequest.newBuilder(URI.create(uri))
.POST(BodyPublishers.ofString("Hello there!"))
Expand All @@ -149,6 +180,13 @@ void test(String uri,
resp = client.sendAsync(request, BodyHandlers.ofString()).join();
}

if(uri.contains("nobody")) {
out.println("Got response: " + resp);
assertTrue(resp.statusCode() == 204,
"Expected 204, got:" + resp.statusCode());
assertEquals(resp.version(), HTTP_2);
continue;
}
out.println("Got response: " + resp);
out.println("Got body: " + resp.body());
assertTrue(resp.statusCode() == 200,
Expand All @@ -166,13 +204,17 @@ public void setup() throws Exception {

http2TestServer = new Http2TestServer("localhost", false, 0);
http2TestServer.addHandler(new Http2EchoHandler(), "/http2/echo");
http2TestServer.addHandler(new Http2NoBodyHandler(), "/http2/nobody");
int port = http2TestServer.getAddress().getPort();
http2URI = "http://localhost:" + port + "/http2/echo";
noBodyhttp2URI = "http://localhost:" + port + "/http2/nobody";

https2TestServer = new Http2TestServer("localhost", true, sslContext);
https2TestServer.addHandler(new Http2EchoHandler(), "/https2/echo");
https2TestServer.addHandler(new Http2NoBodyHandler(), "/https2/nobody");
port = https2TestServer.getAddress().getPort();
https2URI = "https://localhost:" + port + "/https2/echo";
noBodyhttps2URI = "https://localhost:" + port + "/https2/nobody";

// Override the default exchange supplier with a custom one to enable
// particular test scenarios
Expand All @@ -185,8 +227,15 @@ public void setup() throws Exception {

@AfterTest
public void teardown() throws Exception {
http2TestServer.stop();
https2TestServer.stop();
AssertionError fail = TRACKER.check(500);
try {
http2TestServer.stop();
https2TestServer.stop();
} finally {
if (fail != null) {
throw fail;
}
}
}

static class Http2EchoHandler implements Http2Handler {
Expand All @@ -204,6 +253,17 @@ public void handle(Http2TestExchange t) throws IOException {
}
}

static class Http2NoBodyHandler implements Http2Handler {
@Override
public void handle(Http2TestExchange t) throws IOException {
try (InputStream is = t.getRequestBody();
OutputStream os = t.getResponseBody()) {
byte[] bytes = is.readAllBytes();
t.sendResponseHeaders(204, -1);
}
}
}

// A custom Http2TestExchangeImpl that overrides sendResponseHeaders to
// allow headers to be sent with a number of CONTINUATION frames.
static class CFTHttp2TestExchange extends Http2TestExchangeImpl {
Expand All @@ -225,7 +285,7 @@ static void setHeaderFrameSupplier(BiFunction<Integer,List<ByteBuffer>,List<Http
@Override
public void sendResponseHeaders(int rCode, long responseLength) throws IOException {
this.responseLength = responseLength;
if (responseLength > 0 || responseLength < 0) {
if (responseLength != 0 && rCode != 204) {
long clen = responseLength > 0 ? responseLength : 0;
rspheadersBuilder.setHeader("Content-length", Long.toString(clen));
}
Expand All @@ -236,10 +296,8 @@ public void sendResponseHeaders(int rCode, long responseLength) throws IOExcepti
List<Http2Frame> headerFrames = headerFrameSupplier.apply(streamid, encodeHeaders);
assert headerFrames.size() > 0; // there must always be at least 1

if (responseLength < 0) {
headerFrames.get(headerFrames.size() -1).setFlag(HeadersFrame.END_STREAM);
if(headerFrames.get(0).getFlag(HeaderFrame.END_STREAM))
os.closeInternal();
}

for (Http2Frame f : headerFrames)
conn.outputQ.put(f);
Expand Down

1 comment on commit ee370ed

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

Issues

Please sign in to comment.