From c497b61917a4e4fd09bb098bd264bf06e62db317 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Mon, 11 May 2020 13:57:17 +0200 Subject: [PATCH 1/8] Issue #4860 NPE from HttpFields Paranoid catch if sending and exception page throws an exception. Signed-off-by: Greg Wilkins --- .../java/org/eclipse/jetty/server/HttpChannel.java | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java index de21a050398..a1a14623f6e 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java @@ -432,8 +432,17 @@ public class HttpChannel implements Runnable, HttpOutput.Interceptor abort(x); else { - _response.resetContent(); - sendResponseAndComplete(); + try + { + _response.resetContent(); + sendResponseAndComplete(); + } + catch (Throwable t) + { + if (x != t) + x.addSuppressed(t); + abort(x); + } } } finally From 6af6af6419b43fce0c7c291579677681f901b676 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Mon, 11 May 2020 13:58:27 +0200 Subject: [PATCH 2/8] Issue #4860 NPE from HttpFields Improved test coverage Signed-off-by: Greg Wilkins --- .../eclipse/jetty/http/HttpFieldsTest.java | 235 ++++++++++++------ 1 file changed, 163 insertions(+), 72 deletions(-) diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpFieldsTest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpFieldsTest.java index 7175f00dc16..2ce6124642e 100644 --- a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpFieldsTest.java +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpFieldsTest.java @@ -19,13 +19,13 @@ package org.eclipse.jetty.http; import java.nio.ByteBuffer; -import java.util.Collections; import java.util.Enumeration; import java.util.Iterator; import java.util.List; import java.util.ListIterator; import java.util.Locale; import java.util.NoSuchElementException; +import java.util.stream.Collectors; import org.eclipse.jetty.util.BufferUtil; import org.hamcrest.Matchers; @@ -34,6 +34,7 @@ import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.is; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -49,30 +50,25 @@ public class HttpFieldsTest { HttpFields header = new HttpFields(); - header.put("name0", "value:0"); - header.put("name1", "value1"); + header.add("name0", "wrong"); + header.add(HttpHeader.ACCEPT, "nothing"); + header.add("name0", "still wrong"); + header.add(HttpHeader.ACCEPT, "money"); + + header.put("name0", "value0"); + header.put(HttpHeader.ACCEPT, "praise"); assertEquals(2, header.size()); - assertEquals("value:0", header.get("name0")); - assertEquals("value1", header.get("name1")); + assertEquals("value0", header.get("name0")); + assertEquals("praise", header.get("accept")); assertNull(header.get("name2")); - int matches = 0; - Enumeration e = header.getFieldNames(); - while (e.hasMoreElements()) - { - Object o = e.nextElement(); - if ("name0".equals(o)) - matches++; - if ("name1".equals(o)) - matches++; - } - assertEquals(2, matches); + header.add("name0", "wrong"); + header.add(HttpHeader.ACCEPT, "nothing"); - e = header.getValues("name0"); - assertEquals(true, e.hasMoreElements()); - assertEquals(e.nextElement(), "value:0"); - assertEquals(false, e.hasMoreElements()); + header.put("name0", (String)null); + header.put(HttpHeader.ACCEPT, (String)null); + assertEquals(0, header.size()); } @Test @@ -124,6 +120,29 @@ public class HttpFieldsTest }); } + @Test + public void testGetValuesList() throws Exception + { + HttpFields header = new HttpFields(); + + header.add("name0", "value0"); + header.add("name1", "value1a"); + header.add(HttpHeader.ACCEPT, "something"); + header.add("name2", "value2"); + header.add("name1", "value1b"); + header.add(HttpHeader.ACCEPT, "everything"); + + assertThat(header.getValuesList("unknown").size(), is(0)); + assertThat(header.getValuesList(HttpHeader.CONNECTION).size(), is(0)); + assertThat(header.getValuesList("name0"), contains("value0")); + assertThat(header.getValuesList("name1"), contains("value1a", "value1b")); + assertThat(header.getValuesList(HttpHeader.ACCEPT), contains("something", "everything")); + + assertThat(header.getFields(HttpHeader.CONNECTION).size(), is(0)); + assertThat(header.getFields(HttpHeader.ACCEPT).stream().map(HttpField::getValue).collect(Collectors.toList()), + contains("something", "everything")); + } + @Test public void testGetKnown() throws Exception { @@ -266,39 +285,47 @@ public class HttpFieldsTest fields.add("name0", "value0"); fields.add("name1", "valueA"); - fields.add("name2", "value2"); + fields.add(HttpHeader.ACCEPT, "everything"); assertEquals("value0", fields.get("name0")); assertEquals("valueA", fields.get("name1")); - assertEquals("value2", fields.get("name2")); + assertEquals("everything", fields.get("accept")); fields.add("name1", "valueB"); + fields.add(HttpHeader.ACCEPT, "nothing"); + fields.add("name1", null); + // fields.add(HttpHeader.ACCEPT, (String)null); TODO this one throws IAE. Should make the same as the others. + + fields.add("name2", "value2"); assertEquals("value0", fields.get("name0")); assertEquals("valueA", fields.get("name1")); assertEquals("value2", fields.get("name2")); assertNull(fields.get("name3")); - int matches = 0; - Enumeration e = fields.getFieldNames(); - while (e.hasMoreElements()) - { - Object o = e.nextElement(); - if ("name0".equals(o)) - matches++; - if ("name1".equals(o)) - matches++; - if ("name2".equals(o)) - matches++; - } - assertEquals(3, matches); + assertThat(fields.getValuesList("name1"), contains("valueA", "valueB")); + assertThat(fields.getValuesList(HttpHeader.ACCEPT), contains("everything", "nothing")); - e = fields.getValues("name1"); - assertEquals(true, e.hasMoreElements()); - assertEquals(e.nextElement(), "valueA"); - assertEquals(true, e.hasMoreElements()); - assertEquals(e.nextElement(), "valueB"); - assertEquals(false, e.hasMoreElements()); + fields.add(HttpHeader.CONNECTION, HttpHeaderValue.CLOSE); + fields.add(HttpHeader.CONNECTION, (HttpHeaderValue)null); + assertThat(fields.getValuesList("Connection"), contains("close")); + } + + @Test + public void testAddAll() throws Exception + { + HttpFields fields0 = new HttpFields(); + assertThat(fields0.size(), is(0)); + HttpFields fields1 = new HttpFields(fields0); + assertThat(fields1.size(), is(0)); + + fields0.add("name0", "value0"); + fields0.add("name1", "valueA"); + fields0.add("name2", "value2"); + + fields1.addAll(fields0); + assertThat(fields1.size(), is(3)); + assertThat(fields0, is(fields1)); } @Test @@ -381,44 +408,44 @@ public class HttpFieldsTest assertEquals(false, e.hasMoreElements()); } + @Test + public void testAddCSV() throws Exception + { + HttpFields fields = new HttpFields(); + fields.addCSV(HttpHeader.CONNECTION); + fields.addCSV("name"); + assertThat(fields.size(), is(0)); + + fields.addCSV(HttpHeader.CONNECTION, "one"); + fields.addCSV("name", "one"); + assertThat(fields.getValuesList("name"), contains("one")); + assertThat(fields.getValuesList(HttpHeader.CONNECTION), contains("one")); + + fields.addCSV(HttpHeader.CONNECTION, "two"); + fields.addCSV("name", "two"); + assertThat(fields.getValuesList("name"), contains("one", "two")); + assertThat(fields.getValuesList(HttpHeader.CONNECTION), contains("one", "two")); + + fields.addCSV(HttpHeader.CONNECTION, "one", "three", "four"); + fields.addCSV("name", "one", "three", "four"); + assertThat(fields.getValuesList("name"), contains("one", "two", "three, four")); + assertThat(fields.getValuesList(HttpHeader.CONNECTION), contains("one", "two", "three, four")); + } + @Test public void testGetCSV() throws Exception { HttpFields fields = new HttpFields(); - fields.put("name0", "value0A,value0B"); - fields.add("name0", "value0C,value0D"); + fields.put(HttpHeader.ACCEPT, "valueA, \"value, B\""); + fields.add(HttpHeader.ACCEPT, "\"valueC\",valueD"); fields.put("name1", "value1A, \"value\t, 1B\" "); fields.add("name1", "\"value1C\",\tvalue1D"); - Enumeration e = fields.getValues("name0"); - assertEquals(true, e.hasMoreElements()); - assertEquals(e.nextElement(), "value0A,value0B"); - assertEquals(true, e.hasMoreElements()); - assertEquals(e.nextElement(), "value0C,value0D"); - assertEquals(false, e.hasMoreElements()); - - e = Collections.enumeration(fields.getCSV("name0", false)); - assertEquals(true, e.hasMoreElements()); - assertEquals(e.nextElement(), "value0A"); - assertEquals(true, e.hasMoreElements()); - assertEquals(e.nextElement(), "value0B"); - assertEquals(true, e.hasMoreElements()); - assertEquals(e.nextElement(), "value0C"); - assertEquals(true, e.hasMoreElements()); - assertEquals(e.nextElement(), "value0D"); - assertEquals(false, e.hasMoreElements()); - - e = Collections.enumeration(fields.getCSV("name1", false)); - assertEquals(true, e.hasMoreElements()); - assertEquals(e.nextElement(), "value1A"); - assertEquals(true, e.hasMoreElements()); - assertEquals(e.nextElement(), "value\t, 1B"); - assertEquals(true, e.hasMoreElements()); - assertEquals(e.nextElement(), "value1C"); - assertEquals(true, e.hasMoreElements()); - assertEquals(e.nextElement(), "value1D"); - assertEquals(false, e.hasMoreElements()); + assertThat(fields.getCSV(HttpHeader.ACCEPT, false), contains("valueA", "value, B", "valueC", "valueD")); + assertThat(fields.getCSV(HttpHeader.ACCEPT, true), contains("valueA", "\"value, B\"", "\"valueC\"", "valueD")); + assertThat(fields.getCSV("name1", false), contains("value1A", "value\t, 1B", "value1C", "value1D")); + assertThat(fields.getCSV("name1", true), contains("value1A", "\"value\t, 1B\"", "\"value1C\"", "value1D")); } @Test @@ -659,10 +686,12 @@ public class HttpFieldsTest assertTrue(header.contains(new HttpField("N5", "def"))); assertTrue(header.contains(new HttpField("accept", "abc"))); + assertTrue(header.contains(HttpHeader.ACCEPT)); assertTrue(header.contains(HttpHeader.ACCEPT, "abc")); assertFalse(header.contains(new HttpField("N5", "xyz"))); assertFalse(header.contains(new HttpField("N8", "def"))); assertFalse(header.contains(HttpHeader.ACCEPT, "def")); + assertFalse(header.contains(HttpHeader.AGE)); assertFalse(header.contains(HttpHeader.AGE, "abc")); assertFalse(header.containsKey("n11")); @@ -693,7 +722,7 @@ public class HttpFieldsTest } @Test - public void testPreventNullField() + public void testPreventNullFieldName() { HttpFields fields = new HttpFields(); assertThrows(NullPointerException.class, () -> @@ -703,6 +732,27 @@ public class HttpFieldsTest }); } + @Test + public void testPreventNullField() + { + // Attempt various ways that may have put a null field in the array that + // previously caused a NPE in put. If this test doesn't throw then it passes. + HttpFields fields = new HttpFields(); + fields.add((HttpField)null); + fields.put((HttpField)null); + fields.put("something", "else"); + ListIterator iter = fields.listIterator(); + iter.next(); + iter.set(null); + iter.add(null); + fields.put("something", "other"); + iter = fields.listIterator(); + iter.next(); + iter.remove(); + fields.put("something", "other"); + fields.clear(); + } + @Test public void testIteration() throws Exception { @@ -730,29 +780,58 @@ public class HttpFieldsTest ListIterator l = header.listIterator(); assertThat(l.hasNext(), is(true)); + assertThat(l.nextIndex(), is(0)); + assertThat(l.previousIndex(), is(-1)); + l.add(new HttpField("name0", "value")); assertThat(l.hasNext(), is(true)); + assertThat(l.nextIndex(), is(1)); + assertThat(l.hasPrevious(), is(true)); + assertThat(l.previousIndex(), is(0)); + assertThat(l.next().getName(), is("name1")); + assertThat(l.hasNext(), is(true)); + assertThat(l.nextIndex(), is(2)); + assertThat(l.hasPrevious(), is(true)); + assertThat(l.previousIndex(), is(1)); + l.set(new HttpField("NAME1", "value")); assertThat(l.hasNext(), is(true)); + assertThat(l.nextIndex(), is(2)); assertThat(l.hasPrevious(), is(true)); + assertThat(l.previousIndex(), is(1)); + assertThat(l.previous().getName(), is("NAME1")); assertThat(l.hasNext(), is(true)); + assertThat(l.nextIndex(), is(1)); assertThat(l.hasPrevious(), is(true)); + assertThat(l.previousIndex(), is(0)); + assertThat(l.previous().getName(), is("name0")); assertThat(l.hasNext(), is(true)); + assertThat(l.nextIndex(), is(0)); assertThat(l.hasPrevious(), is(false)); + assertThat(l.previousIndex(), is(-1)); + assertThat(l.next().getName(), is("name0")); assertThat(l.hasNext(), is(true)); + assertThat(l.nextIndex(), is(1)); assertThat(l.hasPrevious(), is(true)); + assertThat(l.previousIndex(), is(0)); + assertThat(l.next().getName(), is("NAME1")); l.add(new HttpField("name2", "value")); assertThat(l.next().getName(), is("name3")); assertThat(l.hasNext(), is(false)); + assertThat(l.nextIndex(), is(4)); assertThat(l.hasPrevious(), is(true)); + assertThat(l.previousIndex(), is(3)); + l.add(new HttpField("name4", "value")); assertThat(l.hasNext(), is(false)); + assertThat(l.nextIndex(), is(5)); assertThat(l.hasPrevious(), is(true)); + assertThat(l.previousIndex(), is(4)); assertThat(l.previous().getName(), is("name4")); i = header.iterator(); @@ -764,4 +843,16 @@ public class HttpFieldsTest assertThat(i.next().getName(), is("name4")); assertThat(i.hasNext(), is(false)); } + + @Test + public void testStream() throws Exception + { + HttpFields header = new HttpFields(); + assertThat(header.stream().count(), is(0L)); + header.put("name1", "valueA"); + header.put("name2", "valueB"); + header.add("name3", "valueC"); + assertThat(header.stream().count(), is(3L)); + assertThat(header.stream().map(HttpField::getName).filter("name2"::equalsIgnoreCase).count(), is(1L)); + } } From 1ac7fe4f9c0726c70102e83fe4e3a93901d9872c Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Mon, 11 May 2020 14:00:04 +0200 Subject: [PATCH 3/8] Issue #4860 NPE from HttpFields + Fix bug with list iterator nextIndex + List iterator cannot inject null fields + minor cleanups Signed-off-by: Greg Wilkins --- .../org/eclipse/jetty/http/HttpFields.java | 32 +++++++++++++------ 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpFields.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpFields.java index 316ea07245a..3787fc627f8 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpFields.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpFields.java @@ -303,14 +303,18 @@ public class HttpFields implements Iterable */ public List getValuesList(String name) { - final List list = new ArrayList<>(); + List list = null; for (int i = 0; i < _size; i++) { HttpField f = _fields[i]; if (f.getName().equalsIgnoreCase(name)) + { + if (list == null) + list = new ArrayList<>(size() - i); list.add(f.getValue()); + } } - return list; + return list == null ? Collections.emptyList() : list; } /** @@ -711,7 +715,8 @@ public class HttpFields implements Iterable public void add(HttpHeader header, HttpHeaderValue value) { - add(header, value.toString()); + if (value != null) + add(header, value.toString()); } /** @@ -966,8 +971,11 @@ public class HttpFields implements Iterable * * @param fields the fields to add */ + @Deprecated public void add(HttpFields fields) { + // TODO this implementation doesn't do what the javadoc says and is really the same + // as addAll, which is renamed to add anyway in 10. if (fields == null) return; @@ -1185,7 +1193,7 @@ public class HttpFields implements Iterable @Override public int nextIndex() { - return _cursor + 1; + return _cursor; } @Override @@ -1199,16 +1207,22 @@ public class HttpFields implements Iterable { if (_current < 0) throw new IllegalStateException(); - _fields[_current] = field; + if (field == null) + remove(); + else + _fields[_current] = field; } @Override public void add(HttpField field) { - _fields = Arrays.copyOf(_fields, _fields.length + 1); - System.arraycopy(_fields, _cursor, _fields, _cursor + 1, _size++); - _fields[_cursor++] = field; - _current = -1; + if (field != null) + { + _fields = Arrays.copyOf(_fields, _fields.length + 1); + System.arraycopy(_fields, _cursor, _fields, _cursor + 1, _size++); + _fields[_cursor++] = field; + _current = -1; + } } } } From 2c7c98f4690d2db399005b48dd42e9d2f5131edd Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Mon, 11 May 2020 10:03:12 -0500 Subject: [PATCH 4/8] Eliminate warnings Signed-off-by: Joakim Erdfelt --- .../eclipse/jetty/http/HttpFieldsTest.java | 138 +++++++----------- 1 file changed, 51 insertions(+), 87 deletions(-) diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpFieldsTest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpFieldsTest.java index 2ce6124642e..e4bf4781d78 100644 --- a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpFieldsTest.java +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpFieldsTest.java @@ -46,7 +46,7 @@ import static org.junit.jupiter.api.Assertions.assertTrue; public class HttpFieldsTest { @Test - public void testPut() throws Exception + public void testPut() { HttpFields header = new HttpFields(); @@ -72,7 +72,7 @@ public class HttpFieldsTest } @Test - public void testPutTo() throws Exception + public void testPutTo() { HttpFields header = new HttpFields(); @@ -93,7 +93,7 @@ public class HttpFieldsTest } @Test - public void testGet() throws Exception + public void testGet() { HttpFields header = new HttpFields(); @@ -104,24 +104,21 @@ public class HttpFieldsTest assertEquals("value0", header.get("Name0")); assertEquals("value1", header.get("name1")); assertEquals("value1", header.get("Name1")); - assertEquals(null, header.get("Name2")); + assertNull(header.get("Name2")); assertEquals("value0", header.getField("name0").getValue()); assertEquals("value0", header.getField("Name0").getValue()); assertEquals("value1", header.getField("name1").getValue()); assertEquals("value1", header.getField("Name1").getValue()); - assertEquals(null, header.getField("Name2")); + assertNull(header.getField("Name2")); assertEquals("value0", header.getField(0).getValue()); assertEquals("value1", header.getField(1).getValue()); - assertThrows(NoSuchElementException.class, () -> - { - header.getField(2); - }); + assertThrows(NoSuchElementException.class, () -> header.getField(2)); } @Test - public void testGetValuesList() throws Exception + public void testGetValuesList() { HttpFields header = new HttpFields(); @@ -144,7 +141,7 @@ public class HttpFieldsTest } @Test - public void testGetKnown() throws Exception + public void testGetKnown() { HttpFields header = new HttpFields(); @@ -157,12 +154,12 @@ public class HttpFieldsTest assertEquals("value0", header.getField(HttpHeader.CONNECTION).getValue()); assertEquals("value1", header.getField(HttpHeader.ACCEPT).getValue()); - assertEquals(null, header.getField(HttpHeader.AGE)); - assertEquals(null, header.get(HttpHeader.AGE)); + assertNull(header.getField(HttpHeader.AGE)); + assertNull(header.get(HttpHeader.AGE)); } @Test - public void testCRLF() throws Exception + public void testCRLF() { HttpFields header = new HttpFields(); @@ -181,7 +178,7 @@ public class HttpFieldsTest } @Test - public void testCachedPut() throws Exception + public void testCachedPut() { HttpFields header = new HttpFields(); @@ -201,7 +198,7 @@ public class HttpFieldsTest } @Test - public void testRePut() throws Exception + public void testRePut() { HttpFields header = new HttpFields(); @@ -235,13 +232,13 @@ public class HttpFieldsTest assertEquals(3, matches); e = header.getValues("name1"); - assertEquals(true, e.hasMoreElements()); + assertTrue(e.hasMoreElements()); assertEquals(e.nextElement(), "value1"); - assertEquals(false, e.hasMoreElements()); + assertFalse(e.hasMoreElements()); } @Test - public void testRemovePut() throws Exception + public void testRemovePut() { HttpFields header = new HttpFields(1); @@ -275,11 +272,11 @@ public class HttpFieldsTest assertEquals(2, matches); e = header.getValues("name1"); - assertEquals(false, e.hasMoreElements()); + assertFalse(e.hasMoreElements()); } @Test - public void testAdd() throws Exception + public void testAdd() { HttpFields fields = new HttpFields(); @@ -312,7 +309,7 @@ public class HttpFieldsTest } @Test - public void testAddAll() throws Exception + public void testAddAll() { HttpFields fields0 = new HttpFields(); assertThat(fields0.size(), is(0)); @@ -369,7 +366,7 @@ public class HttpFieldsTest } @Test - public void testGetValues() throws Exception + public void testGetValues() { HttpFields fields = new HttpFields(); @@ -379,37 +376,39 @@ public class HttpFieldsTest fields.add("name1", "\"value1C\",\tvalue1D"); Enumeration e = fields.getValues("name0"); - assertEquals(true, e.hasMoreElements()); + assertTrue(e.hasMoreElements()); assertEquals(e.nextElement(), "value0A,value0B"); - assertEquals(true, e.hasMoreElements()); + assertTrue(e.hasMoreElements()); assertEquals(e.nextElement(), "value0C,value0D"); - assertEquals(false, e.hasMoreElements()); + assertFalse(e.hasMoreElements()); + //noinspection deprecation e = fields.getValues("name0", ","); - assertEquals(true, e.hasMoreElements()); + assertTrue(e.hasMoreElements()); assertEquals(e.nextElement(), "value0A"); - assertEquals(true, e.hasMoreElements()); + assertTrue(e.hasMoreElements()); assertEquals(e.nextElement(), "value0B"); - assertEquals(true, e.hasMoreElements()); + assertTrue(e.hasMoreElements()); assertEquals(e.nextElement(), "value0C"); - assertEquals(true, e.hasMoreElements()); + assertTrue(e.hasMoreElements()); assertEquals(e.nextElement(), "value0D"); - assertEquals(false, e.hasMoreElements()); + assertFalse(e.hasMoreElements()); + //noinspection deprecation e = fields.getValues("name1", ","); - assertEquals(true, e.hasMoreElements()); + assertTrue(e.hasMoreElements()); assertEquals(e.nextElement(), "value1A"); - assertEquals(true, e.hasMoreElements()); + assertTrue(e.hasMoreElements()); assertEquals(e.nextElement(), "value\t, 1B"); - assertEquals(true, e.hasMoreElements()); + assertTrue(e.hasMoreElements()); assertEquals(e.nextElement(), "value1C"); - assertEquals(true, e.hasMoreElements()); + assertTrue(e.hasMoreElements()); assertEquals(e.nextElement(), "value1D"); - assertEquals(false, e.hasMoreElements()); + assertFalse(e.hasMoreElements()); } @Test - public void testAddCSV() throws Exception + public void testAddCSV() { HttpFields fields = new HttpFields(); fields.addCSV(HttpHeader.CONNECTION); @@ -433,7 +432,7 @@ public class HttpFieldsTest } @Test - public void testGetCSV() throws Exception + public void testGetCSV() { HttpFields fields = new HttpFields(); @@ -449,7 +448,7 @@ public class HttpFieldsTest } @Test - public void testAddQuotedCSV() throws Exception + public void testAddQuotedCSV() { HttpFields fields = new HttpFields(); @@ -491,7 +490,7 @@ public class HttpFieldsTest } @Test - public void testGetQualityCSV() throws Exception + public void testGetQualityCSV() { HttpFields fields = new HttpFields(); @@ -513,7 +512,7 @@ public class HttpFieldsTest } @Test - public void testGetQualityCSVHeader() throws Exception + public void testGetQualityCSVHeader() { HttpFields fields = new HttpFields(); @@ -535,7 +534,7 @@ public class HttpFieldsTest } @Test - public void testDateFields() throws Exception + public void testDateFields() { HttpFields fields = new HttpFields(); @@ -577,7 +576,7 @@ public class HttpFieldsTest } @Test - public void testNegDateFields() throws Exception + public void testNegDateFields() { HttpFields fields = new HttpFields(); @@ -595,7 +594,7 @@ public class HttpFieldsTest } @Test - public void testLongFields() throws Exception + public void testLongFields() { HttpFields header = new HttpFields(); @@ -607,47 +606,12 @@ public class HttpFieldsTest header.put("N2", "xx"); long i1 = header.getLongField("I1"); - try - { - header.getLongField("I2"); - assertTrue(false); - } - catch (NumberFormatException e) - { - assertTrue(true); - } + assertThrows(NumberFormatException.class, () -> header.getLongField("I2")); long i3 = header.getLongField("I3"); - - try - { - header.getLongField("I4"); - assertTrue(false); - } - catch (NumberFormatException e) - { - assertTrue(true); - } - - try - { - header.getLongField("N1"); - assertTrue(false); - } - catch (NumberFormatException e) - { - assertTrue(true); - } - - try - { - header.getLongField("N2"); - assertTrue(false); - } - catch (NumberFormatException e) - { - assertTrue(true); - } + assertThrows(NumberFormatException.class, () -> header.getLongField("I4")); + assertThrows(NumberFormatException.class, () -> header.getLongField("N1")); + assertThrows(NumberFormatException.class, () -> header.getLongField("N2")); assertEquals(42, i1); assertEquals(-44, i3); @@ -659,7 +623,7 @@ public class HttpFieldsTest } @Test - public void testContains() throws Exception + public void testContains() { HttpFields header = new HttpFields(); @@ -739,7 +703,7 @@ public class HttpFieldsTest // previously caused a NPE in put. If this test doesn't throw then it passes. HttpFields fields = new HttpFields(); fields.add((HttpField)null); - fields.put((HttpField)null); + fields.put(null); fields.put("something", "else"); ListIterator iter = fields.listIterator(); iter.next(); @@ -754,7 +718,7 @@ public class HttpFieldsTest } @Test - public void testIteration() throws Exception + public void testIteration() { HttpFields header = new HttpFields(); Iterator i = header.iterator(); @@ -845,7 +809,7 @@ public class HttpFieldsTest } @Test - public void testStream() throws Exception + public void testStream() { HttpFields header = new HttpFields(); assertThat(header.stream().count(), is(0L)); From 90da10dac57c9a4cf5bea85176077e5862c9589b Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Mon, 11 May 2020 10:05:56 -0500 Subject: [PATCH 5/8] Issue #4860 - Improving testPreventNullField testcase Signed-off-by: Joakim Erdfelt --- .../org/eclipse/jetty/http/HttpFieldsTest.java | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpFieldsTest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpFieldsTest.java index e4bf4781d78..d42638e58e6 100644 --- a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpFieldsTest.java +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpFieldsTest.java @@ -700,20 +700,28 @@ public class HttpFieldsTest public void testPreventNullField() { // Attempt various ways that may have put a null field in the array that - // previously caused a NPE in put. If this test doesn't throw then it passes. + // previously caused a NPE in put. HttpFields fields = new HttpFields(); - fields.add((HttpField)null); - fields.put(null); + fields.add((HttpField)null); // should not result in field being added + assertThat(fields.size(), is(0)); + fields.put(null); // should not result in field being added + assertThat(fields.size(), is(0)); fields.put("something", "else"); + assertThat(fields.size(), is(1)); ListIterator iter = fields.listIterator(); iter.next(); iter.set(null); + assertThat(fields.size(), is(0)); iter.add(null); + assertThat(fields.size(), is(0)); fields.put("something", "other"); + assertThat(fields.size(), is(1)); iter = fields.listIterator(); iter.next(); iter.remove(); + assertThat(fields.size(), is(0)); fields.put("something", "other"); + assertThat(fields.size(), is(1)); fields.clear(); } From 6ae75be9bcb7e2059804f1bffcfd18dad44c68d6 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Mon, 11 May 2020 10:34:52 -0500 Subject: [PATCH 6/8] Issue #4860 - Consistency to HttpFields API If a null name (or HttpHeader or HttpField) is used it should throw an ISE + .add() should remain consistent + .put() should remain consistent Signed-off-by: Joakim Erdfelt --- .../org/eclipse/jetty/http/HttpField.java | 2 +- .../org/eclipse/jetty/http/HttpFields.java | 10 +++++ .../org/eclipse/jetty/http/HttpFieldTest.java | 7 +++ .../eclipse/jetty/http/HttpFieldsTest.java | 45 +++++++++++++++++-- 4 files changed, 60 insertions(+), 4 deletions(-) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpField.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpField.java index 0d3853e19fe..e04d7e2345e 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpField.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpField.java @@ -40,7 +40,7 @@ public class HttpField if (_header != null && name == null) _name = _header.asString(); else - _name = Objects.requireNonNull(name); + _name = Objects.requireNonNull(name, "name"); _value = value; } diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpFields.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpFields.java index 3787fc627f8..bd3171a7591 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpFields.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpFields.java @@ -29,6 +29,7 @@ import java.util.List; import java.util.ListIterator; import java.util.Map; import java.util.NoSuchElementException; +import java.util.Objects; import java.util.Set; import java.util.StringTokenizer; import java.util.function.ToIntFunction; @@ -675,6 +676,8 @@ public class HttpFields implements Iterable */ public void put(HttpHeader header, String value) { + Objects.requireNonNull(header, "header"); + if (value == null) remove(header); else @@ -689,7 +692,12 @@ public class HttpFields implements Iterable */ public void put(String name, List list) { + Objects.requireNonNull(name, "name"); + remove(name); + if (list == null) + return; + for (String v : list) { if (v != null) @@ -728,6 +736,8 @@ public class HttpFields implements Iterable */ public void add(HttpHeader header, String value) { + Objects.requireNonNull(header, "header"); + if (value == null) throw new IllegalArgumentException("null value"); diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpFieldTest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpFieldTest.java index ccffa9ca85d..787295a129c 100644 --- a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpFieldTest.java +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpFieldTest.java @@ -28,6 +28,7 @@ import static org.hamcrest.Matchers.is; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; public class HttpFieldTest @@ -165,6 +166,12 @@ public class HttpFieldTest assertEquals("c", values[2]); } + @Test + public void testFieldNameNull() + { + assertThrows(NullPointerException.class, () -> new HttpField((String)null, null)); + } + @Test public void testCachedField() { diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpFieldsTest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpFieldsTest.java index d42638e58e6..8d10cbd6e67 100644 --- a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpFieldsTest.java +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpFieldsTest.java @@ -19,6 +19,7 @@ package org.eclipse.jetty.http; import java.nio.ByteBuffer; +import java.util.ArrayList; import java.util.Enumeration; import java.util.Iterator; import java.util.List; @@ -696,6 +697,44 @@ public class HttpFieldsTest }); } + @Test + public void testAddNullName() + { + HttpFields fields = new HttpFields(); + assertThrows(NullPointerException.class, () -> fields.add((String)null, "bogus")); + assertThat(fields.size(), is(0)); + + assertThrows(NullPointerException.class, () -> fields.add((HttpHeader)null, "bogus")); + assertThat(fields.size(), is(0)); + } + + @Test + public void testPutNullName() + { + HttpFields fields = new HttpFields(); + assertThrows(NullPointerException.class, () -> fields.put((String)null, "bogus")); + assertThat(fields.size(), is(0)); + + assertThrows(NullPointerException.class, () -> fields.put(null, (List)null)); + assertThat(fields.size(), is(0)); + + List emptyList = new ArrayList<>(); + assertThrows(NullPointerException.class, () -> fields.put(null, emptyList)); + assertThat(fields.size(), is(0)); + + assertThrows(NullPointerException.class, () -> fields.put((HttpHeader)null, "bogus")); + assertThat(fields.size(), is(0)); + } + + @Test + public void testPutNullValueList() + { + HttpFields fields = new HttpFields(); + + fields.put("name", (List)null); + assertThat(fields.size(), is(0)); + } + @Test public void testPreventNullField() { @@ -710,15 +749,15 @@ public class HttpFieldsTest assertThat(fields.size(), is(1)); ListIterator iter = fields.listIterator(); iter.next(); - iter.set(null); + iter.set(null); // set field to null - null entry in list now assertThat(fields.size(), is(0)); - iter.add(null); + iter.add(null); // attempt to add null entry assertThat(fields.size(), is(0)); fields.put("something", "other"); assertThat(fields.size(), is(1)); iter = fields.listIterator(); iter.next(); - iter.remove(); + iter.remove(); // remove only entry assertThat(fields.size(), is(0)); fields.put("something", "other"); assertThat(fields.size(), is(1)); From 59a66158ed0c12dd10cec842b0ccf595cbe2e4eb Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Mon, 11 May 2020 22:09:19 +0200 Subject: [PATCH 7/8] Issue #4860 NPE HttpFields Fixes from review. Fixed iterator overflow bug clearer updates of size better nonNull messages Signed-off-by: Greg Wilkins --- .../org/eclipse/jetty/http/HttpFields.java | 18 +++++++++++------- .../org/eclipse/jetty/http/HttpFieldsTest.java | 2 +- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpFields.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpFields.java index bd3171a7591..019b8601fc8 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpFields.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpFields.java @@ -636,7 +636,8 @@ public class HttpFields implements Iterable { if (put) { - System.arraycopy(_fields, i + 1, _fields, i, --_size - i); + _size--; + System.arraycopy(_fields, i + 1, _fields, i, _size - i); } else { @@ -676,7 +677,7 @@ public class HttpFields implements Iterable */ public void put(HttpHeader header, String value) { - Objects.requireNonNull(header, "header"); + Objects.requireNonNull(header, "header must not be null"); if (value == null) remove(header); @@ -692,7 +693,7 @@ public class HttpFields implements Iterable */ public void put(String name, List list) { - Objects.requireNonNull(name, "name"); + Objects.requireNonNull(name, "name must not be null"); remove(name); if (list == null) @@ -736,7 +737,7 @@ public class HttpFields implements Iterable */ public void add(HttpHeader header, String value) { - Objects.requireNonNull(header, "header"); + Objects.requireNonNull(header, "header must not be null"); if (value == null) throw new IllegalArgumentException("null value"); @@ -760,7 +761,8 @@ public class HttpFields implements Iterable if (f.getHeader() == name) { removed = f; - System.arraycopy(_fields, i + 1, _fields, i, --_size - i); + _size--; + System.arraycopy(_fields, i + 1, _fields, i, _size - i); } } return removed; @@ -781,7 +783,8 @@ public class HttpFields implements Iterable if (f.getName().equalsIgnoreCase(name)) { removed = f; - System.arraycopy(_fields, i + 1, _fields, i, --_size - i); + _size--; + System.arraycopy(_fields, i + 1, _fields, i, _size - i); } } return removed; @@ -1229,8 +1232,9 @@ public class HttpFields implements Iterable if (field != null) { _fields = Arrays.copyOf(_fields, _fields.length + 1); - System.arraycopy(_fields, _cursor, _fields, _cursor + 1, _size++); + System.arraycopy(_fields, _cursor, _fields, _cursor + 1, _size - _cursor); _fields[_cursor++] = field; + _size++; _current = -1; } } diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpFieldsTest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpFieldsTest.java index 8d10cbd6e67..b24b623a3ba 100644 --- a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpFieldsTest.java +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpFieldsTest.java @@ -767,7 +767,7 @@ public class HttpFieldsTest @Test public void testIteration() { - HttpFields header = new HttpFields(); + HttpFields header = new HttpFields(5); Iterator i = header.iterator(); assertThat(i.hasNext(), is(false)); From 4dc024b2b1b4d84493675403df77e2724bed831f Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Mon, 11 May 2020 17:25:49 -0500 Subject: [PATCH 8/8] Issue #4860 - Fixing test comment Signed-off-by: Joakim Erdfelt --- .../src/test/java/org/eclipse/jetty/http/HttpFieldsTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpFieldsTest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpFieldsTest.java index 8d10cbd6e67..9becdd06e1f 100644 --- a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpFieldsTest.java +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpFieldsTest.java @@ -749,7 +749,7 @@ public class HttpFieldsTest assertThat(fields.size(), is(1)); ListIterator iter = fields.listIterator(); iter.next(); - iter.set(null); // set field to null - null entry in list now + iter.set(null); // set field to null - should result in noop assertThat(fields.size(), is(0)); iter.add(null); // attempt to add null entry assertThat(fields.size(), is(0));