changeset 39315:355b087366f0

8160218: HPack decoder fails when processing header in multiple ByteBuffers Reviewed-by: michaelm
author prappo
date Wed, 29 Jun 2016 10:19:48 +0100
parents 779f1d11a746
children 3042646f9b7b 3c1532acb5e0
files jdk/src/java.httpclient/share/classes/java/net/http/Http2Connection.java jdk/src/java.httpclient/share/classes/sun/net/httpclient/hpack/StringReader.java jdk/test/java/net/httpclient/http2/java.httpclient/sun/net/httpclient/hpack/DecoderTest.java jdk/test/java/net/httpclient/http2/java.httpclient/sun/net/httpclient/hpack/EncoderTest.java
diffstat 4 files changed, 168 insertions(+), 3 deletions(-) [+]
line wrap: on
line diff
--- a/jdk/src/java.httpclient/share/classes/java/net/http/Http2Connection.java	Wed Jun 29 10:07:15 2016 +0200
+++ b/jdk/src/java.httpclient/share/classes/java/net/http/Http2Connection.java	Wed Jun 29 10:19:48 2016 +0100
@@ -298,7 +298,7 @@
 
         ByteBuffer[] buffers = frame.getHeaderBlock();
         for (int i = 0; i < buffers.length; i++) {
-            hpackIn.decode(buffers[i], endOfHeaders, decoder);
+            hpackIn.decode(buffers[i], endOfHeaders && (i == buffers.length - 1), decoder);
         }
     }
 
--- a/jdk/src/java.httpclient/share/classes/sun/net/httpclient/hpack/StringReader.java	Wed Jun 29 10:07:15 2016 +0200
+++ b/jdk/src/java.httpclient/share/classes/sun/net/httpclient/hpack/StringReader.java	Wed Jun 29 10:19:48 2016 +0100
@@ -78,6 +78,7 @@
             if (isLast) {
                 input.limit(input.position() + remainingLength);
             }
+            remainingLength -= Math.min(input.remaining(), remainingLength);
             if (huffman) {
                 huffmanReader.read(input, output, isLast);
             } else {
@@ -85,6 +86,7 @@
             }
             if (isLast) {
                 input.limit(oldLimit);
+                state = DONE;
             }
             return isLast;
         }
--- a/jdk/test/java/net/httpclient/http2/java.httpclient/sun/net/httpclient/hpack/DecoderTest.java	Wed Jun 29 10:07:15 2016 +0200
+++ b/jdk/test/java/net/httpclient/http2/java.httpclient/sun/net/httpclient/hpack/DecoderTest.java	Wed Jun 29 10:19:48 2016 +0100
@@ -27,14 +27,20 @@
 import java.io.UncheckedIOException;
 import java.net.ProtocolException;
 import java.nio.ByteBuffer;
+import java.util.Iterator;
 import java.util.LinkedList;
 import java.util.List;
+import java.util.function.Supplier;
 import java.util.stream.Collectors;
 
 import static org.testng.Assert.assertEquals;
 import static org.testng.Assert.assertNotNull;
 import static sun.net.httpclient.hpack.TestHelper.*;
 
+//
+// Tests whose names start with "testX" are the ones captured from real HPACK
+// use cases
+//
 public final class DecoderTest {
 
     //
@@ -138,6 +144,23 @@
         // @formatter:on
     }
 
+    @Test
+    public void example5AllSplits() {
+        // @formatter:off
+        testAllSplits(
+                "8286 8441 0f77 7777 2e65 7861 6d70 6c65\n" +
+                "2e63 6f6d",
+
+                "[  1] (s =  57) :authority: www.example.com\n" +
+                "      Table size:  57",
+
+                ":method: GET\n" +
+                ":scheme: http\n" +
+                ":path: /\n" +
+                ":authority: www.example.com");
+        // @formatter:on
+    }
+
     //
     // http://tools.ietf.org/html/rfc7541#appendix-C.4
     //
@@ -334,6 +357,45 @@
         // @formatter:on
     }
 
+    @Test
+    public void testX1() {
+        // Supplier of a decoder with a particular state
+        Supplier<Decoder> s = () -> {
+            Decoder d = new Decoder(4096);
+            // @formatter:off
+            test(d, "88 76 92 ca 54 a7 d7 f4 fa ec af ed 6d da 61 d7 bb 1e ad ff" +
+                    "df 61 97 c3 61 be 94 13 4a 65 b6 a5 04 00 b8 a0 5a b8 db 77" +
+                    "1b 71 4c 5a 37 ff 0f 0d 84 08 00 00 03",
+
+                    "[  1] (s =  65) date: Fri, 24 Jun 2016 14:55:56 GMT\n" +
+                    "[  2] (s =  59) server: Jetty(9.3.z-SNAPSHOT)\n" +
+                    "      Table size: 124",
+
+                    ":status: 200\n" +
+                    "server: Jetty(9.3.z-SNAPSHOT)\n" +
+                    "date: Fri, 24 Jun 2016 14:55:56 GMT\n" +
+                    "content-length: 100000"
+            );
+            // @formatter:on
+            return d;
+        };
+        // For all splits of the following data fed to the supplied decoder we
+        // must get what's expected
+        // @formatter:off
+        testAllSplits(s,
+                "88 bf be 0f 0d 84 08 00 00 03",
+
+                "[  1] (s =  65) date: Fri, 24 Jun 2016 14:55:56 GMT\n" +
+                "[  2] (s =  59) server: Jetty(9.3.z-SNAPSHOT)\n" +
+                "      Table size: 124",
+
+                ":status: 200\n" +
+                "server: Jetty(9.3.z-SNAPSHOT)\n" +
+                "date: Fri, 24 Jun 2016 14:55:56 GMT\n" +
+                "content-length: 100000");
+        // @formatter:on
+    }
+
     //
     // This test is missing in the spec
     //
@@ -567,6 +629,38 @@
         test(new Decoder(4096), hexdump, headerTable, headerList);
     }
 
+    private static void testAllSplits(String hexdump,
+                                      String expectedHeaderTable,
+                                      String expectedHeaderList) {
+        testAllSplits(() -> new Decoder(256), hexdump, expectedHeaderTable, expectedHeaderList);
+    }
+
+    private static void testAllSplits(Supplier<Decoder> supplier, String hexdump,
+                                      String expectedHeaderTable, String expectedHeaderList) {
+        ByteBuffer source = SpecHelper.toBytes(hexdump);
+
+        BuffersTestingKit.forEachSplit(source, iterable -> {
+            List<String> actual = new LinkedList<>();
+            Iterator<? extends ByteBuffer> i = iterable.iterator();
+            if (!i.hasNext()) {
+                return;
+            }
+            Decoder d = supplier.get();
+            do {
+                ByteBuffer n = i.next();
+                d.decode(n, !i.hasNext(), (name, value) -> {
+                    if (value == null) {
+                        actual.add(name.toString());
+                    } else {
+                        actual.add(name + ": " + value);
+                    }
+                });
+            } while (i.hasNext());
+            assertEquals(d.getTable().getStateString(), expectedHeaderTable);
+            assertEquals(actual.stream().collect(Collectors.joining("\n")), expectedHeaderList);
+        });
+    }
+
     //
     // Sometimes we need to keep the same decoder along several runs,
     // as it models the same connection
--- a/jdk/test/java/net/httpclient/http2/java.httpclient/sun/net/httpclient/hpack/EncoderTest.java	Wed Jun 29 10:07:15 2016 +0200
+++ b/jdk/test/java/net/httpclient/http2/java.httpclient/sun/net/httpclient/hpack/EncoderTest.java	Wed Jun 29 10:19:48 2016 +0100
@@ -24,17 +24,23 @@
 
 import org.testng.annotations.Test;
 
+import java.nio.Buffer;
 import java.nio.ByteBuffer;
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.Iterator;
+import java.util.LinkedList;
 import java.util.List;
+import java.util.function.Consumer;
 import java.util.function.Function;
 
+import static sun.net.httpclient.hpack.BuffersTestingKit.concat;
+import static sun.net.httpclient.hpack.BuffersTestingKit.forEachSplit;
+import static sun.net.httpclient.hpack.SpecHelper.toHexdump;
+import static sun.net.httpclient.hpack.TestHelper.assertVoidThrows;
 import static java.util.Arrays.asList;
 import static org.testng.Assert.assertEquals;
 import static org.testng.Assert.assertTrue;
-import static sun.net.httpclient.hpack.SpecHelper.toHexdump;
-import static sun.net.httpclient.hpack.TestHelper.assertVoidThrows;
 
 // TODO: map textual representation of commands from the spec to actual
 // calls to encoder (actually, this is a good idea for decoder as well)
@@ -198,6 +204,61 @@
         // @formatter:on
     }
 
+    @Test
+    public void example5AllSplits() {
+
+        List<Consumer<Encoder>> actions = new LinkedList<>();
+        actions.add(e -> e.indexed(2));
+        actions.add(e -> e.indexed(6));
+        actions.add(e -> e.indexed(4));
+        actions.add(e -> e.literalWithIndexing(1, "www.example.com", false));
+
+        encodeAllSplits(
+                actions,
+
+                "8286 8441 0f77 7777 2e65 7861 6d70 6c65\n" +
+                "2e63 6f6d",
+
+                "[  1] (s =  57) :authority: www.example.com\n" +
+                "      Table size:  57");
+    }
+
+    private static void encodeAllSplits(Iterable<Consumer<Encoder>> consumers,
+                                        String expectedHexdump,
+                                        String expectedTableState) {
+        ByteBuffer buffer = SpecHelper.toBytes(expectedHexdump);
+        erase(buffer); // Zeroed buffer of size needed to hold the encoding
+        forEachSplit(buffer, iterable -> {
+            List<ByteBuffer> copy = new LinkedList<>();
+            iterable.forEach(b -> copy.add(ByteBuffer.allocate(b.remaining())));
+            Iterator<ByteBuffer> output = copy.iterator();
+            if (!output.hasNext()) {
+                throw new IllegalStateException("No buffers to encode to");
+            }
+            Encoder e = newCustomEncoder(256); // FIXME: pull up (as a parameter)
+            drainInitialUpdate(e);
+            boolean encoded;
+            ByteBuffer b = output.next();
+            for (Consumer<Encoder> c : consumers) {
+                c.accept(e);
+                do {
+                    encoded = e.encode(b);
+                    if (!encoded) {
+                        if (output.hasNext()) {
+                            b = output.next();
+                        } else {
+                            throw new IllegalStateException("No room for encoding");
+                        }
+                    }
+                }
+                while (!encoded);
+            }
+            copy.forEach(Buffer::flip);
+            ByteBuffer data = concat(copy);
+            test(e, data, expectedHexdump, expectedTableState);
+        });
+    }
+
     //
     // http://tools.ietf.org/html/rfc7541#appendix-C.4
     //
@@ -620,4 +681,12 @@
             b.flip();
         } while (!done);
     }
+
+    private static void erase(ByteBuffer buffer) {
+        buffer.clear();
+        while (buffer.hasRemaining()) {
+            buffer.put((byte) 0);
+        }
+        buffer.clear();
+    }
 }