From ab5461d73e77a7edcb450bbceeb911f9a6474bed Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Tue, 10 Jun 2014 12:55:21 +0200 Subject: [PATCH] fixed hpack literal encoding bug --- .../jetty/http2/hpack/HpackContext.java | 8 ++- .../jetty/http2/hpack/HpackEncoder.java | 32 +++++++-- .../eclipse/jetty/http2/hpack/MetaData.java | 69 +++++++++++++++++++ .../jetty/http2/hpack/HpackContextTest.java | 15 ++++ .../jetty/http2/hpack/HpackEncoderTest.java | 1 - .../eclipse/jetty/http2/hpack/HpackTest.java | 55 ++++++++++++++- 6 files changed, 171 insertions(+), 9 deletions(-) diff --git a/jetty-http2/http2-hpack/src/main/java/org/eclipse/jetty/http2/hpack/HpackContext.java b/jetty-http2/http2-hpack/src/main/java/org/eclipse/jetty/http2/hpack/HpackContext.java index 327033a4d30..69f10a7e1ce 100644 --- a/jetty-http2/http2-hpack/src/main/java/org/eclipse/jetty/http2/hpack/HpackContext.java +++ b/jetty-http2/http2-hpack/src/main/java/org/eclipse/jetty/http2/hpack/HpackContext.java @@ -30,6 +30,7 @@ import org.eclipse.jetty.http.HttpField; import org.eclipse.jetty.http.HttpMethod; import org.eclipse.jetty.http.HttpScheme; import org.eclipse.jetty.util.ArrayQueue; +import org.eclipse.jetty.util.ArrayTernaryTrie; import org.eclipse.jetty.util.ArrayTrie; import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.Trie; @@ -104,7 +105,7 @@ public class HpackContext }; private static final Map __staticFieldMap = new HashMap<>(); - private static final Trie __staticNameMap = new ArrayTrie<>(24); + private static final Trie __staticNameMap = new ArrayTernaryTrie<>(true,512); private static final Entry[] __staticTable=new Entry[STATIC_TABLE.length]; static @@ -140,15 +141,18 @@ public class HpackContext default: entry=new StaticEntry(i,new HttpField(STATIC_TABLE[i][0],STATIC_TABLE[i][1])); } - + __staticTable[i]=entry; if (entry._field.getValue()!=null) __staticFieldMap.put(entry._field,entry); + if (!added.contains(entry._field.getName())) { added.add(entry._field.getName()); __staticNameMap.put(entry._field.getName(),entry); + if (__staticNameMap.get(entry._field.getName())==null) + throw new IllegalStateException("name trie too small"); } } } diff --git a/jetty-http2/http2-hpack/src/main/java/org/eclipse/jetty/http2/hpack/HpackEncoder.java b/jetty-http2/http2-hpack/src/main/java/org/eclipse/jetty/http2/hpack/HpackEncoder.java index 6996d234ee5..7e513e8f076 100644 --- a/jetty-http2/http2-hpack/src/main/java/org/eclipse/jetty/http2/hpack/HpackEncoder.java +++ b/jetty-http2/http2-hpack/src/main/java/org/eclipse/jetty/http2/hpack/HpackEncoder.java @@ -27,6 +27,8 @@ import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http2.hpack.HpackContext.Entry; import org.eclipse.jetty.io.ByteBufferPool.Lease; +import org.eclipse.jetty.util.BufferUtil; +import org.eclipse.jetty.util.TypeUtil; public class HpackEncoder { @@ -130,6 +132,13 @@ public class HpackEncoder private void encode(ByteBuffer buffer, HttpField field) { + + /* + System.err.println("encode "+field); + int p=buffer.position(); + try{ + */ + // TODO currently we do not check if there is enough space, so we will always // return true or fail nastily. @@ -142,6 +151,7 @@ public class HpackEncoder if (entry.isInReferenceSet()) { entry.used(); + // System.err.println("In Reference Set"); return; } @@ -163,8 +173,11 @@ public class HpackEncoder // Add the value buffer.put(entry.getStaticHuffmanValue()); + // System.err.println("Literal without Indexing, indexed name"); return; } + + // System.err.println("Indexed from Header Set"); // So we can add the entry to the reference Set and emit the index; _context.addToRefSet(entry); @@ -200,7 +213,7 @@ public class HpackEncoder { reference=true; never_index=false; - huffman=__DO_NOT_HUFFMAN.contains(header); + huffman=!__DO_NOT_HUFFMAN.contains(header); name_bits = 6; mask=(byte)0x40; } @@ -208,17 +221,19 @@ public class HpackEncoder { reference=false; never_index=__NEVER_INDEX.contains(header); - huffman=__DO_NOT_HUFFMAN.contains(header); + huffman=!__DO_NOT_HUFFMAN.contains(header); name_bits = 4; mask=never_index?(byte)0x01:(byte)0x00; } - // Add the mask bits buffer.put(mask); // Look for a name Index Entry name_entry = _context.get(field.getName()); + + // System.err.printf("Literal huff=%b refed=%b neverIdx=%b nameIdx=%b b=%d m=0x%02x%n",huffman,reference,never_index,name_entry!=null,name_bits,mask); + if (name_entry!=null) NBitInteger.encode(buffer,name_bits,_context.index(name_entry)); else @@ -241,7 +256,7 @@ public class HpackEncoder else { // add literal assuming iso_8859_1 - buffer.put((byte)0x80); + buffer.put((byte)0x00); NBitInteger.encode(buffer,7,value.length()); for (int i=0;i return list; } + @Override + public boolean equals(Object o) + { + if (!(o instanceof MetaData)) + return false; + MetaData m = (MetaData)o; + + List lm=m.getFields(); + int s=0; + for (HttpField field: this) + { + s++; + if (!lm.contains(field)) + return false; + } + + if (s!=lm.size()) + return false; + + return true; + } + + @Override + public String toString() + { + StringBuilder out = new StringBuilder(); + for (HttpField field: this) + out.append(field).append('\n'); + return out.toString(); + } + /* -------------------------------------------------------- */ /* -------------------------------------------------------- */ @@ -132,6 +164,27 @@ public class MetaData implements Iterable { return _path; } + + @Override + public boolean equals(Object o) + { + if (!(o instanceof Request)) + return false; + Request r = (Request)o; + if (!_method.equals(r._method) || + !_scheme.equals(r._scheme) || + !_authority.equals(r._authority) || + !_path.equals(r._path)) + return false; + return super.equals(o); + } + + @Override + public String toString() + { + return _method+" "+_scheme+"://"+_authority+_path+" HTTP/2\n"+super.toString(); + } + } /* -------------------------------------------------------- */ @@ -169,5 +222,21 @@ public class MetaData implements Iterable { return _status; } + + @Override + public boolean equals(Object o) + { + if (!(o instanceof Response)) + return false; + Response r = (Response)o; + if (_status!=r._status) + return false; + return super.equals(o); + } + @Override + public String toString() + { + return "HTTP/2 "+_status+"\n"+super.toString(); + } } } diff --git a/jetty-http2/http2-hpack/src/test/java/org/eclipse/jetty/http2/hpack/HpackContextTest.java b/jetty-http2/http2-hpack/src/test/java/org/eclipse/jetty/http2/hpack/HpackContextTest.java index 138980ded20..8c9a978a2ad 100644 --- a/jetty-http2/http2-hpack/src/test/java/org/eclipse/jetty/http2/hpack/HpackContextTest.java +++ b/jetty-http2/http2-hpack/src/test/java/org/eclipse/jetty/http2/hpack/HpackContextTest.java @@ -686,4 +686,19 @@ public class HpackContextTest } } + + + + @Test + public void testNameInsensitivity() + { + HpackContext ctx = new HpackContext(4096); + assertEquals("content-length",ctx.get("content-length").getHttpField().getName()); + assertEquals("content-length",ctx.get("Content-Length").getHttpField().getName()); + + ctx.add(new HttpField("Wibble","Wobble")); + assertEquals("Wibble",ctx.get("wibble").getHttpField().getName()); + assertEquals("Wibble",ctx.get("Wibble").getHttpField().getName()); + + } } diff --git a/jetty-http2/http2-hpack/src/test/java/org/eclipse/jetty/http2/hpack/HpackEncoderTest.java b/jetty-http2/http2-hpack/src/test/java/org/eclipse/jetty/http2/hpack/HpackEncoderTest.java index 082f588901d..7dee4b407d3 100644 --- a/jetty-http2/http2-hpack/src/test/java/org/eclipse/jetty/http2/hpack/HpackEncoderTest.java +++ b/jetty-http2/http2-hpack/src/test/java/org/eclipse/jetty/http2/hpack/HpackEncoderTest.java @@ -267,7 +267,6 @@ public class HpackEncoderTest } - diff --git a/jetty-http2/http2-hpack/src/test/java/org/eclipse/jetty/http2/hpack/HpackTest.java b/jetty-http2/http2-hpack/src/test/java/org/eclipse/jetty/http2/hpack/HpackTest.java index da75ce2a1ff..ab5937e87d9 100644 --- a/jetty-http2/http2-hpack/src/test/java/org/eclipse/jetty/http2/hpack/HpackTest.java +++ b/jetty-http2/http2-hpack/src/test/java/org/eclipse/jetty/http2/hpack/HpackTest.java @@ -19,16 +19,67 @@ package org.eclipse.jetty.http2.hpack; +import java.nio.ByteBuffer; + import org.eclipse.jetty.http.HttpFields; +import org.eclipse.jetty.http.HttpHeader; +import org.junit.Assert; import org.junit.Test; +import org.eclipse.jetty.http2.hpack.MetaData.Response; +import org.eclipse.jetty.http2.hpack.MetaData.Request; +import org.eclipse.jetty.util.BufferUtil; public class HpackTest { @Test - public void encodeDecodeTest() + public void encodeDecodeResponseTest() { - HttpFields fields = new HttpFields(); + HpackEncoder encoder = new HpackEncoder(); + HpackDecoder decoder = new HpackDecoder(); + ByteBuffer buffer = BufferUtil.allocate(16*1024); + + HttpFields fields0 = new HttpFields(); + fields0.add(HttpHeader.CONTENT_TYPE,"text/html"); + fields0.add(HttpHeader.CONTENT_LENGTH,"1024"); + fields0.add(HttpHeader.SERVER,"jetty"); + fields0.add(HttpHeader.SET_COOKIE,"abcdefghijklmnopqrstuvwxyz"); + fields0.add("custom-key","custom-value"); + Response original0 = new Response(200,fields0); + + BufferUtil.clearToFill(buffer); + encoder.encode(buffer,original0); + BufferUtil.flipToFlush(buffer,0); + Response decoded0 = (Response)decoder.decode(buffer); + + System.err.println(decoded0); + Assert.assertEquals(original0,decoded0); + + // Same again? + BufferUtil.clearToFill(buffer); + encoder.encode(buffer,original0); + BufferUtil.flipToFlush(buffer,0); + Response decoded0b = (Response)decoder.decode(buffer); + + System.err.println(decoded0b); + Assert.assertEquals(original0,decoded0b); + + + HttpFields fields1 = new HttpFields(); + fields1.add(HttpHeader.CONTENT_TYPE,"text/plain"); + fields1.add(HttpHeader.CONTENT_LENGTH,"1234"); + fields1.add(HttpHeader.SERVER,"jetty"); + fields1.add("custom-key","other-value"); + Response original1 = new Response(200,fields1); + + // Same again? + BufferUtil.clearToFill(buffer); + encoder.encode(buffer,original1); + BufferUtil.flipToFlush(buffer,0); + Response decoded1 = (Response)decoder.decode(buffer); + + System.err.println(decoded1); + Assert.assertEquals(original1,decoded1); }