diff --git a/codec/src/main/java/io/netty/handler/codec/DefaultHeaders.java b/codec/src/main/java/io/netty/handler/codec/DefaultHeaders.java index f246c0324d..ead92ccb0a 100644 --- a/codec/src/main/java/io/netty/handler/codec/DefaultHeaders.java +++ b/codec/src/main/java/io/netty/handler/codec/DefaultHeaders.java @@ -1012,12 +1012,22 @@ public class DefaultHeaders> implements Headers return value; } - private HeaderEntry remove0(HeaderEntry entry, HeaderEntry previous) { + HeaderEntry remove0(HeaderEntry entry, HeaderEntry previous) { int i = index(entry.hash); - HeaderEntry e = entries[i]; - if (e == entry) { + HeaderEntry firstEntry = entries[i]; + if (firstEntry == entry) { entries[i] = entry.next; previous = entries[i]; + } else if (previous == null) { + // If we don't have any existing starting point, then start from the beginning. + previous = firstEntry; + HeaderEntry next = firstEntry.next; + while (next != null && next != entry) { + previous = next; + next = next.next; + } + assert next != null: "Entry not found in its hash bucket: " + entry; + previous.next = entry.next; } else { previous.next = entry.next; } diff --git a/codec/src/test/java/io/netty/handler/codec/DefaultHeadersTest.java b/codec/src/test/java/io/netty/handler/codec/DefaultHeadersTest.java index 7064d28e48..9dee841eb4 100644 --- a/codec/src/test/java/io/netty/handler/codec/DefaultHeadersTest.java +++ b/codec/src/test/java/io/netty/handler/codec/DefaultHeadersTest.java @@ -15,6 +15,7 @@ package io.netty.handler.codec; import io.netty.util.AsciiString; +import io.netty.util.HashingStrategy; import org.junit.Test; import java.util.ArrayList; @@ -26,6 +27,9 @@ import java.util.NoSuchElementException; import static io.netty.util.AsciiString.of; import static java.util.Arrays.asList; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.hasSize; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotEquals; @@ -48,6 +52,10 @@ public class DefaultHeadersTest { TestDefaultHeaders(ValueConverter converter) { super(converter); } + + TestDefaultHeaders(HashingStrategy nameHashingStrategy) { + super(nameHashingStrategy, CharSequenceValueConverter.INSTANCE); + } } private static TestDefaultHeaders newInstance() { @@ -759,4 +767,50 @@ public class DefaultHeadersTest { assertTrue(headers.getBoolean("name2", false)); assertTrue(headers.getBoolean("name3", false)); } + + @Test + public void handlingOfHeaderNameHashCollisions() { + TestDefaultHeaders headers = new TestDefaultHeaders(new HashingStrategy() { + @Override + public int hashCode(CharSequence obj) { + return 0; // Degenerate hashing strategy to enforce collisions. + } + + @Override + public boolean equals(CharSequence a, CharSequence b) { + return a.equals(b); + } + }); + + headers.add("Cookie", "a=b; c=d; e=f"); + headers.add("other", "text/plain"); // Add another header which will be saved in the same entries[index] + + simulateCookieSplitting(headers); + List cookies = headers.getAll("Cookie"); + + assertThat(cookies, hasSize(3)); + assertThat(cookies, containsInAnyOrder((CharSequence) "a=b", "c=d", "e=f")); + } + + /** + * Split up cookies into individual cookie crumb headers. + */ + static void simulateCookieSplitting(TestDefaultHeaders headers) { + Iterator cookieItr = headers.valueIterator("Cookie"); + if (!cookieItr.hasNext()) { + return; + } + // We want to avoid "concurrent modifications" of the headers while we are iterating. So we insert crumbs + // into an intermediate collection and insert them after the split process concludes. + List cookiesToAdd = new ArrayList(); + while (cookieItr.hasNext()) { + //noinspection DynamicRegexReplaceableByCompiledPattern + String[] cookies = cookieItr.next().toString().split("; "); + cookiesToAdd.addAll(asList(cookies)); + cookieItr.remove(); + } + for (CharSequence crumb : cookiesToAdd) { + headers.add("Cookie", crumb); + } + } }