http-client-branch: HttpClient HTTP/1.1 could be more lenient and accept LF as well as CRLF as header separator.
8203427: HttpClient HTTP/1.1 could be more lenient and accept LF as well as CRLF as header separator.
Reviewed-by: chegar
--- a/src/java.net.http/share/classes/jdk/internal/net/http/Http1HeaderParser.java Wed May 23 16:44:13 2018 +0100
+++ b/src/java.net.http/share/classes/jdk/internal/net/http/Http1HeaderParser.java Thu May 24 11:31:50 2018 +0100
@@ -51,8 +51,10 @@
enum State { STATUS_LINE,
STATUS_LINE_FOUND_CR,
+ STATUS_LINE_FOUND_LF,
STATUS_LINE_END,
STATUS_LINE_END_CR,
+ STATUS_LINE_END_LF,
HEADER,
HEADER_FOUND_CR,
HEADER_FOUND_LF,
@@ -69,7 +71,10 @@
int responseCode() { return responseCode; }
/** Returns the headers, possibly empty. */
- HttpHeaders headers() { assert state == State.FINISHED; return headers; }
+ HttpHeaders headers() {
+ assert state == State.FINISHED : "Unexpected state " + state;
+ return headers;
+ }
/**
* Parses HTTP/1.X status-line and headers from the given bytes. Must be
@@ -84,18 +89,22 @@
boolean parse(ByteBuffer input) throws ProtocolException {
requireNonNull(input, "null input");
- while (input.hasRemaining() && state != State.FINISHED) {
+ while (canContinueParsing(input)) {
switch (state) {
case STATUS_LINE:
readResumeStatusLine(input);
break;
+ // fallthrough
case STATUS_LINE_FOUND_CR:
+ case STATUS_LINE_FOUND_LF:
readStatusLineFeed(input);
break;
case STATUS_LINE_END:
maybeStartHeaders(input);
break;
+ // fallthrough
case STATUS_LINE_END_CR:
+ case STATUS_LINE_END_LF:
maybeEndHeaders(input);
break;
case HEADER:
@@ -121,19 +130,33 @@
return state == State.FINISHED;
}
+ private boolean canContinueParsing(ByteBuffer buffer) {
+ // some states don't require any input to transition
+ // to the next state.
+ switch (state) {
+ case FINISHED: return false;
+ case STATUS_LINE_FOUND_LF: return true;
+ case STATUS_LINE_END_LF: return true;
+ case HEADER_FOUND_LF: return true;
+ default: return buffer.hasRemaining();
+ }
+ }
+
private void readResumeStatusLine(ByteBuffer input) {
char c = 0;
while (input.hasRemaining() && (c =(char)input.get()) != CR) {
+ if (c == LF) break;
sb.append(c);
}
-
if (c == CR) {
state = State.STATUS_LINE_FOUND_CR;
+ } else if (c == LF) {
+ state = State.STATUS_LINE_FOUND_LF;
}
}
private void readStatusLineFeed(ByteBuffer input) throws ProtocolException {
- char c = (char)input.get();
+ char c = state == State.STATUS_LINE_FOUND_LF ? LF : (char)input.get();
if (c != LF) {
throw protocolException("Bad trailing char, \"%s\", when parsing status-line, \"%s\"",
c, sb.toString());
@@ -158,6 +181,8 @@
char c = (char)input.get();
if (c == CR) {
state = State.STATUS_LINE_END_CR;
+ } else if (c == LF) {
+ state = State.STATUS_LINE_END_LF;
} else {
sb.append(c);
state = State.HEADER;
@@ -165,9 +190,9 @@
}
private void maybeEndHeaders(ByteBuffer input) throws ProtocolException {
- assert state == State.STATUS_LINE_END_CR;
+ assert state == State.STATUS_LINE_END_CR || state == State.STATUS_LINE_END_LF;
assert sb.length() == 0;
- char c = (char)input.get();
+ char c = state == State.STATUS_LINE_END_LF ? LF : (char)input.get();
if (c == LF) {
headers = ImmutableHeaders.of(privateMap);
privateMap = null;
@@ -212,8 +237,8 @@
private void resumeOrLF(ByteBuffer input) {
assert state == State.HEADER_FOUND_CR || state == State.HEADER_FOUND_LF;
- char c = (char)input.get();
- if (c == LF && state == State.HEADER_FOUND_CR) {
+ char c = state == State.HEADER_FOUND_LF ? LF : (char)input.get();
+ if (c == LF) {
// header value will be flushed by
// resumeOrSecondCR if next line does not
// begin by SP or HT
@@ -231,7 +256,7 @@
private void resumeOrSecondCR(ByteBuffer input) {
assert state == State.HEADER_FOUND_CR_LF;
char c = (char)input.get();
- if (c == CR) {
+ if (c == CR || c == LF) {
if (sb.length() > 0) {
// no continuation line - flush
// previous header value.
@@ -239,7 +264,13 @@
sb = new StringBuilder();
addHeaderFromString(headerString);
}
- state = State.HEADER_FOUND_CR_LF_CR;
+ if (c == CR) {
+ state = State.HEADER_FOUND_CR_LF_CR;
+ } else {
+ state = State.FINISHED;
+ headers = ImmutableHeaders.of(privateMap);
+ privateMap = null;
+ }
} else if (c == SP || c == HT) {
assert sb.length() != 0;
sb.append(SP); // continuation line
--- a/test/jdk/java/net/httpclient/whitebox/java.net.http/jdk/internal/net/http/Http1HeaderParserTest.java Wed May 23 16:44:13 2018 +0100
+++ b/test/jdk/java/net/httpclient/whitebox/java.net.http/jdk/internal/net/http/Http1HeaderParserTest.java Thu May 24 11:31:50 2018 +0100
@@ -32,6 +32,8 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
import java.util.stream.IntStream;
import sun.net.www.MessageHeader;
import org.testng.annotations.Test;
@@ -132,6 +134,11 @@
"Connection: keep-alive\r\n\r\n"
};
Arrays.stream(basic).forEach(responses::add);
+ // add some tests where some of the CRLF are replaced
+ // by a single LF
+ Arrays.stream(basic)
+ .map(Http1HeaderParserTest::mixedCRLF)
+ .forEach(responses::add);
String[] foldingTemplate =
{ "HTTP/1.1 200 OK\r\n" +
@@ -187,6 +194,12 @@
for (String template : foldingTemplate)
responses.add(template.replace("$NEWLINE", newLineChar));
}
+ // add some tests where some of the CRLF are replaced
+ // by a single LF
+ for (String newLineChar : new String[] { "\n", "\r", "\r\n" }) {
+ for (String template : foldingTemplate)
+ responses.add(mixedCRLF(template).replace("$NEWLINE", newLineChar));
+ }
String[] bad = // much of this is to retain parity with legacy MessageHeaders
{ "HTTP/1.1 200 OK\r\n" +
@@ -237,20 +250,77 @@
return responses.stream().map(p -> new Object[] { p }).toArray(Object[][]::new);
}
+ static final AtomicInteger index = new AtomicInteger();
+ static final AtomicInteger limit = new AtomicInteger(1);
+ static final AtomicBoolean useCRLF = new AtomicBoolean();
+ // A small method to replace part of the CRLF present in a string
+ // with simple LF. The method uses a deterministic algorithm based
+ // on current values of static index/limit/useCRLF counters.
+ // These counters are used to produce a stream of substitutes that
+ // looks like this:
+ // LF CRLF LF LF CRLF CRLF LF LF LF CRLF CRLF CRLF (then repeat from start)
+ static final String mixedCRLF(String headers) {
+ int next;
+ int start = 0;
+ int last = headers.lastIndexOf("\r\n");
+ String prev = "";
+ StringBuilder res = new StringBuilder();
+ while ((next = headers.indexOf("\r\n", start)) > 0) {
+ res.append(headers.substring(start, next));
+ if ("\n".equals(prev) && next == last) {
+ // for some reason the legacy MessageHeader parser will
+ // not consume the final LF if the headers are terminated
+ // by <LF><CRLF> instead of <CRLF><CRLF>. It consume
+ // <LF><CR> but leaves the last <LF> in the stream.
+ // Here we just make sure to avoid using <LF><CRLF>
+ // as that would cause the legacy parser to consume
+ // 1 byte less than the Http1HeadersParser - which
+ // does consume the last <LF>, as it should.
+ // if this is the last CRLF and the previous one
+ // was replaced by LF then use LF.
+ res.append(prev);
+ } else {
+ prev = useCRLF.get() ? "\r\n" : "\n";
+ res.append(prev);
+ }
+ // skip CRLF
+ start = next + 2;
+
+ // The idea is to substitute some of the CRLF with LF.
+ // Rather than doing this randomly, always use the following
+ // sequence:
+ // LF CRLF LF LF CRLF CRLF LF LF LF CRLF CRLF CRLF
+ index.incrementAndGet();
+ if (index.get() == limit.get()) {
+ index.set(0);
+ if (useCRLF.get()) limit.incrementAndGet();
+ if (limit.get() > 3) limit.set(1);
+ useCRLF.set(!useCRLF.get());
+ }
+ }
+ res.append(headers.substring(start));
+ return res.toString();
+ }
+
+
@Test(dataProvider = "responses")
public void verifyHeaders(String respString) throws Exception {
+ System.out.println("\ntesting:\n\t" + respString
+ .replace("\r\n", "<CRLF>")
+ .replace("\r", "<CR>")
+ .replace("\n","<LF>")
+ .replace("LF>", "LF>\n\t"));
byte[] bytes = respString.getBytes(US_ASCII);
ByteArrayInputStream bais = new ByteArrayInputStream(bytes);
MessageHeader m = new MessageHeader(bais);
Map<String,List<String>> messageHeaderMap = m.getHeaders();
- int available = bais.available();
+ int availableBytes = bais.available();
Http1HeaderParser decoder = new Http1HeaderParser();
ByteBuffer b = ByteBuffer.wrap(bytes);
decoder.parse(b);
+ System.out.printf("Http1HeaderParser parsed %d bytes out of %d%n", b.position(), bytes.length);
Map<String,List<String>> decoderMap1 = decoder.headers().map();
- assertEquals(available, b.remaining(),
- "stream available not equal to remaining");
// assert status-line
String statusLine1 = messageHeaderMap.get(null).get(0);
@@ -273,6 +343,9 @@
assertHeadersEqual(messageHeaderMap, decoderMap1,
"messageHeaderMap not equal to decoderMap1");
+ assertEquals(availableBytes, b.remaining(),
+ String.format("stream available (%d) not equal to remaining (%d)",
+ availableBytes, b.remaining()));
// byte at a time
decoder = new Http1HeaderParser();
List<ByteBuffer> buffers = IntStream.range(0, bytes.length)
@@ -280,9 +353,10 @@
.collect(toList());
while (decoder.parse(buffers.remove(0)) != true);
Map<String,List<String>> decoderMap2 = decoder.headers().map();
- assertEquals(available, buffers.size(),
+ assertEquals(availableBytes, buffers.size(),
"stream available not equals to remaining buffers");
assertEquals(decoderMap1, decoderMap2, "decoder maps not equal");
+
}
@DataProvider(name = "errors")