Skip to content

Commit b6319f5

Browse files
vyvy
committed
8369595: HttpClient: HttpHeaders.firstValueAsLong failures should be converted to ProtocolException
Reviewed-by: dfuchs, djelinski
1 parent 6e2ab84 commit b6319f5

File tree

7 files changed

+546
-423
lines changed

7 files changed

+546
-423
lines changed

‎src/java.net.http/share/classes/jdk/internal/net/http/Http1Response.java‎

Lines changed: 43 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
import java.lang.invoke.MethodHandles;
3131
import java.lang.invoke.VarHandle;
3232
import java.net.http.HttpResponse.BodySubscriber;
33+
import java.net.ProtocolException;
3334
import java.nio.ByteBuffer;
3435
import java.util.concurrent.CompletableFuture;
3536
import java.util.concurrent.Executor;
@@ -46,6 +47,7 @@
4647
import jdk.internal.net.http.common.Utils;
4748
import static java.net.http.HttpClient.Version.HTTP_1_1;
4849
import static java.net.http.HttpResponse.BodySubscribers.discarding;
50+
import static jdk.internal.net.http.common.Utils.readContentLength;
4951
import static jdk.internal.net.http.common.Utils.wrapWithExtraDetail;
5052
import static jdk.internal.net.http.RedirectFilter.HTTP_NOT_MODIFIED;
5153

@@ -272,16 +274,31 @@ long fixupContentLen(long clen) {
272274
}
273275

274276
/**
275-
* Read up to MAX_IGNORE bytes discarding
277+
* Reads the body, if it is present and less than {@value MAX_IGNORE} bytes.
278+
* Otherwise, just the connection is closed.
276279
*/
277280
public CompletableFuture<Void> ignoreBody(Executor executor) {
278-
int clen = (int)headers.firstValueAsLong("Content-Length").orElse(-1);
279-
if (clen == -1 || clen > MAX_IGNORE) {
281+
282+
// Read the `Content-Length` header
283+
long clen;
284+
try {
285+
clen = readContentLength(headers, "", -1);
286+
} catch (ProtocolException pe) {
287+
return MinimalFuture.failedFuture(pe);
288+
}
289+
290+
// Read the body, if it is present and less than `MAX_IGNORE` bytes.
291+
//
292+
// We proceed with reading the body even when `Content-Length: 0` to
293+
// ensure that the happy path is taken, and upon success, the connection
294+
// is returned back to the pool.
295+
if (clen != -1 && clen <= MAX_IGNORE) {
296+
return readBody(discarding(), !request.isWebSocket(), executor);
297+
} else {
280298
connection.close();
281299
return MinimalFuture.completedFuture(null); // not treating as error
282-
} else {
283-
return readBody(discarding(), !request.isWebSocket(), executor);
284300
}
301+
285302
}
286303

287304
// Used for those response codes that have no body associated
@@ -308,11 +325,28 @@ public <U> CompletableFuture<U> readBody(HttpResponse.BodySubscriber<U> p,
308325
this.return2Cache = return2Cache;
309326
final BodySubscriber<U> subscriber = p;
310327

311-
312328
final CompletableFuture<U> cf = new MinimalFuture<>();
313329

314-
long clen0 = headers.firstValueAsLong("Content-Length").orElse(-1L);
315-
final long clen = fixupContentLen(clen0);
330+
Consumer<Throwable> errorNotifier = error -> {
331+
try {
332+
subscriber.onError(error);
333+
cf.completeExceptionally(error);
334+
} finally {
335+
asyncReceiver.setRetryOnError(false);
336+
asyncReceiver.onReadError(error);
337+
}
338+
};
339+
340+
// Read the content length
341+
long clen;
342+
try {
343+
long clen0 = readContentLength(headers, "", -1);
344+
clen = fixupContentLen(clen0);
345+
} catch (ProtocolException pe) {
346+
errorNotifier.accept(pe);
347+
connection.close(pe);
348+
return cf;
349+
}
316350

317351
// expect-continue reads headers and body twice.
318352
// if we reach here, we must reset the headersReader state.
@@ -398,12 +432,7 @@ public <U> CompletableFuture<U> readBody(HttpResponse.BodySubscriber<U> p,
398432
}
399433
});
400434

401-
ResponseSubscribers.getBodyAsync(executor, p, cf, (t) -> {
402-
subscriber.onError(t);
403-
cf.completeExceptionally(t);
404-
asyncReceiver.setRetryOnError(false);
405-
asyncReceiver.onReadError(t);
406-
});
435+
ResponseSubscribers.getBodyAsync(executor, p, cf, errorNotifier);
407436

408437
return cf.whenComplete((s,t) -> {
409438
if (t != null) {

‎src/java.net.http/share/classes/jdk/internal/net/http/Http3ExchangeImpl.java‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,8 @@
8181
import jdk.internal.net.http.quic.streams.QuicBidiStream;
8282
import jdk.internal.net.http.quic.streams.QuicStreamReader;
8383
import jdk.internal.net.http.quic.streams.QuicStreamWriter;
84+
85+
import static jdk.internal.net.http.common.Utils.readContentLength;
8486
import static jdk.internal.net.http.http3.ConnectionSettings.UNLIMITED_MAX_FIELD_SECTION_SIZE;
8587

8688
/**
@@ -1293,12 +1295,16 @@ protected void handlePromise(PushHeadersConsumer consumer) throws IOException {
12931295
if (Set.of("PUT", "DELETE", "OPTIONS", "TRACE").contains(method)) {
12941296
throw new ProtocolException("push method not allowed pushId=" + pushId);
12951297
}
1296-
long clen = promiseHeaders.firstValueAsLong("Content-Length").orElse(-1);
1298+
1299+
// Read & validate `Content-Length`
1300+
long clen = readContentLength(
1301+
promiseHeaders, "illegal push headers for pushId=%s: ".formatted(pushId), -1);
12971302
if (clen > 0) {
1298-
throw new ProtocolException("push headers contain non-zero Content-Length for pushId=" + pushId);
1303+
throw new ProtocolException("push headers contain non-zero \"Content-Length\" for pushId=" + pushId);
12991304
}
1305+
13001306
if (promiseHeaders.firstValue("Transfer-Encoding").isPresent()) {
1301-
throw new ProtocolException("push headers contain Transfer-Encoding for pushId=" + pushId);
1307+
throw new ProtocolException("push headers contain \"Transfer-Encoding\" for pushId=" + pushId);
13021308
}
13031309

13041310

‎src/java.net.http/share/classes/jdk/internal/net/http/Http3Stream.java‎

Lines changed: 20 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@
3131
import java.net.http.HttpHeaders;
3232
import java.nio.ByteBuffer;
3333
import java.util.List;
34-
import java.util.OptionalLong;
3534
import java.util.concurrent.ConcurrentLinkedQueue;
3635
import java.util.concurrent.atomic.AtomicInteger;
3736

@@ -58,6 +57,8 @@
5857

5958
import static jdk.internal.net.http.Exchange.MAX_NON_FINAL_RESPONSES;
6059
import static jdk.internal.net.http.RedirectFilter.HTTP_NOT_MODIFIED;
60+
import static jdk.internal.net.http.common.Utils.readContentLength;
61+
import static jdk.internal.net.http.common.Utils.readStatusCode;
6162

6263
/**
6364
* A common super class for the HTTP/3 request/response stream ({@link Http3ExchangeImpl}
@@ -606,23 +607,17 @@ void handleResponse(HttpHeadersBuilder responseHeadersBuilder,
606607
}
607608

608609
int responseCode;
609-
boolean finalResponse = false;
610610
try {
611-
responseCode = (int) responseHeaders
612-
.firstValueAsLong(":status")
613-
.orElseThrow(() -> new IOException("no statuscode in response"));
614-
} catch (IOException | NumberFormatException exception) {
611+
responseCode = readStatusCode(responseHeaders, "");
612+
} catch (ProtocolException pe) {
615613
// RFC-9114: 4.1.2. Malformed Requests and Responses:
616614
// "Malformed requests or responses that are
617615
// detected MUST be treated as a stream error of type H3_MESSAGE_ERROR"
618-
cancelImpl(exception, Http3Error.H3_MESSAGE_ERROR);
619-
return;
620-
}
621-
if (responseCode < 100 || responseCode > 999) {
622-
cancelImpl(new IOException("Unexpected :status header value"), Http3Error.H3_MESSAGE_ERROR);
616+
cancelImpl(pe, Http3Error.H3_MESSAGE_ERROR);
623617
return;
624618
}
625619

620+
boolean finalResponse = false;
626621
if (responseCode >= 200) {
627622
responseState = ResponseState.PERMIT_TRAILER;
628623
finalResponse = true;
@@ -653,23 +648,21 @@ void handleResponse(HttpHeadersBuilder responseHeadersBuilder,
653648
responseHeaders);
654649
}
655650

656-
try {
657-
OptionalLong cl = responseHeaders.firstValueAsLong("content-length");
658-
if (finalResponse && cl.isPresent()) {
659-
long cll = cl.getAsLong();
660-
if (cll < 0) {
661-
cancelImpl(new IOException("Invalid content-length value "+cll), Http3Error.H3_MESSAGE_ERROR);
662-
return;
663-
}
664-
if (!(exchange.request().method().equalsIgnoreCase("HEAD") || responseCode == HTTP_NOT_MODIFIED)) {
665-
// HEAD response and 304 response might have a content-length header,
666-
// but it carries no meaning
667-
contentLength = cll;
668-
}
651+
if (finalResponse) {
652+
long cl;
653+
try {
654+
cl = readContentLength(responseHeaders, "", -1);
655+
} catch (ProtocolException pe) {
656+
cancelImpl(pe, Http3Error.H3_MESSAGE_ERROR);
657+
return;
658+
}
659+
if (cl != -1 &&
660+
!(exchange.request().method().equalsIgnoreCase("HEAD") ||
661+
responseCode == HTTP_NOT_MODIFIED)) {
662+
// HEAD response and 304 response might have a content-length header,
663+
// but it carries no meaning
664+
contentLength = cl;
669665
}
670-
} catch (NumberFormatException nfe) {
671-
cancelImpl(nfe, Http3Error.H3_MESSAGE_ERROR);
672-
return;
673666
}
674667

675668
if (Log.headers() || debug.on()) {

‎src/java.net.http/share/classes/jdk/internal/net/http/MultiExchange.java‎

Lines changed: 27 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,11 @@
2828
import java.io.IOException;
2929
import java.lang.ref.WeakReference;
3030
import java.net.ConnectException;
31+
import java.net.ProtocolException;
32+
import java.net.ProtocolException;
3133
import java.net.http.HttpClient.Version;
3234
import java.net.http.HttpConnectTimeoutException;
35+
import java.net.http.HttpHeaders;
3336
import java.net.http.StreamLimitException;
3437
import java.time.Duration;
3538
import java.util.List;
@@ -47,7 +50,6 @@
4750
import java.util.function.Function;
4851

4952
import java.net.http.HttpClient;
50-
import java.net.http.HttpHeaders;
5153
import java.net.http.HttpRequest;
5254
import java.net.http.HttpResponse;
5355
import java.net.http.HttpResponse.BodySubscriber;
@@ -62,6 +64,7 @@
6264
import static jdk.internal.net.http.common.MinimalFuture.completedFuture;
6365
import static jdk.internal.net.http.common.MinimalFuture.failedFuture;
6466
import static jdk.internal.net.http.AltSvcProcessor.processAltSvcHeader;
67+
import static jdk.internal.net.http.common.Utils.readContentLength;
6568

6669

6770
/**
@@ -358,13 +361,22 @@ private static boolean bodyNotPermitted(Response r) {
358361
return r.statusCode == 204;
359362
}
360363

361-
private boolean bodyIsPresent(Response r) {
362-
HttpHeaders headers = r.headers();
363-
if (headers.firstValueAsLong("Content-length").orElse(0L) != 0L)
364-
return true;
365-
if (headers.firstValue("Transfer-encoding").isPresent())
366-
return true;
367-
return false;
364+
private void ensureNoBody(HttpHeaders headers) throws ProtocolException {
365+
366+
// Check `Content-Length`
367+
var contentLength = readContentLength(headers, "", 0);
368+
if (contentLength > 0) {
369+
throw new ProtocolException(
370+
"Unexpected \"Content-Length\" header in a 204 response: " + contentLength);
371+
}
372+
373+
// Check `Transfer-Encoding`
374+
var transferEncoding = headers.firstValue("Transfer-Encoding");
375+
if (transferEncoding.isPresent()) {
376+
throw new ProtocolException(
377+
"Unexpected \"Transfer-Encoding\" header in a 204 response: " + transferEncoding.get());
378+
}
379+
368380
}
369381

370382
// Call the user's body handler to get an empty body object
@@ -405,13 +417,13 @@ private HttpResponse<T> setNewResponse(HttpRequest request, Response r, T body,
405417
if (bodyNotPermitted(r)) {
406418
// No response body consumption is expected, we can cancel the timer right away
407419
cancelTimer();
408-
if (bodyIsPresent(r)) {
409-
IOException ioe = new IOException(
410-
"unexpected content length header with 204 response");
411-
exch.cancel(ioe);
412-
return MinimalFuture.failedFuture(ioe);
413-
} else
414-
return handleNoBody(r, exch);
420+
try {
421+
ensureNoBody(r.headers);
422+
} catch (ProtocolException pe) {
423+
exch.cancel(pe);
424+
return MinimalFuture.failedFuture(pe);
425+
}
426+
return handleNoBody(r, exch);
415427
}
416428
return exch.readBodyAsync(responseHandler)
417429
.thenApply((T body) -> setNewResponse(r.request, r, body, exch));

‎src/java.net.http/share/classes/jdk/internal/net/http/Stream.java‎

Lines changed: 15 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@
6161
import static jdk.internal.net.http.AltSvcProcessor.processAltSvcFrame;
6262

6363
import static jdk.internal.net.http.Exchange.MAX_NON_FINAL_RESPONSES;
64+
import static jdk.internal.net.http.common.Utils.readStatusCode;
6465

6566
/**
6667
* Http/2 Stream handling.
@@ -615,18 +616,14 @@ String checkInterimResponseCountExceeded() {
615616
return null;
616617
}
617618

618-
protected void handleResponse(HeaderFrame hf) throws IOException {
619+
protected void handleResponse(HeaderFrame hf) {
619620
HttpHeaders responseHeaders = responseHeadersBuilder.build();
620621

621622
if (!finalResponseCodeReceived) {
622623
try {
623-
responseCode = (int) responseHeaders
624-
.firstValueAsLong(":status")
625-
.orElseThrow(() -> new ProtocolException(String.format(
626-
"Stream %s PROTOCOL_ERROR: no status code in response",
627-
streamid)));
628-
} catch (ProtocolException cause) {
629-
cancelImpl(cause, ResetFrame.PROTOCOL_ERROR);
624+
responseCode = readStatusCode(responseHeaders, "Stream %s PROTOCOL_ERROR: ".formatted(streamid));
625+
} catch (ProtocolException pe) {
626+
cancelImpl(pe, ResetFrame.PROTOCOL_ERROR);
630627
rspHeadersConsumer.reset();
631628
return;
632629
}
@@ -1730,21 +1727,18 @@ protected void handleResponse(HeaderFrame hf) {
17301727
HttpHeaders responseHeaders = responseHeadersBuilder.build();
17311728

17321729
if (!finalPushResponseCodeReceived) {
1733-
responseCode = (int)responseHeaders
1734-
.firstValueAsLong(":status")
1735-
.orElse(-1);
1736-
1737-
if (responseCode == -1) {
1738-
cancelImpl(new ProtocolException("No status code"), ResetFrame.PROTOCOL_ERROR);
1730+
try {
1731+
responseCode = readStatusCode(responseHeaders, "");
1732+
if (responseCode >= 100 && responseCode < 200) {
1733+
String protocolErrorMsg = checkInterimResponseCountExceeded();
1734+
if (protocolErrorMsg != null) {
1735+
throw new ProtocolException(protocolErrorMsg);
1736+
}
1737+
}
1738+
} catch (ProtocolException pe) {
1739+
cancelImpl(pe, ResetFrame.PROTOCOL_ERROR);
17391740
rspHeadersConsumer.reset();
17401741
return;
1741-
} else if (responseCode >= 100 && responseCode < 200) {
1742-
String protocolErrorMsg = checkInterimResponseCountExceeded();
1743-
if (protocolErrorMsg != null) {
1744-
cancelImpl(new ProtocolException(protocolErrorMsg), ResetFrame.PROTOCOL_ERROR);
1745-
rspHeadersConsumer.reset();
1746-
return;
1747-
}
17481742
}
17491743

17501744
this.finalPushResponseCodeReceived = true;

0 commit comments

Comments
 (0)