http-client-branch: HttpClient HTTP/1.1 could be more lenient and accept LF as well as CRLF as header separator. http-client-branch
authordfuchs
Thu, 24 May 2018 11:31:50 +0100
branchhttp-client-branch
changeset 56601 e38ce7e04995
parent 56598 4c502e3991bf
child 56602 f2917c858701
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
src/java.net.http/share/classes/jdk/internal/net/http/Http1HeaderParser.java
test/jdk/java/net/httpclient/whitebox/java.net.http/jdk/internal/net/http/Http1HeaderParserTest.java
--- 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")