From a96fb176f62cf52495fd3934596bcffbc814479e Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Fri, 11 Oct 2019 11:24:48 +1100 Subject: [PATCH 01/11] Issue #4142 - module config for http2 maxSettingsKeys and RateControl Signed-off-by: Lachlan Roberts --- .../http2-server/src/main/config/etc/jetty-http2.xml | 11 +++++++++++ .../http2-server/src/main/config/etc/jetty-http2c.xml | 11 +++++++++++ .../http2-server/src/main/config/modules/http2.mod | 6 ++++++ .../http2-server/src/main/config/modules/http2c.mod | 6 ++++++ 4 files changed, 34 insertions(+) diff --git a/jetty-http2/http2-server/src/main/config/etc/jetty-http2.xml b/jetty-http2/http2-server/src/main/config/etc/jetty-http2.xml index 56777719fdf..7bcc76e28a4 100644 --- a/jetty-http2/http2-server/src/main/config/etc/jetty-http2.xml +++ b/jetty-http2/http2-server/src/main/config/etc/jetty-http2.xml @@ -9,6 +9,17 @@ + + + + + + + 1 + + + + diff --git a/jetty-http2/http2-server/src/main/config/etc/jetty-http2c.xml b/jetty-http2/http2-server/src/main/config/etc/jetty-http2c.xml index 37cb745fe0e..68f7e470cde 100644 --- a/jetty-http2/http2-server/src/main/config/etc/jetty-http2c.xml +++ b/jetty-http2/http2-server/src/main/config/etc/jetty-http2c.xml @@ -8,6 +8,17 @@ + + + + + + + 1 + + + + diff --git a/jetty-http2/http2-server/src/main/config/modules/http2.mod b/jetty-http2/http2-server/src/main/config/modules/http2.mod index 2ca6596e578..fae583a370f 100644 --- a/jetty-http2/http2-server/src/main/config/modules/http2.mod +++ b/jetty-http2/http2-server/src/main/config/modules/http2.mod @@ -29,3 +29,9 @@ etc/jetty-http2.xml ## Initial session receive window (client to server) # jetty.http2.initialSessionRecvWindow=1048576 + +## The max number of keys in all SETTINGS frames +# jetty.http2.maxSettingsKeys=64 + +## Max number of bad frames and pings per second +# jetty.http2.rateControl=20 diff --git a/jetty-http2/http2-server/src/main/config/modules/http2c.mod b/jetty-http2/http2-server/src/main/config/modules/http2c.mod index b109a8c4ce8..ff763760b4f 100644 --- a/jetty-http2/http2-server/src/main/config/modules/http2c.mod +++ b/jetty-http2/http2-server/src/main/config/modules/http2c.mod @@ -24,3 +24,9 @@ etc/jetty-http2c.xml ## Initial stream receive window (client to server) # jetty.http2c.initialStreamRecvWindow=65535 + +## The max number of keys in all SETTINGS frames +# jetty.http2.maxSettingsKeys=64 + +## Max number of bad frames and pings per second +# jetty.http2.rateControl=20 From 5ff79b0bf1164ff3124873a52a6979b9097794a4 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 15 Oct 2019 11:23:53 +1100 Subject: [PATCH 02/11] Issue #4142 - changes from review Signed-off-by: Lachlan Roberts --- .../eclipse/jetty/http2/parser/WindowRateControl.java | 5 +++++ .../http2-server/src/main/config/etc/jetty-http2.xml | 11 +++-------- .../http2-server/src/main/config/etc/jetty-http2c.xml | 11 +++-------- .../http2-server/src/main/config/modules/http2.mod | 2 +- .../http2-server/src/main/config/modules/http2c.mod | 2 +- 5 files changed, 13 insertions(+), 18 deletions(-) diff --git a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/parser/WindowRateControl.java b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/parser/WindowRateControl.java index 00a2c769737..7a198637cbc 100644 --- a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/parser/WindowRateControl.java +++ b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/parser/WindowRateControl.java @@ -39,6 +39,11 @@ public class WindowRateControl implements RateControl private final int maxEvents; private final long window; + public static WindowRateControl ofSecond(int maxEvents) + { + return new WindowRateControl(maxEvents, Duration.ofSeconds(1)); + } + public WindowRateControl(int maxEvents, Duration window) { this.maxEvents = maxEvents; diff --git a/jetty-http2/http2-server/src/main/config/etc/jetty-http2.xml b/jetty-http2/http2-server/src/main/config/etc/jetty-http2.xml index 7bcc76e28a4..c213d35fe29 100644 --- a/jetty-http2/http2-server/src/main/config/etc/jetty-http2.xml +++ b/jetty-http2/http2-server/src/main/config/etc/jetty-http2.xml @@ -11,14 +11,9 @@ - - - - - 1 - - - + + + diff --git a/jetty-http2/http2-server/src/main/config/etc/jetty-http2c.xml b/jetty-http2/http2-server/src/main/config/etc/jetty-http2c.xml index 68f7e470cde..e4c9ad69f99 100644 --- a/jetty-http2/http2-server/src/main/config/etc/jetty-http2c.xml +++ b/jetty-http2/http2-server/src/main/config/etc/jetty-http2c.xml @@ -10,14 +10,9 @@ - - - - - 1 - - - + + + diff --git a/jetty-http2/http2-server/src/main/config/modules/http2.mod b/jetty-http2/http2-server/src/main/config/modules/http2.mod index fae583a370f..ccc7d7b520c 100644 --- a/jetty-http2/http2-server/src/main/config/modules/http2.mod +++ b/jetty-http2/http2-server/src/main/config/modules/http2.mod @@ -34,4 +34,4 @@ etc/jetty-http2.xml # jetty.http2.maxSettingsKeys=64 ## Max number of bad frames and pings per second -# jetty.http2.rateControl=20 +# jetty.http2.rateControlWindow=20 diff --git a/jetty-http2/http2-server/src/main/config/modules/http2c.mod b/jetty-http2/http2-server/src/main/config/modules/http2c.mod index ff763760b4f..82e0a67d1b9 100644 --- a/jetty-http2/http2-server/src/main/config/modules/http2c.mod +++ b/jetty-http2/http2-server/src/main/config/modules/http2c.mod @@ -29,4 +29,4 @@ etc/jetty-http2c.xml # jetty.http2.maxSettingsKeys=64 ## Max number of bad frames and pings per second -# jetty.http2.rateControl=20 +# jetty.http2.rateControlWindow=20 From 709e05a19fdbf61ab99b91ca0d42abbf3f327020 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 15 Oct 2019 19:08:45 +1100 Subject: [PATCH 03/11] Issue #4142 - changes from review Signed-off-by: Lachlan Roberts --- .../java/org/eclipse/jetty/http2/parser/WindowRateControl.java | 2 +- jetty-http2/http2-server/src/main/config/etc/jetty-http2.xml | 2 +- jetty-http2/http2-server/src/main/config/etc/jetty-http2c.xml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/parser/WindowRateControl.java b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/parser/WindowRateControl.java index 7a198637cbc..4c00b01218b 100644 --- a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/parser/WindowRateControl.java +++ b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/parser/WindowRateControl.java @@ -39,7 +39,7 @@ public class WindowRateControl implements RateControl private final int maxEvents; private final long window; - public static WindowRateControl ofSecond(int maxEvents) + public static WindowRateControl fromEventsPerSecond(int maxEvents) { return new WindowRateControl(maxEvents, Duration.ofSeconds(1)); } diff --git a/jetty-http2/http2-server/src/main/config/etc/jetty-http2.xml b/jetty-http2/http2-server/src/main/config/etc/jetty-http2.xml index c213d35fe29..52129459c1f 100644 --- a/jetty-http2/http2-server/src/main/config/etc/jetty-http2.xml +++ b/jetty-http2/http2-server/src/main/config/etc/jetty-http2.xml @@ -11,7 +11,7 @@ - + diff --git a/jetty-http2/http2-server/src/main/config/etc/jetty-http2c.xml b/jetty-http2/http2-server/src/main/config/etc/jetty-http2c.xml index e4c9ad69f99..2631110f658 100644 --- a/jetty-http2/http2-server/src/main/config/etc/jetty-http2c.xml +++ b/jetty-http2/http2-server/src/main/config/etc/jetty-http2c.xml @@ -10,7 +10,7 @@ - + From 4f640387f21dcd2006312e55bae56e008ed22080 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 15 Oct 2019 21:09:01 +1100 Subject: [PATCH 04/11] Issue #4142 - changes from review Signed-off-by: Lachlan Roberts --- jetty-http2/http2-server/src/main/config/etc/jetty-http2.xml | 2 +- jetty-http2/http2-server/src/main/config/etc/jetty-http2c.xml | 2 +- jetty-http2/http2-server/src/main/config/modules/http2.mod | 2 +- jetty-http2/http2-server/src/main/config/modules/http2c.mod | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/jetty-http2/http2-server/src/main/config/etc/jetty-http2.xml b/jetty-http2/http2-server/src/main/config/etc/jetty-http2.xml index 52129459c1f..3cb2c652d65 100644 --- a/jetty-http2/http2-server/src/main/config/etc/jetty-http2.xml +++ b/jetty-http2/http2-server/src/main/config/etc/jetty-http2.xml @@ -12,7 +12,7 @@ - + diff --git a/jetty-http2/http2-server/src/main/config/etc/jetty-http2c.xml b/jetty-http2/http2-server/src/main/config/etc/jetty-http2c.xml index 2631110f658..cf169b47f65 100644 --- a/jetty-http2/http2-server/src/main/config/etc/jetty-http2c.xml +++ b/jetty-http2/http2-server/src/main/config/etc/jetty-http2c.xml @@ -11,7 +11,7 @@ - + diff --git a/jetty-http2/http2-server/src/main/config/modules/http2.mod b/jetty-http2/http2-server/src/main/config/modules/http2.mod index ccc7d7b520c..ab57f77caeb 100644 --- a/jetty-http2/http2-server/src/main/config/modules/http2.mod +++ b/jetty-http2/http2-server/src/main/config/modules/http2.mod @@ -34,4 +34,4 @@ etc/jetty-http2.xml # jetty.http2.maxSettingsKeys=64 ## Max number of bad frames and pings per second -# jetty.http2.rateControlWindow=20 +# jetty.http2.maxEventsPerSecond=20 diff --git a/jetty-http2/http2-server/src/main/config/modules/http2c.mod b/jetty-http2/http2-server/src/main/config/modules/http2c.mod index 82e0a67d1b9..3b844cdda1e 100644 --- a/jetty-http2/http2-server/src/main/config/modules/http2c.mod +++ b/jetty-http2/http2-server/src/main/config/modules/http2c.mod @@ -29,4 +29,4 @@ etc/jetty-http2c.xml # jetty.http2.maxSettingsKeys=64 ## Max number of bad frames and pings per second -# jetty.http2.rateControlWindow=20 +# jetty.http2.maxEventsPerSecond=20 From 5329ecf5bebcf582b2c665867921122ec5e1ee9a Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 15 Oct 2019 21:56:54 +1100 Subject: [PATCH 05/11] Issue #4142 - changes from review Signed-off-by: Lachlan Roberts --- jetty-http2/http2-server/src/main/config/etc/jetty-http2.xml | 2 +- jetty-http2/http2-server/src/main/config/etc/jetty-http2c.xml | 2 +- jetty-http2/http2-server/src/main/config/modules/http2.mod | 2 +- jetty-http2/http2-server/src/main/config/modules/http2c.mod | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/jetty-http2/http2-server/src/main/config/etc/jetty-http2.xml b/jetty-http2/http2-server/src/main/config/etc/jetty-http2.xml index 3cb2c652d65..43a59e88c51 100644 --- a/jetty-http2/http2-server/src/main/config/etc/jetty-http2.xml +++ b/jetty-http2/http2-server/src/main/config/etc/jetty-http2.xml @@ -12,7 +12,7 @@ - + diff --git a/jetty-http2/http2-server/src/main/config/etc/jetty-http2c.xml b/jetty-http2/http2-server/src/main/config/etc/jetty-http2c.xml index cf169b47f65..a9e4380c0aa 100644 --- a/jetty-http2/http2-server/src/main/config/etc/jetty-http2c.xml +++ b/jetty-http2/http2-server/src/main/config/etc/jetty-http2c.xml @@ -11,7 +11,7 @@ - + diff --git a/jetty-http2/http2-server/src/main/config/modules/http2.mod b/jetty-http2/http2-server/src/main/config/modules/http2.mod index ab57f77caeb..c33beaf2c25 100644 --- a/jetty-http2/http2-server/src/main/config/modules/http2.mod +++ b/jetty-http2/http2-server/src/main/config/modules/http2.mod @@ -34,4 +34,4 @@ etc/jetty-http2.xml # jetty.http2.maxSettingsKeys=64 ## Max number of bad frames and pings per second -# jetty.http2.maxEventsPerSecond=20 +# jetty.http2.rateControl.maxEventsPerSecond=20 diff --git a/jetty-http2/http2-server/src/main/config/modules/http2c.mod b/jetty-http2/http2-server/src/main/config/modules/http2c.mod index 3b844cdda1e..cac69f065ad 100644 --- a/jetty-http2/http2-server/src/main/config/modules/http2c.mod +++ b/jetty-http2/http2-server/src/main/config/modules/http2c.mod @@ -29,4 +29,4 @@ etc/jetty-http2c.xml # jetty.http2.maxSettingsKeys=64 ## Max number of bad frames and pings per second -# jetty.http2.maxEventsPerSecond=20 +# jetty.http2.rateControl.maxEventsPerSecond=20 From 2eb251a4b898feeb3ce55fc040e32ba4bdf33f86 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Wed, 16 Oct 2019 13:53:57 +1100 Subject: [PATCH 06/11] fix logging defaults to INFO Signed-off-by: Greg Wilkins --- examples/embedded/src/test/resources/jetty-logging.properties | 2 +- jetty-server/src/test/resources/jetty-logging.properties | 2 +- jetty-servlet/src/test/resources/jetty-logging.properties | 2 +- .../src/test/resources/jetty-logging.properties | 2 +- .../src/test/resources/jetty-logging.properties | 2 +- .../src/test/resources/jetty-logging.properties | 2 +- .../src/test/resources/jetty-logging.properties | 2 +- .../src/test/resources/jetty-logging.properties | 2 +- .../src/test/resources/jetty-logging.properties | 2 +- .../src/test/resources/jetty-logging.properties | 2 +- .../src/test/resources/jetty-logging.properties | 2 +- 11 files changed, 11 insertions(+), 11 deletions(-) diff --git a/examples/embedded/src/test/resources/jetty-logging.properties b/examples/embedded/src/test/resources/jetty-logging.properties index b17c80c3330..b86623fd081 100644 --- a/examples/embedded/src/test/resources/jetty-logging.properties +++ b/examples/embedded/src/test/resources/jetty-logging.properties @@ -1,5 +1,5 @@ org.eclipse.jetty.util.log.class=org.eclipse.jetty.util.log.StdErrLog -org.eclipse.jetty.LEVEL=WARN +org.eclipse.jetty.LEVEL=INFO org.eclipse.jetty.embedded.JettyDistribution.LEVEL=DEBUG #org.eclipse.jetty.STACKS=true #org.eclipse.jetty.STACKS=false diff --git a/jetty-server/src/test/resources/jetty-logging.properties b/jetty-server/src/test/resources/jetty-logging.properties index 1ba30af5c5f..bc71630ee88 100644 --- a/jetty-server/src/test/resources/jetty-logging.properties +++ b/jetty-server/src/test/resources/jetty-logging.properties @@ -1,5 +1,5 @@ org.eclipse.jetty.util.log.class=org.eclipse.jetty.util.log.StdErrLog -org.eclipse.jetty.LEVEL=WARN +org.eclipse.jetty.LEVEL=INFO #org.eclipse.jetty.LEVEL=DEBUG #org.eclipse.jetty.server.LEVEL=DEBUG #org.eclipse.jetty.server.ConnectionLimit.LEVEL=DEBUG diff --git a/jetty-servlet/src/test/resources/jetty-logging.properties b/jetty-servlet/src/test/resources/jetty-logging.properties index 354a0e3a707..37f092141fc 100644 --- a/jetty-servlet/src/test/resources/jetty-logging.properties +++ b/jetty-servlet/src/test/resources/jetty-logging.properties @@ -1,5 +1,5 @@ org.eclipse.jetty.util.log.class=org.eclipse.jetty.util.log.StdErrLog -org.eclipse.jetty.LEVEL=WARN +org.eclipse.jetty.LEVEL=INFO #org.eclipse.jetty.LEVEL=DEBUG #org.eclipse.jetty.server.LEVEL=DEBUG #org.eclipse.jetty.servlet.LEVEL=DEBUG diff --git a/jetty-websocket/javax-websocket-client-impl/src/test/resources/jetty-logging.properties b/jetty-websocket/javax-websocket-client-impl/src/test/resources/jetty-logging.properties index 2b8dfa9b594..b992ca52252 100644 --- a/jetty-websocket/javax-websocket-client-impl/src/test/resources/jetty-logging.properties +++ b/jetty-websocket/javax-websocket-client-impl/src/test/resources/jetty-logging.properties @@ -1,5 +1,5 @@ org.eclipse.jetty.util.log.class=org.eclipse.jetty.util.log.StdErrLog -org.eclipse.jetty.LEVEL=WARN +org.eclipse.jetty.LEVEL=INFO # org.eclipse.jetty.websocket.LEVEL=DEBUG # org.eclipse.jetty.websocket.LEVEL=ALL diff --git a/jetty-websocket/javax-websocket-server-impl/src/test/resources/jetty-logging.properties b/jetty-websocket/javax-websocket-server-impl/src/test/resources/jetty-logging.properties index 5474a69539e..7477f382ec9 100644 --- a/jetty-websocket/javax-websocket-server-impl/src/test/resources/jetty-logging.properties +++ b/jetty-websocket/javax-websocket-server-impl/src/test/resources/jetty-logging.properties @@ -1,5 +1,5 @@ org.eclipse.jetty.util.log.class=org.eclipse.jetty.util.log.StdErrLog -org.eclipse.jetty.LEVEL=WARN +org.eclipse.jetty.LEVEL=INFO # org.eclipse.jetty.websocket.LEVEL=DEBUG # org.eclipse.jetty.websocket.LEVEL=INFO diff --git a/jetty-websocket/jetty-websocket-tests/src/test/resources/jetty-logging.properties b/jetty-websocket/jetty-websocket-tests/src/test/resources/jetty-logging.properties index 5cd1295117d..a429c612f5e 100644 --- a/jetty-websocket/jetty-websocket-tests/src/test/resources/jetty-logging.properties +++ b/jetty-websocket/jetty-websocket-tests/src/test/resources/jetty-logging.properties @@ -1,5 +1,5 @@ org.eclipse.jetty.util.log.class=org.eclipse.jetty.util.log.StdErrLog -org.eclipse.jetty.LEVEL=WARN +org.eclipse.jetty.LEVEL=INFO # org.eclipse.jetty.LEVEL=DEBUG # org.eclipse.jetty.io.LEVEL=INFO # org.eclipse.jetty.client.LEVEL=DEBUG diff --git a/jetty-websocket/websocket-client/src/test/resources/jetty-logging.properties b/jetty-websocket/websocket-client/src/test/resources/jetty-logging.properties index 3d5a1bb92cb..2545bc0dddf 100644 --- a/jetty-websocket/websocket-client/src/test/resources/jetty-logging.properties +++ b/jetty-websocket/websocket-client/src/test/resources/jetty-logging.properties @@ -1,5 +1,5 @@ org.eclipse.jetty.util.log.class=org.eclipse.jetty.util.log.StdErrLog -org.eclipse.jetty.LEVEL=WARN +org.eclipse.jetty.LEVEL=INFO # org.eclipse.jetty.LEVEL=DEBUG # org.eclipse.jetty.io.LEVEL=INFO # org.eclipse.jetty.client.LEVEL=DEBUG diff --git a/jetty-websocket/websocket-common/src/test/resources/jetty-logging.properties b/jetty-websocket/websocket-common/src/test/resources/jetty-logging.properties index 525226380f0..6207609e501 100644 --- a/jetty-websocket/websocket-common/src/test/resources/jetty-logging.properties +++ b/jetty-websocket/websocket-common/src/test/resources/jetty-logging.properties @@ -1,5 +1,5 @@ org.eclipse.jetty.util.log.class=org.eclipse.jetty.util.log.StdErrLog -org.eclipse.jetty.LEVEL=WARN +org.eclipse.jetty.LEVEL=INFO # org.eclipse.jetty.websocket.LEVEL=DEBUG # org.eclipse.jetty.websocket.common.test.LEVEL=DEBUG # org.eclipse.jetty.websocket.protocol.Parser.LEVEL=DEBUG diff --git a/jetty-websocket/websocket-server/src/test/resources/jetty-logging.properties b/jetty-websocket/websocket-server/src/test/resources/jetty-logging.properties index cd02b8ec412..e8dcdf39139 100644 --- a/jetty-websocket/websocket-server/src/test/resources/jetty-logging.properties +++ b/jetty-websocket/websocket-server/src/test/resources/jetty-logging.properties @@ -1,5 +1,5 @@ org.eclipse.jetty.util.log.class=org.eclipse.jetty.util.log.StdErrLog -org.eclipse.jetty.LEVEL=WARN +org.eclipse.jetty.LEVEL=INFO # org.eclipse.jetty.io.WriteFlusher.LEVEL=DEBUG # org.eclipse.jetty.websocket.LEVEL=DEBUG diff --git a/tests/test-continuation/src/test/resources/jetty-logging.properties b/tests/test-continuation/src/test/resources/jetty-logging.properties index d1e0e0cf038..bcf6eb5707f 100644 --- a/tests/test-continuation/src/test/resources/jetty-logging.properties +++ b/tests/test-continuation/src/test/resources/jetty-logging.properties @@ -1,3 +1,3 @@ org.eclipse.jetty.util.log.class=org.eclipse.jetty.util.log.StdErrLog -org.eclipse.jetty.LEVEL=WARN +org.eclipse.jetty.LEVEL=INFO # org.eclipse.jetty.server.LEVEL=DEBUG diff --git a/tests/test-integration/src/test/resources/jetty-logging.properties b/tests/test-integration/src/test/resources/jetty-logging.properties index fdc5a51caba..d79ba45bdd3 100644 --- a/tests/test-integration/src/test/resources/jetty-logging.properties +++ b/tests/test-integration/src/test/resources/jetty-logging.properties @@ -1,4 +1,4 @@ org.eclipse.jetty.util.log.class=org.eclipse.jetty.util.log.StdErrLog -org.eclipse.jetty.LEVEL=WARN +org.eclipse.jetty.LEVEL=INFO #org.eclipse.jetty.LEVEL=DEBUG #org.eclipse.jetty.websocket.LEVEL=DEBUG From 92c8bb8dd5c2b299c5d5e3418b938c0080f85be3 Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Wed, 16 Oct 2019 14:08:51 +1100 Subject: [PATCH 07/11] Issue #4158 Re-enable support for duplicate session cookies. (#4168) * Issue #4158 Re-enable support for duplicate session cookies. Signed-off-by: Jan Bartel Signed-off-by: Greg Wilkins --- .../jetty/server/session/SessionHandler.java | 34 ++- ...tractClusteredInvalidationSessionTest.java | 5 +- ...bstractClusteredSessionScavengingTest.java | 6 +- .../ClientCrossContextSessionTest.java | 5 +- .../server/session/DuplicateCookieTest.java | 226 ++++++++++++++++++ .../session/ReentrantRequestSessionTest.java | 1 - 6 files changed, 260 insertions(+), 17 deletions(-) create mode 100644 tests/test-sessions/test-sessions-common/src/test/java/org/eclipse/jetty/server/session/DuplicateCookieTest.java diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionHandler.java index 632ff9c7a70..65c170a0be2 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionHandler.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionHandler.java @@ -44,6 +44,7 @@ import javax.servlet.http.HttpSessionEvent; import javax.servlet.http.HttpSessionIdListener; import javax.servlet.http.HttpSessionListener; +import org.eclipse.jetty.http.BadMessageException; import org.eclipse.jetty.http.HttpCookie; import org.eclipse.jetty.server.Request; import org.eclipse.jetty.server.Server; @@ -1629,13 +1630,34 @@ public class SessionHandler extends ScopedHandler { if (sessionCookie.equalsIgnoreCase(cookies[i].getName())) { - requestedSessionId = cookies[i].getValue(); + String id = cookies[i].getValue(); requestedSessionIdFromCookie = true; if (LOG.isDebugEnabled()) - LOG.debug("Got Session ID {} from cookie {}", requestedSessionId, sessionCookie); - if (requestedSessionId != null) + LOG.debug("Got Session ID {} from cookie {}", id, sessionCookie); + + HttpSession s = getHttpSession(id); + + if (requestedSessionId == null) { - break; + //no previous id, always accept this one + requestedSessionId = id; + session = s; + } + else if (requestedSessionId.equals(id)) + { + //really a bad request, but will forgive the duplication + } + else if (session == null || !isValid(session)) + { + //no previous session or invalid, accept this one + requestedSessionId = id; + session = s; + } + else + { + //previous session is valid, use it unless both valid + if (s != null && isValid(s)) + throw new BadMessageException("Duplicate valid session cookies: " + requestedSessionId + "," + id); } } } @@ -1666,6 +1688,7 @@ public class SessionHandler extends ScopedHandler requestedSessionIdFromCookie = false; if (LOG.isDebugEnabled()) LOG.debug("Got Session ID {} from URL", requestedSessionId); + session = getHttpSession(requestedSessionId); } } } @@ -1675,11 +1698,10 @@ public class SessionHandler extends ScopedHandler if (requestedSessionId != null) { - session = getHttpSession(requestedSessionId); if (session != null && isValid(session)) { baseRequest.enterSession(session); //request enters this session for first time - baseRequest.setSession(session); + baseRequest.setSession(session); //associate the session with the request } } } diff --git a/tests/test-sessions/test-sessions-common/src/main/java/org/eclipse/jetty/server/session/AbstractClusteredInvalidationSessionTest.java b/tests/test-sessions/test-sessions-common/src/main/java/org/eclipse/jetty/server/session/AbstractClusteredInvalidationSessionTest.java index e0af754ec22..5f394478988 100644 --- a/tests/test-sessions/test-sessions-common/src/main/java/org/eclipse/jetty/server/session/AbstractClusteredInvalidationSessionTest.java +++ b/tests/test-sessions/test-sessions-common/src/main/java/org/eclipse/jetty/server/session/AbstractClusteredInvalidationSessionTest.java @@ -103,8 +103,6 @@ public abstract class AbstractClusteredInvalidationSessionTest extends AbstractT assertEquals(HttpServletResponse.SC_OK, response1.getStatus()); String sessionCookie = response1.getHeaders().get("Set-Cookie"); assertTrue(sessionCookie != null); - // Mangle the cookie, replacing Path with $Path, etc. - sessionCookie = sessionCookie.replaceFirst("(\\W)(P|p)ath=", "$1\\$Path="); //ensure request is fully finished processing latch.await(5, TimeUnit.SECONDS); @@ -113,7 +111,6 @@ public abstract class AbstractClusteredInvalidationSessionTest extends AbstractT latch = new CountDownLatch(1); scopeListener.setExitSynchronizer(latch); Request request2 = client.newRequest(urls[1] + "?action=increment"); - request2.header("Cookie", sessionCookie); ContentResponse response2 = request2.send(); assertEquals(HttpServletResponse.SC_OK, response2.getStatus()); @@ -153,6 +150,8 @@ public abstract class AbstractClusteredInvalidationSessionTest extends AbstractT public static class TestServlet extends HttpServlet { + private static final long serialVersionUID = 1L; + @Override protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException { diff --git a/tests/test-sessions/test-sessions-common/src/main/java/org/eclipse/jetty/server/session/AbstractClusteredSessionScavengingTest.java b/tests/test-sessions/test-sessions-common/src/main/java/org/eclipse/jetty/server/session/AbstractClusteredSessionScavengingTest.java index 06bf2f6edc9..b65cffe0fdc 100644 --- a/tests/test-sessions/test-sessions-common/src/main/java/org/eclipse/jetty/server/session/AbstractClusteredSessionScavengingTest.java +++ b/tests/test-sessions/test-sessions-common/src/main/java/org/eclipse/jetty/server/session/AbstractClusteredSessionScavengingTest.java @@ -117,6 +117,7 @@ public abstract class AbstractClusteredSessionScavengingTest extends AbstractTes assertTrue(response1.getContentAsString().startsWith("init")); String sessionCookie = response1.getHeaders().get("Set-Cookie"); assertTrue(sessionCookie != null); + String id = TestServer.extractSessionId(sessionCookie); //ensure request has finished being handled synchronizer.await(5, TimeUnit.SECONDS); @@ -124,9 +125,7 @@ public abstract class AbstractClusteredSessionScavengingTest extends AbstractTes assertEquals(1, ((DefaultSessionCache)m1.getSessionCache()).getSessionsCurrent()); assertEquals(1, ((DefaultSessionCache)m1.getSessionCache()).getSessionsMax()); assertEquals(1, ((DefaultSessionCache)m1.getSessionCache()).getSessionsTotal()); - // Mangle the cookie, replacing Path with $Path, etc. - sessionCookie = sessionCookie.replaceFirst("(\\W)(P|p)ath=", "$1\\$Path="); - String id = TestServer.extractSessionId(sessionCookie); + //Peek at the contents of the cache without doing all the reference counting etc Session s1 = ((AbstractSessionCache)m1.getSessionCache()).doGet(id); @@ -144,7 +143,6 @@ public abstract class AbstractClusteredSessionScavengingTest extends AbstractTes synchronizer = new CountDownLatch(1); scopeListener2.setExitSynchronizer(synchronizer); Request request = client.newRequest("http://localhost:" + port2 + contextPath + servletMapping.substring(1)); - request.header("Cookie", sessionCookie); //use existing session ContentResponse response2 = request.send(); assertEquals(HttpServletResponse.SC_OK, response2.getStatus()); assertTrue(response2.getContentAsString().startsWith("test")); diff --git a/tests/test-sessions/test-sessions-common/src/test/java/org/eclipse/jetty/server/session/ClientCrossContextSessionTest.java b/tests/test-sessions/test-sessions-common/src/test/java/org/eclipse/jetty/server/session/ClientCrossContextSessionTest.java index 1363f0b7480..2848db4c20b 100644 --- a/tests/test-sessions/test-sessions-common/src/test/java/org/eclipse/jetty/server/session/ClientCrossContextSessionTest.java +++ b/tests/test-sessions/test-sessions-common/src/test/java/org/eclipse/jetty/server/session/ClientCrossContextSessionTest.java @@ -82,12 +82,11 @@ public class ClientCrossContextSessionTest assertEquals(HttpServletResponse.SC_OK, response.getStatus()); String sessionCookie = response.getHeaders().get("Set-Cookie"); assertTrue(sessionCookie != null); - // Mangle the cookie, replacing Path with $Path, etc. - sessionCookie = sessionCookie.replaceFirst("(\\W)(P|p)ath=", "$1\\$Path="); + String sessionId = TestServer.extractSessionId(sessionCookie); // Perform a request to contextB with the same session cookie Request request = client.newRequest("http://localhost:" + port + contextB + servletMapping); - request.header("Cookie", sessionCookie); + request.header("Cookie", "JSESSIONID=" + sessionId); ContentResponse responseB = request.send(); assertEquals(HttpServletResponse.SC_OK, responseB.getStatus()); assertEquals(servletA.sessionId, servletB.sessionId); diff --git a/tests/test-sessions/test-sessions-common/src/test/java/org/eclipse/jetty/server/session/DuplicateCookieTest.java b/tests/test-sessions/test-sessions-common/src/test/java/org/eclipse/jetty/server/session/DuplicateCookieTest.java new file mode 100644 index 00000000000..b60997519c8 --- /dev/null +++ b/tests/test-sessions/test-sessions-common/src/test/java/org/eclipse/jetty/server/session/DuplicateCookieTest.java @@ -0,0 +1,226 @@ +// +// ======================================================================== +// Copyright (c) 1995-2019 Mort Bay Consulting Pty. Ltd. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.server.session; + +import java.io.IOException; +import java.util.concurrent.TimeUnit; + +import javax.servlet.ServletException; +import javax.servlet.http.HttpServlet; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import javax.servlet.http.HttpSession; + +import org.eclipse.jetty.client.HttpClient; +import org.eclipse.jetty.client.api.ContentResponse; +import org.eclipse.jetty.client.api.Request; +import org.eclipse.jetty.servlet.ServletContextHandler; +import org.eclipse.jetty.servlet.ServletHolder; +import org.eclipse.jetty.util.StringUtil; +import org.eclipse.jetty.util.log.Log; +import org.eclipse.jetty.util.log.StacklessLogging; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; + +/** + * Test having multiple session cookies in a request. + */ +public class DuplicateCookieTest +{ + @Test + public void testMultipleSessionCookiesOnlyOneExists() throws Exception + { + String contextPath = ""; + String servletMapping = "/server"; + HttpClient client = null; + + DefaultSessionCacheFactory cacheFactory = new DefaultSessionCacheFactory(); + SessionDataStoreFactory storeFactory = new TestSessionDataStoreFactory(); + + TestServer server1 = new TestServer(0, -1, -1, cacheFactory, storeFactory); + TestServlet servlet = new TestServlet(); + ServletHolder holder = new ServletHolder(servlet); + ServletContextHandler contextHandler = server1.addContext(contextPath); + contextHandler.addServlet(holder, servletMapping); + server1.start(); + int port1 = server1.getPort(); + + try (StacklessLogging stackless = new StacklessLogging(Log.getLogger("org.eclipse.jetty.server.session"))) + { + //create a valid session + createUnExpiredSession(contextHandler.getSessionHandler().getSessionCache(), + contextHandler.getSessionHandler().getSessionCache().getSessionDataStore(), + "4422"); + + client = new HttpClient(); + client.start(); + + //make a request with another session cookie in there that does not exist + Request request = client.newRequest("http://localhost:" + port1 + contextPath + servletMapping + "?action=check"); + request.header("Cookie", "JSESSIONID=123"); //doesn't exist + request.header("Cookie", "JSESSIONID=4422"); //does exist + ContentResponse response = request.send(); + assertEquals(HttpServletResponse.SC_OK, response.getStatus()); + assertEquals("4422", response.getContentAsString()); + } + finally + { + server1.stop(); + client.stop(); + } + } + + @Test + public void testMultipleSessionCookiesOnlyOneValid() throws Exception + { + String contextPath = ""; + String servletMapping = "/server"; + HttpClient client = null; + + DefaultSessionCacheFactory cacheFactory = new DefaultSessionCacheFactory(); + SessionDataStoreFactory storeFactory = new TestSessionDataStoreFactory(); + + TestServer server1 = new TestServer(0, -1, -1, cacheFactory, storeFactory); + TestServlet servlet = new TestServlet(); + ServletHolder holder = new ServletHolder(servlet); + ServletContextHandler contextHandler = server1.addContext(contextPath); + contextHandler.addServlet(holder, servletMapping); + server1.start(); + int port1 = server1.getPort(); + + try (StacklessLogging stackless = new StacklessLogging(Log.getLogger("org.eclipse.jetty.server.session"))) + { + //create a valid session + createUnExpiredSession(contextHandler.getSessionHandler().getSessionCache(), + contextHandler.getSessionHandler().getSessionCache().getSessionDataStore(), + "1122"); + //create an invalid session + createInvalidSession(contextHandler.getSessionHandler().getSessionCache(), + contextHandler.getSessionHandler().getSessionCache().getSessionDataStore(), + "2233"); + + client = new HttpClient(); + client.start(); + + //make a request with another session cookie in there that is not valid + Request request = client.newRequest("http://localhost:" + port1 + contextPath + servletMapping + "?action=check"); + request.header("Cookie", "JSESSIONID=1122"); //is valid + request.header("Cookie", "JSESSIONID=2233"); //is invalid + ContentResponse response = request.send(); + assertEquals(HttpServletResponse.SC_OK, response.getStatus()); + assertEquals("1122", response.getContentAsString()); + } + finally + { + server1.stop(); + client.stop(); + } + } + + @Test + public void testMultipleSessionCookiesMultipleExists() throws Exception + { + String contextPath = ""; + String servletMapping = "/server"; + HttpClient client = null; + + DefaultSessionCacheFactory cacheFactory = new DefaultSessionCacheFactory(); + SessionDataStoreFactory storeFactory = new TestSessionDataStoreFactory(); + + TestServer server1 = new TestServer(0, -1, -1, cacheFactory, storeFactory); + TestServlet servlet = new TestServlet(); + ServletHolder holder = new ServletHolder(servlet); + ServletContextHandler contextHandler = server1.addContext(contextPath); + contextHandler.addServlet(holder, servletMapping); + server1.start(); + int port1 = server1.getPort(); + + try (StacklessLogging stackless = new StacklessLogging(Log.getLogger("org.eclipse.jetty.server.session"))) + { + //create some of unexpired sessions + createUnExpiredSession(contextHandler.getSessionHandler().getSessionCache(), + contextHandler.getSessionHandler().getSessionCache().getSessionDataStore(), + "1234"); + createUnExpiredSession(contextHandler.getSessionHandler().getSessionCache(), + contextHandler.getSessionHandler().getSessionCache().getSessionDataStore(), + "5678"); + createUnExpiredSession(contextHandler.getSessionHandler().getSessionCache(), + contextHandler.getSessionHandler().getSessionCache().getSessionDataStore(), + "9111"); + + client = new HttpClient(); + client.start(); + + //make a request with multiple valid session ids + Request request = client.newRequest("http://localhost:" + port1 + contextPath + servletMapping + "?action=check"); + request.header("Cookie", "JSESSIONID=1234"); + request.header("Cookie", "JSESSIONID=5678"); + ContentResponse response = request.send(); + assertEquals(HttpServletResponse.SC_BAD_REQUEST, response.getStatus()); + } + finally + { + server1.stop(); + client.stop(); + } + } + + public Session createUnExpiredSession(SessionCache cache, SessionDataStore store, String id) throws Exception + { + long now = System.currentTimeMillis(); + SessionData data = store.newSessionData(id, now - 20, now - 10, now - 20, TimeUnit.MINUTES.toMillis(10)); + data.setExpiry(now + TimeUnit.DAYS.toMillis(1)); + Session s = cache.newSession(data); + cache.add(id, s); + return s; + } + + public Session createInvalidSession(SessionCache cache, SessionDataStore store, String id) throws Exception + { + Session session = createUnExpiredSession(cache, store, id); + session._state = Session.State.INVALID; + return session; + } + + public static class TestServlet extends HttpServlet + { + private static final long serialVersionUID = 1L; + + @Override + protected void doGet(HttpServletRequest request, HttpServletResponse httpServletResponse) throws ServletException, IOException + { + String action = request.getParameter("action"); + if (StringUtil.isBlank(action)) + return; + + if (action.equalsIgnoreCase("check")) + { + HttpSession session = request.getSession(false); + assertNotNull(session); + httpServletResponse.getWriter().print(session.getId()); + } + else if (action.equalsIgnoreCase("create")) + { + request.getSession(true); + } + } + } +} diff --git a/tests/test-sessions/test-sessions-common/src/test/java/org/eclipse/jetty/server/session/ReentrantRequestSessionTest.java b/tests/test-sessions/test-sessions-common/src/test/java/org/eclipse/jetty/server/session/ReentrantRequestSessionTest.java index 663444b8a82..a3b68761603 100644 --- a/tests/test-sessions/test-sessions-common/src/test/java/org/eclipse/jetty/server/session/ReentrantRequestSessionTest.java +++ b/tests/test-sessions/test-sessions-common/src/test/java/org/eclipse/jetty/server/session/ReentrantRequestSessionTest.java @@ -85,7 +85,6 @@ public class ReentrantRequestSessionTest //make a request that will make a simultaneous request for the same session Request request = client.newRequest("http://localhost:" + port + contextPath + servletMapping + "?action=reenter&port=" + port + "&path=" + contextPath + servletMapping); - request.header("Cookie", sessionCookie); response = request.send(); assertEquals(HttpServletResponse.SC_OK, response.getStatus()); } From 73924d277444f5089a13038becb99e965e35afe4 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Wed, 16 Oct 2019 14:12:52 +1100 Subject: [PATCH 08/11] Issue #4188 Spin in close of GzipHandler (#4198) * Issue #4188 Spin in close of GzipHandler Cleanup and simplify code Signed-off-by: Greg Wilkins * Issue #4188 Spin in close of GzipHandler Fix slice code. Added unit test for it. Signed-off-by: Greg Wilkins * Issue #4188 Spin in close of GzipHandler Fixed last slice. Signed-off-by: Greg Wilkins * cleanup from review Signed-off-by: Greg Wilkins --- .../gzip/GzipHttpOutputInterceptor.java | 171 ++++++++++-------- .../jetty/servlet/GzipHandlerTest.java | 67 +++++++ .../org/eclipse/jetty/util/BufferUtil.java | 14 ++ 3 files changed, 176 insertions(+), 76 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHttpOutputInterceptor.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHttpOutputInterceptor.java index 4a582348277..2589f99c0b2 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHttpOutputInterceptor.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHttpOutputInterceptor.java @@ -128,20 +128,8 @@ public class GzipHttpOutputInterceptor implements HttpOutput.Interceptor private void addTrailer() { - int i = _buffer.limit(); - _buffer.limit(i + 8); - - int v = (int)_crc.getValue(); - _buffer.put(i++, (byte)(v & 0xFF)); - _buffer.put(i++, (byte)((v >>> 8) & 0xFF)); - _buffer.put(i++, (byte)((v >>> 16) & 0xFF)); - _buffer.put(i++, (byte)((v >>> 24) & 0xFF)); - - v = _deflater.getTotalIn(); - _buffer.put(i++, (byte)(v & 0xFF)); - _buffer.put(i++, (byte)((v >>> 8) & 0xFF)); - _buffer.put(i++, (byte)((v >>> 16) & 0xFF)); - _buffer.put(i++, (byte)((v >>> 24) & 0xFF)); + BufferUtil.putIntLittleEndian(_buffer, (int)_crc.getValue()); + BufferUtil.putIntLittleEndian(_buffer, _deflater.getTotalIn()); } private void gzip(ByteBuffer content, boolean complete, final Callback callback) @@ -231,8 +219,6 @@ public class GzipHttpOutputInterceptor implements HttpOutput.Interceptor fields.put(GZIP._contentEncoding); _crc.reset(); - _buffer = _channel.getByteBufferPool().acquire(_bufferSize, false); - BufferUtil.fill(_buffer, GZIP_HEADER, 0, GZIP_HEADER.length); // Adjust headers response.setContentLength(-1); @@ -325,82 +311,115 @@ public class GzipHttpOutputInterceptor implements HttpOutput.Interceptor @Override protected Action process() throws Exception { + // If we have no deflator if (_deflater == null) - return Action.SUCCEEDED; - - if (_deflater.needsInput()) { - if (BufferUtil.isEmpty(_content)) + // then the trailer has been generated and written below. + // we have finished compressing the entire content, so + // cleanup and succeed. + if (_buffer != null) { - if (_deflater.finished()) - { - _factory.recycle(_deflater); - _deflater = null; - _channel.getByteBufferPool().release(_buffer); - _buffer = null; - if (_copy != null) - { - _channel.getByteBufferPool().release(_copy); - _copy = null; - } - return Action.SUCCEEDED; - } - - if (!_last) - { - return Action.SUCCEEDED; - } - - _deflater.finish(); + _channel.getByteBufferPool().release(_buffer); + _buffer = null; } - else if (_content.hasArray()) + if (_copy != null) { - byte[] array = _content.array(); - int off = _content.arrayOffset() + _content.position(); - int len = _content.remaining(); - BufferUtil.clear(_content); - - _crc.update(array, off, len); - _deflater.setInput(array, off, len); - if (_last) - _deflater.finish(); - } - else - { - if (_copy == null) - _copy = _channel.getByteBufferPool().acquire(_bufferSize, false); - BufferUtil.clearToFill(_copy); - int took = BufferUtil.put(_content, _copy); - BufferUtil.flipToFlush(_copy, 0); - if (took == 0) - throw new IllegalStateException(); - - byte[] array = _copy.array(); - int off = _copy.arrayOffset() + _copy.position(); - int len = _copy.remaining(); - - _crc.update(array, off, len); - _deflater.setInput(array, off, len); - if (_last && BufferUtil.isEmpty(_content)) - _deflater.finish(); + _channel.getByteBufferPool().release(_copy); + _copy = null; } + return Action.SUCCEEDED; } - BufferUtil.compact(_buffer); - int off = _buffer.arrayOffset() + _buffer.limit(); - int len = _buffer.capacity() - _buffer.limit() - (_last ? 8 : 0); - if (len > 0) + // If we have no buffer + if (_buffer == null) { + // allocate a buffer and add the gzip header + _buffer = _channel.getByteBufferPool().acquire(_bufferSize, false); + BufferUtil.fill(_buffer, GZIP_HEADER, 0, GZIP_HEADER.length); + } + else + { + // otherwise clear the buffer as previous writes will always fully consume. + BufferUtil.clear(_buffer); + } + + // If the deflator is not finished, then compress more data + if (!_deflater.finished()) + { + if (_deflater.needsInput()) + { + // if there is no more content available to compress + // then we are either finished all content or just the current write. + if (BufferUtil.isEmpty(_content)) + { + if (_last) + _deflater.finish(); + else + return Action.SUCCEEDED; + } + else + { + // If there is more content available to compress, we have to make sure + // it is available in an array for the current deflator API, maybe slicing + // of content. + ByteBuffer slice; + if (_content.hasArray()) + slice = _content; + else + { + if (_copy == null) + _copy = _channel.getByteBufferPool().acquire(_bufferSize, false); + else + BufferUtil.clear(_copy); + slice = _copy; + BufferUtil.append(_copy, _content); + } + + // transfer the data from the slice to the the deflator + byte[] array = slice.array(); + int off = slice.arrayOffset() + slice.position(); + int len = slice.remaining(); + _crc.update(array, off, len); + _deflater.setInput(array, off, len); // TODO use ByteBuffer API in Jetty-10 + BufferUtil.clear(slice); + if (_last && BufferUtil.isEmpty(_content)) + _deflater.finish(); + } + } + + // deflate the content into the available space in the buffer + int off = _buffer.arrayOffset() + _buffer.limit(); + int len = BufferUtil.space(_buffer); int produced = _deflater.deflate(_buffer.array(), off, len, _syncFlush ? Deflater.SYNC_FLUSH : Deflater.NO_FLUSH); _buffer.limit(_buffer.limit() + produced); } - boolean finished = _deflater.finished(); - if (finished) + // If we have finished deflation and there is room for the trailer. + if (_deflater.finished() && BufferUtil.space(_buffer) >= 8) + { + // add the trailer and recycle the deflator to flag that we will have had completeSuccess when + // the write below completes. addTrailer(); + _factory.recycle(_deflater); + _deflater = null; + } - _interceptor.write(_buffer, finished, this); + // write the compressed buffer. + _interceptor.write(_buffer, _deflater == null, this); return Action.SCHEDULED; } + + @Override + public String toString() + { + return String.format("%s[content=%s last=%b copy=%s buffer=%s deflate=%s", + super.toString(), + BufferUtil.toDetailString(_content), + _last, + BufferUtil.toDetailString(_copy), + BufferUtil.toDetailString(_buffer), + _deflater, + _deflater != null && _deflater.finished() ? "(finished)" : ""); + } } } diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/GzipHandlerTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/GzipHandlerTest.java index ba7495fee1f..e7c3c6118c0 100644 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/GzipHandlerTest.java +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/GzipHandlerTest.java @@ -45,9 +45,11 @@ import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import org.eclipse.jetty.http.HttpTester; +import org.eclipse.jetty.server.HttpOutput; import org.eclipse.jetty.server.LocalConnector; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.handler.gzip.GzipHandler; +import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.IO; import org.hamcrest.Matchers; import org.junit.jupiter.api.AfterEach; @@ -116,6 +118,7 @@ public class GzipHandlerTest servlets.addServletWithMapping(EchoServlet.class, "/echo/*"); servlets.addServletWithMapping(DumpServlet.class, "/dump/*"); servlets.addServletWithMapping(AsyncServlet.class, "/async/*"); + servlets.addServletWithMapping(BufferServlet.class, "/buffer/*"); servlets.addFilterWithMapping(CheckFilter.class, "/*", EnumSet.of(DispatcherType.REQUEST)); _server.start(); @@ -221,6 +224,19 @@ public class GzipHandlerTest } } + public static class BufferServlet extends HttpServlet + { + @Override + protected void doGet(HttpServletRequest req, HttpServletResponse response) throws ServletException, IOException + { + HttpOutput out = (HttpOutput)response.getOutputStream(); + ByteBuffer buffer = BufferUtil.toBuffer(__bytes).asReadOnlyBuffer(); + response.setContentLength(buffer.remaining()); + response.setContentType("text/plain"); + out.write(buffer); + } + } + public static class EchoServlet extends HttpServlet { @Override @@ -357,6 +373,32 @@ public class GzipHandlerTest assertEquals(__content, testOut.toString("UTF8")); } + @Test + public void testBufferResponse() throws Exception + { + // generated and parsed test + HttpTester.Request request = HttpTester.newRequest(); + HttpTester.Response response; + + request.setMethod("GET"); + request.setURI("/ctx/buffer/info"); + request.setVersion("HTTP/1.0"); + request.setHeader("Host", "tester"); + request.setHeader("accept-encoding", "gzip"); + + response = HttpTester.parseResponse(_connector.getResponse(request.generate())); + + assertThat(response.getStatus(), is(200)); + assertThat(response.get("Content-Encoding"), Matchers.equalToIgnoringCase("gzip")); + assertThat(response.getCSV("Vary", false), Matchers.contains("Accept-Encoding")); + + InputStream testIn = new GZIPInputStream(new ByteArrayInputStream(response.getContentBytes())); + ByteArrayOutputStream testOut = new ByteArrayOutputStream(); + IO.copy(testIn, testOut); + + assertEquals(__content, testOut.toString("UTF8")); + } + @Test public void testAsyncLargeResponse() throws Exception { @@ -387,6 +429,31 @@ public class GzipHandlerTest assertEquals(__content, new String(Arrays.copyOfRange(bytes,i * __bytes.length, (i + 1) * __bytes.length), StandardCharsets.UTF_8), "chunk " + i); } + @Test + public void testAsyncEmptyResponse() throws Exception + { + int writes = 0; + _server.getChildHandlerByClass(GzipHandler.class).setMinGzipSize(0); + + // generated and parsed test + HttpTester.Request request = HttpTester.newRequest(); + HttpTester.Response response; + + request.setMethod("GET"); + request.setURI("/ctx/async/info?writes=" + writes); + request.setVersion("HTTP/1.0"); + request.setHeader("Host", "tester"); + request.setHeader("accept-encoding", "gzip"); + + response = HttpTester.parseResponse(_connector.getResponse(request.generate())); + + assertThat(response.getStatus(), is(200)); + assertThat(response.get("Content-Encoding"), Matchers.equalToIgnoringCase("gzip")); + assertThat(response.getCSV("Vary", false), Matchers.contains("Accept-Encoding")); + + + } + @Test public void testGzipHandlerWithMultipleAcceptEncodingHeaders() throws Exception { diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/BufferUtil.java b/jetty-util/src/main/java/org/eclipse/jetty/util/BufferUtil.java index 452c1be848c..ee54e2726ed 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/BufferUtil.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/BufferUtil.java @@ -219,6 +219,20 @@ public class BufferUtil buffer.position(position); } + /** Put an integer little endian + * @param buffer The buffer to put to + * @param value The value to put. + */ + public static void putIntLittleEndian(ByteBuffer buffer, int value) + { + int p = flipToFill(buffer); + buffer.put((byte)(value & 0xFF)); + buffer.put((byte)((value >>> 8) & 0xFF)); + buffer.put((byte)((value >>> 16) & 0xFF)); + buffer.put((byte)((value >>> 24) & 0xFF)); + flipToFlush(buffer, p); + } + /** * Convert a ByteBuffer to a byte array. * From 20e7aa01f2057299e75302e7370d8b308c20161c Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Wed, 16 Oct 2019 14:14:49 +1100 Subject: [PATCH 09/11] Issue #4183 Handle null class location for ClasspathPattern. (#4197) Signed-off-by: Jan Bartel --- .../jetty/webapp/ClasspathPattern.java | 36 ++++++++++--- .../jetty/webapp/ClasspathPatternTest.java | 51 +++++++++++++++++++ 2 files changed, 80 insertions(+), 7 deletions(-) diff --git a/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/ClasspathPattern.java b/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/ClasspathPattern.java index bd61bb55b2b..91388128db2 100644 --- a/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/ClasspathPattern.java +++ b/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/ClasspathPattern.java @@ -71,7 +71,7 @@ public class ClasspathPattern extends AbstractSet { private static final Logger LOG = Log.getLogger(ClasspathPattern.class); - private static class Entry + static class Entry { private final String _pattern; private final String _name; @@ -727,21 +727,43 @@ public class ClasspathPattern extends AbstractSet }); } - private static boolean combine(IncludeExcludeSet names, String name, IncludeExcludeSet locations, Supplier location) + /** + * Match a class against inclusions and exclusions by name and location. + * Name based checks are performed before location checks. For a class to match, + * it must not be excluded by either name or location, and must either be explicitly + * included, or for there to be no inclusions. In the case where the location + * of the class is null, it will match if it is included by name, or + * if there are no location exclusions. + * + * @param names configured inclusions and exclusions by name + * @param name the name to check + * @param locations configured inclusions and exclusions by location + * @param location the location of the class (can be null) + * @return true if the class is not excluded but is included, or there are + * no inclusions. False otherwise. + */ + static boolean combine(IncludeExcludeSet names, String name, IncludeExcludeSet locations, Supplier location) { + // check the name set Boolean byName = names.isIncludedAndNotExcluded(name); + + // If we excluded by name, then no match if (Boolean.FALSE == byName) return false; + // check the location set URI uri = location.get(); - if (uri == null) - return locations.isEmpty() || locations.hasExcludes() && !locations.hasIncludes(); + Boolean byLocation = uri == null ? null : locations.isIncludedAndNotExcluded(uri); - Boolean byLocation = locations.isIncludedAndNotExcluded(uri); - if (Boolean.FALSE == byLocation) + // If we excluded by location or couldn't check location exclusion, then no match + if (Boolean.FALSE == byLocation || (locations.hasExcludes() && uri == null)) return false; - return Boolean.TRUE.equals(byName) || Boolean.TRUE.equals(byLocation) || !(names.hasIncludes() || locations.hasIncludes()); + // If there are includes, then we must be included to match. + if (names.hasIncludes() || locations.hasIncludes()) + return byName == Boolean.TRUE || byLocation == Boolean.TRUE; + // Otherwise there are no includes and it was not excluded, so match + return true; } } diff --git a/jetty-webapp/src/test/java/org/eclipse/jetty/webapp/ClasspathPatternTest.java b/jetty-webapp/src/test/java/org/eclipse/jetty/webapp/ClasspathPatternTest.java index cef710c565f..da07429464c 100644 --- a/jetty-webapp/src/test/java/org/eclipse/jetty/webapp/ClasspathPatternTest.java +++ b/jetty-webapp/src/test/java/org/eclipse/jetty/webapp/ClasspathPatternTest.java @@ -20,8 +20,13 @@ package org.eclipse.jetty.webapp; import java.net.URI; import java.util.Arrays; +import java.util.function.Supplier; +import org.eclipse.jetty.util.IncludeExcludeSet; import org.eclipse.jetty.util.TypeUtil; +import org.eclipse.jetty.webapp.ClasspathPattern.ByLocationOrModule; +import org.eclipse.jetty.webapp.ClasspathPattern.ByPackageOrName; +import org.eclipse.jetty.webapp.ClasspathPattern.Entry; import org.hamcrest.Matchers; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -36,6 +41,14 @@ import static org.junit.jupiter.api.Assertions.assertTrue; public class ClasspathPatternTest { private final ClasspathPattern _pattern = new ClasspathPattern(); + + protected static Supplier NULL_SUPPLIER = new Supplier() + { + public URI get() + { + return null; + } + }; @BeforeEach public void before() @@ -262,6 +275,44 @@ public class ClasspathPatternTest assertThat(pattern.match(Test.class), Matchers.is(false)); assertThat(pattern.match(ClasspathPatternTest.class), Matchers.is(false)); } + + @Test + public void testWithNullLocation() throws Exception + { + ClasspathPattern pattern = new ClasspathPattern(); + + IncludeExcludeSet names = new IncludeExcludeSet<>(ByPackageOrName.class); + IncludeExcludeSet locations = new IncludeExcludeSet<>(ByLocationOrModule.class); + + //Test no name or location includes or excludes - should match + assertThat(ClasspathPattern.combine(names, "a.b.c", locations, NULL_SUPPLIER), Matchers.is(true)); + + names.include(pattern.newEntry("a.b.", true)); + names.exclude(pattern.newEntry("d.e.", false)); + + //Test explicit include by name no locations - should match + assertThat(ClasspathPattern.combine(names, "a.b.c", locations, NULL_SUPPLIER), Matchers.is(true)); + + //Test explicit exclude by name no locations - should not match + assertThat(ClasspathPattern.combine(names, "d.e.f", locations, NULL_SUPPLIER), Matchers.is(false)); + + //Test include by name with location includes - should match + locations.include(pattern.newEntry("file:/foo/bar", true)); + assertThat(ClasspathPattern.combine(names, "a.b.c", locations, NULL_SUPPLIER), Matchers.is(true)); + + //Test include by name but with location exclusions - should not match + locations.clear(); + locations.exclude(pattern.newEntry("file:/high/low", false)); + assertThat(ClasspathPattern.combine(names, "a.b.c", locations, NULL_SUPPLIER), Matchers.is(false)); + + //Test neither included or excluded by name, but with location exclusions - should not match + assertThat(ClasspathPattern.combine(names, "g.b.r", locations, NULL_SUPPLIER), Matchers.is(false)); + + //Test neither included nor excluded by name, but with location inclusions - should not match + locations.clear(); + locations.include(pattern.newEntry("file:/foo/bar", true)); + assertThat(ClasspathPattern.combine(names, "g.b.r", locations, NULL_SUPPLIER), Matchers.is(false)); + } @Test public void testLarge() From 4769de8a2b5129b4745d1b9af465a1195ca895ac Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Wed, 16 Oct 2019 13:02:24 +0200 Subject: [PATCH 10/11] Issue #4209 - Unused TLS connection is not closed in Java 11. Code cleanup. Signed-off-by: Simone Bordet --- .../jetty/client/HttpClientTLSTest.java | 46 +++++++-------- .../io/ssl/SslClientConnectionFactory.java | 36 ++++++++++-- .../eclipse/jetty/io/ssl/SslConnection.java | 57 +++++++++++++++---- 3 files changed, 99 insertions(+), 40 deletions(-) diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTLSTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTLSTest.java index d5c17170c12..dbfbdd93669 100644 --- a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTLSTest.java +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTLSTest.java @@ -67,8 +67,6 @@ public class HttpClientTLSTest private ServerConnector connector; private HttpClient client; - private SSLSocket sslSocket; - private void startServer(SslContextFactory sslContextFactory, Handler handler) throws Exception { ExecutorThreadPool serverThreads = new ExecutorThreadPool(); @@ -420,16 +418,16 @@ public class HttpClientTLSTest String host = "localhost"; int port = connector.getLocalPort(); - Socket socket = new Socket(host, port); - sslSocket = (SSLSocket)clientTLSFactory.getSslContext().getSocketFactory().createSocket(socket, host, port, true); + Socket socket1 = new Socket(host, port); + SSLSocket sslSocket1 = (SSLSocket)clientTLSFactory.getSslContext().getSocketFactory().createSocket(socket1, host, port, true); CountDownLatch handshakeLatch1 = new CountDownLatch(1); AtomicReference session1 = new AtomicReference<>(); - sslSocket.addHandshakeCompletedListener(event -> + sslSocket1.addHandshakeCompletedListener(event -> { session1.set(event.getSession().getId()); handshakeLatch1.countDown(); }); - sslSocket.startHandshake(); + sslSocket1.startHandshake(); assertTrue(handshakeLatch1.await(5, TimeUnit.SECONDS)); // In TLS 1.3 the server sends a NewSessionTicket post-handshake message @@ -437,29 +435,29 @@ public class HttpClientTLSTest assertThrows(SocketTimeoutException.class, () -> { - sslSocket.setSoTimeout(1000); - sslSocket.getInputStream().read(); + sslSocket1.setSoTimeout(1000); + sslSocket1.getInputStream().read(); }); // The client closes abruptly. - socket.close(); + socket1.close(); // Try again and compare the session ids. - socket = new Socket(host, port); - sslSocket = (SSLSocket)clientTLSFactory.getSslContext().getSocketFactory().createSocket(socket, host, port, true); + Socket socket2 = new Socket(host, port); + SSLSocket sslSocket2 = (SSLSocket)clientTLSFactory.getSslContext().getSocketFactory().createSocket(socket2, host, port, true); CountDownLatch handshakeLatch2 = new CountDownLatch(1); AtomicReference session2 = new AtomicReference<>(); - sslSocket.addHandshakeCompletedListener(event -> + sslSocket2.addHandshakeCompletedListener(event -> { session2.set(event.getSession().getId()); handshakeLatch2.countDown(); }); - sslSocket.startHandshake(); + sslSocket2.startHandshake(); assertTrue(handshakeLatch2.await(5, TimeUnit.SECONDS)); assertArrayEquals(session1.get(), session2.get()); - sslSocket.close(); + sslSocket2.close(); } @Test @@ -477,7 +475,7 @@ public class HttpClientTLSTest protected ClientConnectionFactory newSslClientConnectionFactory(SslContextFactory sslContextFactory, ClientConnectionFactory connectionFactory) { SslClientConnectionFactory ssl = (SslClientConnectionFactory)super.newSslClientConnectionFactory(sslContextFactory, connectionFactory); - ssl.setAllowMissingCloseMessage(false); + ssl.setRequireCloseMessage(true); return ssl; } }; @@ -505,19 +503,19 @@ public class HttpClientTLSTest break; } - // If the response is Content-Length delimited, allowing the - // missing TLS Close Message is fine because the application - // will see a EOFException anyway. - // If the response is connection delimited, allowing the - // missing TLS Close Message is bad because the application - // will see a successful response with truncated content. + // If the response is Content-Length delimited, the lack of + // the TLS Close Message is fine because the application + // will see a EOFException anyway: the Content-Length and + // the actual content bytes count won't match. + // If the response is connection delimited, the lack of the + // TLS Close Message is bad because the application will + // see a successful response, but with truncated content. - // Verify that by not allowing the missing - // TLS Close Message we get a response failure. + // Verify that by requiring the TLS Close Message we get + // a response failure. byte[] half = new byte[8]; String response = "HTTP/1.1 200 OK\r\n" + -// "Content-Length: " + (half.length * 2) + "\r\n" + "Connection: close\r\n" + "\r\n"; OutputStream output = sslSocket.getOutputStream(); diff --git a/jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslClientConnectionFactory.java b/jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslClientConnectionFactory.java index 236d4397f78..cfe960bd56f 100644 --- a/jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslClientConnectionFactory.java +++ b/jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslClientConnectionFactory.java @@ -47,7 +47,7 @@ public class SslClientConnectionFactory implements ClientConnectionFactory private final ClientConnectionFactory connectionFactory; private boolean _directBuffersForEncryption = true; private boolean _directBuffersForDecryption = true; - private boolean allowMissingCloseMessage = true; + private boolean _requireCloseMessage; public SslClientConnectionFactory(SslContextFactory sslContextFactory, ByteBufferPool byteBufferPool, Executor executor, ClientConnectionFactory connectionFactory) { @@ -77,14 +77,42 @@ public class SslClientConnectionFactory implements ClientConnectionFactory return _directBuffersForEncryption; } + /** + * @return whether is not required that peers send the TLS {@code close_notify} message + * @deprecated use {@link #isRequireCloseMessage()} instead + */ + @Deprecated public boolean isAllowMissingCloseMessage() { - return allowMissingCloseMessage; + return !isRequireCloseMessage(); } + /** + * @param allowMissingCloseMessage whether is not required that peers send the TLS {@code close_notify} message + * @deprecated use {@link #setRequireCloseMessage(boolean)} instead + */ + @Deprecated public void setAllowMissingCloseMessage(boolean allowMissingCloseMessage) { - this.allowMissingCloseMessage = allowMissingCloseMessage; + setRequireCloseMessage(!allowMissingCloseMessage); + } + + /** + * @return whether peers must send the TLS {@code close_notify} message + * @see SslConnection#isRequireCloseMessage() + */ + public boolean isRequireCloseMessage() + { + return _requireCloseMessage; + } + + /** + * @param requireCloseMessage whether peers must send the TLS {@code close_notify} message + * @see SslConnection#setRequireCloseMessage(boolean) + */ + public void setRequireCloseMessage(boolean requireCloseMessage) + { + _requireCloseMessage = requireCloseMessage; } @Override @@ -120,7 +148,7 @@ public class SslClientConnectionFactory implements ClientConnectionFactory SslConnection sslConnection = (SslConnection)connection; sslConnection.setRenegotiationAllowed(sslContextFactory.isRenegotiationAllowed()); sslConnection.setRenegotiationLimit(sslContextFactory.getRenegotiationLimit()); - sslConnection.setAllowMissingCloseMessage(isAllowMissingCloseMessage()); + sslConnection.setRequireCloseMessage(isRequireCloseMessage()); ContainerLifeCycle connector = (ContainerLifeCycle)context.get(ClientConnectionFactory.CONNECTOR_CONTEXT_KEY); connector.getBeans(SslHandshakeListener.class).forEach(sslConnection::addHandshakeListener); } diff --git a/jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslConnection.java b/jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslConnection.java index 6e61f5ad451..5dd6da588b1 100644 --- a/jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslConnection.java +++ b/jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslConnection.java @@ -113,7 +113,7 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr private boolean _renegotiationAllowed; private int _renegotiationLimit = -1; private boolean _closedOutbound; - private boolean _allowMissingCloseMessage = true; + private boolean _requireCloseMessage; private FlushState _flushState = FlushState.IDLE; private FillState _fillState = FillState.IDLE; private AtomicReference _handshake = new AtomicReference<>(Handshake.INITIAL); @@ -231,7 +231,7 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr } /** - * @return The number of renegotions allowed for this connection. When the limit + * @return The number of renegotiations allowed for this connection. When the limit * is 0 renegotiation will be denied. If the limit is less than 0 then no limit is applied. */ public int getRenegotiationLimit() @@ -240,7 +240,7 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr } /** - * @param renegotiationLimit The number of renegotions allowed for this connection. + * @param renegotiationLimit The number of renegotiations allowed for this connection. * When the limit is 0 renegotiation will be denied. If the limit is less than 0 then no limit is applied. * Default -1. */ @@ -249,14 +249,46 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr _renegotiationLimit = renegotiationLimit; } + /** + * @return whether is not required that peers send the TLS {@code close_notify} message + * @deprecated use inverted {@link #isRequireCloseMessage()} instead + */ + @Deprecated public boolean isAllowMissingCloseMessage() { - return _allowMissingCloseMessage; + return !isRequireCloseMessage(); } + /** + * @param allowMissingCloseMessage whether is not required that peers send the TLS {@code close_notify} message + * @deprecated use inverted {@link #setRequireCloseMessage(boolean)} instead + */ + @Deprecated public void setAllowMissingCloseMessage(boolean allowMissingCloseMessage) { - this._allowMissingCloseMessage = allowMissingCloseMessage; + setRequireCloseMessage(!allowMissingCloseMessage); + } + + /** + * @return whether peers must send the TLS {@code close_notify} message + */ + public boolean isRequireCloseMessage() + { + return _requireCloseMessage; + } + + /** + *

Sets whether it is required that a peer send the TLS {@code close_notify} message + * to indicate the will to close the connection, otherwise it may be interpreted as a + * truncation attack.

+ *

This option is only useful on clients, since typically servers cannot accept + * connection-delimited content that may be truncated.

+ * + * @param requireCloseMessage whether peers must send the TLS {@code close_notify} message + */ + public void setRequireCloseMessage(boolean requireCloseMessage) + { + _requireCloseMessage = requireCloseMessage; } private void acquireEncryptedInput() @@ -1096,15 +1128,15 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr @Override public void doShutdownOutput() { - final EndPoint endp = getEndPoint(); + EndPoint endPoint = getEndPoint(); try { boolean close; boolean flush = false; synchronized (_decryptedEndPoint) { - boolean ishut = endp.isInputShutdown(); - boolean oshut = endp.isOutputShutdown(); + boolean ishut = endPoint.isInputShutdown(); + boolean oshut = endPoint.isOutputShutdown(); if (LOG.isDebugEnabled()) LOG.debug("shutdownOutput: {} oshut={}, ishut={}", SslConnection.this, oshut, ishut); @@ -1128,19 +1160,19 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr // let's just flush the encrypted output in the background. ByteBuffer write = _encryptedOutput; if (BufferUtil.hasContent(write)) - endp.write(Callback.from(Callback.NOOP::succeeded, t -> endp.close()), write); + endPoint.write(Callback.from(Callback.NOOP::succeeded, t -> endPoint.close()), write); } } if (close) - endp.close(); + endPoint.close(); else ensureFillInterested(); } catch (Throwable x) { LOG.ignore(x); - endp.close(); + endPoint.close(); } } @@ -1152,7 +1184,8 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr } catch (Throwable x) { - LOG.ignore(x); + if (LOG.isDebugEnabled()) + LOG.debug(x); } } From 1e360244a59d3ddb135fab4557b021606feb1c52 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Wed, 16 Oct 2019 13:10:40 +0200 Subject: [PATCH 11/11] Fixes #4209 - Unused TLS connection is not closed in Java 11. Added workarounds for the Java 11 behavior. In fill(), call closeInbound() if we filled -1 and the handshake did not start yet. This avoids to send a ClientHello to the peer even if we are closing. In flush(), if the handshake status is NEED_UNWRAP but we are closing, force a wrap(). Added test cases. Signed-off-by: Simone Bordet --- .../jetty/client/HttpClientTLSTest.java | 205 ++++++++++++++++++ .../eclipse/jetty/io/ssl/SslConnection.java | 69 +++++- 2 files changed, 262 insertions(+), 12 deletions(-) diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTLSTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTLSTest.java index dbfbdd93669..145f92346e2 100644 --- a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTLSTest.java +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTLSTest.java @@ -19,17 +19,22 @@ package org.eclipse.jetty.client; import java.io.BufferedReader; +import java.io.IOException; import java.io.InputStreamReader; import java.io.OutputStream; import java.net.ServerSocket; import java.net.Socket; import java.net.SocketTimeoutException; +import java.nio.ByteBuffer; import java.nio.charset.StandardCharsets; import java.util.Arrays; import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutionException; +import java.util.concurrent.Executor; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.atomic.AtomicReference; +import javax.net.ssl.SSLEngine; import javax.net.ssl.SSLException; import javax.net.ssl.SSLPeerUnverifiedException; import javax.net.ssl.SSLSocket; @@ -38,12 +43,20 @@ import org.eclipse.jetty.client.api.ContentResponse; import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.HttpScheme; import org.eclipse.jetty.http.HttpStatus; +import org.eclipse.jetty.io.ByteBufferPool; import org.eclipse.jetty.io.ClientConnectionFactory; +import org.eclipse.jetty.io.EndPoint; import org.eclipse.jetty.io.ssl.SslClientConnectionFactory; +import org.eclipse.jetty.io.ssl.SslConnection; import org.eclipse.jetty.io.ssl.SslHandshakeListener; +import org.eclipse.jetty.server.Connector; import org.eclipse.jetty.server.Handler; +import org.eclipse.jetty.server.HttpConfiguration; +import org.eclipse.jetty.server.HttpConnectionFactory; +import org.eclipse.jetty.server.SecureRequestCustomizer; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.ServerConnector; +import org.eclipse.jetty.server.SslConnectionFactory; import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.ssl.SslContextFactory; import org.eclipse.jetty.util.thread.ExecutorThreadPool; @@ -555,4 +568,196 @@ public class HttpClientTLSTest assertTrue(latch.await(5, TimeUnit.SECONDS)); } + + @Test + public void testNeverUsedConnectionThenServerIdleTimeout() throws Exception + { + long idleTimeout = 2000; + + SslContextFactory serverTLSFactory = createServerSslContextFactory(); + QueuedThreadPool serverThreads = new QueuedThreadPool(); + serverThreads.setName("server"); + server = new Server(serverThreads); + HttpConfiguration httpConfig = new HttpConfiguration(); + httpConfig.addCustomizer(new SecureRequestCustomizer()); + HttpConnectionFactory http = new HttpConnectionFactory(httpConfig); + AtomicLong serverBytes = new AtomicLong(); + SslConnectionFactory ssl = new SslConnectionFactory(serverTLSFactory, http.getProtocol()) + { + @Override + protected SslConnection newSslConnection(Connector connector, EndPoint endPoint, SSLEngine engine) + { + return new SslConnection(connector.getByteBufferPool(), connector.getExecutor(), endPoint, engine, isDirectBuffersForEncryption(), isDirectBuffersForDecryption()) + { + @Override + protected int networkFill(ByteBuffer input) throws IOException + { + int n = super.networkFill(input); + if (n > 0) + serverBytes.addAndGet(n); + return n; + } + }; + } + }; + connector = new ServerConnector(server, 1, 1, ssl, http); + connector.setIdleTimeout(idleTimeout); + server.addConnector(connector); + server.setHandler(new EmptyServerHandler()); + server.start(); + + SslContextFactory clientTLSFactory = createClientSslContextFactory(); + QueuedThreadPool clientThreads = new QueuedThreadPool(); + clientThreads.setName("client"); + AtomicLong clientBytes = new AtomicLong(); + client = new HttpClient(clientTLSFactory) + { + @Override + protected ClientConnectionFactory newSslClientConnectionFactory(SslContextFactory sslContextFactory, ClientConnectionFactory connectionFactory) + { + if (sslContextFactory == null) + sslContextFactory = getSslContextFactory(); + return new SslClientConnectionFactory(sslContextFactory, getByteBufferPool(), getExecutor(), connectionFactory) + { + @Override + protected SslConnection newSslConnection(ByteBufferPool byteBufferPool, Executor executor, EndPoint endPoint, SSLEngine engine) + { + return new SslConnection(byteBufferPool, executor, endPoint, engine, isDirectBuffersForEncryption(), isDirectBuffersForDecryption()) + { + @Override + protected int networkFill(ByteBuffer input) throws IOException + { + int n = super.networkFill(input); + if (n > 0) + clientBytes.addAndGet(n); + return n; + } + }; + } + }; + } + }; + client.setExecutor(clientThreads); + client.start(); + + // Create a connection but don't use it. + String scheme = HttpScheme.HTTPS.asString(); + String host = "localhost"; + int port = connector.getLocalPort(); + HttpDestination destination = (HttpDestination)client.getDestination(scheme, host, port); + DuplexConnectionPool connectionPool = (DuplexConnectionPool)destination.getConnectionPool(); + // Trigger the creation of a new connection, but don't use it. + connectionPool.tryCreate(-1); + // Verify that the connection has been created. + while (true) + { + Thread.sleep(50); + if (connectionPool.getConnectionCount() == 1) + break; + } + + // Wait for the server to idle timeout the connection. + Thread.sleep(idleTimeout + idleTimeout / 2); + + // The connection should be gone from the connection pool. + assertEquals(0, connectionPool.getConnectionCount(), connectionPool.dump()); + assertEquals(0, serverBytes.get()); + assertEquals(0, clientBytes.get()); + } + + @Test + public void testNeverUsedConnectionThenClientIdleTimeout() throws Exception + { + SslContextFactory serverTLSFactory = createServerSslContextFactory(); + QueuedThreadPool serverThreads = new QueuedThreadPool(); + serverThreads.setName("server"); + server = new Server(serverThreads); + HttpConfiguration httpConfig = new HttpConfiguration(); + httpConfig.addCustomizer(new SecureRequestCustomizer()); + HttpConnectionFactory http = new HttpConnectionFactory(httpConfig); + AtomicLong serverBytes = new AtomicLong(); + SslConnectionFactory ssl = new SslConnectionFactory(serverTLSFactory, http.getProtocol()) + { + @Override + protected SslConnection newSslConnection(Connector connector, EndPoint endPoint, SSLEngine engine) + { + return new SslConnection(connector.getByteBufferPool(), connector.getExecutor(), endPoint, engine, isDirectBuffersForEncryption(), isDirectBuffersForDecryption()) + { + @Override + protected int networkFill(ByteBuffer input) throws IOException + { + int n = super.networkFill(input); + if (n > 0) + serverBytes.addAndGet(n); + return n; + } + }; + } + }; + connector = new ServerConnector(server, 1, 1, ssl, http); + server.addConnector(connector); + server.setHandler(new EmptyServerHandler()); + server.start(); + + long idleTimeout = 2000; + + SslContextFactory clientTLSFactory = createClientSslContextFactory(); + QueuedThreadPool clientThreads = new QueuedThreadPool(); + clientThreads.setName("client"); + AtomicLong clientBytes = new AtomicLong(); + client = new HttpClient(clientTLSFactory) + { + @Override + protected ClientConnectionFactory newSslClientConnectionFactory(SslContextFactory sslContextFactory, ClientConnectionFactory connectionFactory) + { + if (sslContextFactory == null) + sslContextFactory = getSslContextFactory(); + return new SslClientConnectionFactory(sslContextFactory, getByteBufferPool(), getExecutor(), connectionFactory) + { + @Override + protected SslConnection newSslConnection(ByteBufferPool byteBufferPool, Executor executor, EndPoint endPoint, SSLEngine engine) + { + return new SslConnection(byteBufferPool, executor, endPoint, engine, isDirectBuffersForEncryption(), isDirectBuffersForDecryption()) + { + @Override + protected int networkFill(ByteBuffer input) throws IOException + { + int n = super.networkFill(input); + if (n > 0) + clientBytes.addAndGet(n); + return n; + } + }; + } + }; + } + }; + client.setIdleTimeout(idleTimeout); + client.setExecutor(clientThreads); + client.start(); + + // Create a connection but don't use it. + String scheme = HttpScheme.HTTPS.asString(); + String host = "localhost"; + int port = connector.getLocalPort(); + HttpDestination destination = (HttpDestination)client.getDestination(scheme, host, port); + DuplexConnectionPool connectionPool = (DuplexConnectionPool)destination.getConnectionPool(); + // Trigger the creation of a new connection, but don't use it. + connectionPool.tryCreate(-1); + // Verify that the connection has been created. + while (true) + { + Thread.sleep(50); + if (connectionPool.getConnectionCount() == 1) + break; + } + + // Wait for the client to idle timeout the connection. + Thread.sleep(idleTimeout + idleTimeout / 2); + + // The connection should be gone from the connection pool. + assertEquals(0, connectionPool.getConnectionCount(), connectionPool.dump()); + assertEquals(0, serverBytes.get()); + assertEquals(0, clientBytes.get()); + } } diff --git a/jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslConnection.java b/jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslConnection.java index 5dd6da588b1..8d76d742dfa 100644 --- a/jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslConnection.java +++ b/jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslConnection.java @@ -80,9 +80,10 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr private static final Logger LOG = Log.getLogger(SslConnection.class); private static final String TLS_1_3 = "TLSv1.3"; - private enum Handshake + private enum HandshakeState { INITIAL, + HANDSHAKE, SUCCEEDED, FAILED } @@ -116,7 +117,7 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr private boolean _requireCloseMessage; private FlushState _flushState = FlushState.IDLE; private FillState _fillState = FillState.IDLE; - private AtomicReference _handshake = new AtomicReference<>(Handshake.INITIAL); + private AtomicReference _handshake = new AtomicReference<>(HandshakeState.INITIAL); private boolean _underflown; private abstract class RunnableTask implements Runnable, Invocable @@ -291,6 +292,22 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr _requireCloseMessage = requireCloseMessage; } + private boolean isHandshakeInitial() + { + return _handshake.get() == HandshakeState.INITIAL; + } + + private boolean isHandshakeSucceeded() + { + return _handshake.get() == HandshakeState.SUCCEEDED; + } + + private boolean isHandshakeComplete() + { + HandshakeState state = _handshake.get(); + return state == HandshakeState.SUCCEEDED || state == HandshakeState.FAILED; + } + private void acquireEncryptedInput() { if (_encryptedInput == null) @@ -393,6 +410,16 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr } } + protected int networkFill(ByteBuffer input) throws IOException + { + return getEndPoint().fill(input); + } + + protected boolean networkFlush(ByteBuffer output) throws IOException + { + return getEndPoint().flush(output); + } + public class DecryptedEndPoint extends AbstractEndPoint { private final Callback _incompleteWriteCallback = new IncompleteWriteCallback(); @@ -590,14 +617,23 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr } // Let's try reading some encrypted data... even if we have some already. - int netFilled = getEndPoint().fill(_encryptedInput); - + int netFilled = networkFill(_encryptedInput); if (LOG.isDebugEnabled()) LOG.debug("net filled={}", netFilled); - if (netFilled > 0 && _handshake.get() == Handshake.INITIAL && isOutboundDone()) + // Workaround for Java 11 behavior. + if (netFilled < 0 && isHandshakeInitial() && BufferUtil.isEmpty(_encryptedInput)) + closeInbound(); + + if (netFilled > 0 && !isHandshakeComplete() && isOutboundDone()) throw new SSLHandshakeException("Closed during handshake"); + if (_handshake.compareAndSet(HandshakeState.INITIAL, HandshakeState.HANDSHAKE)) + { + if (LOG.isDebugEnabled()) + LOG.debug("fill starting handshake {}", SslConnection.this); + } + // Let's unwrap even if we have no net data because in that // case we want to fall through to the handshake handling int pos = BufferUtil.flipToFill(appIn); @@ -803,7 +839,7 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr private void handshakeSucceeded() throws SSLException { - if (_handshake.compareAndSet(Handshake.INITIAL, Handshake.SUCCEEDED)) + if (_handshake.compareAndSet(HandshakeState.HANDSHAKE, HandshakeState.SUCCEEDED)) { if (LOG.isDebugEnabled()) LOG.debug("handshake succeeded {} {} {}/{}", SslConnection.this, @@ -811,7 +847,7 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr _sslEngine.getSession().getProtocol(), _sslEngine.getSession().getCipherSuite()); notifyHandshakeSucceeded(_sslEngine); } - else if (_handshake.get() == Handshake.SUCCEEDED) + else if (isHandshakeSucceeded()) { if (_renegotiationLimit > 0) _renegotiationLimit--; @@ -820,7 +856,7 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr private void handshakeFailed(Throwable failure) { - if (_handshake.compareAndSet(Handshake.INITIAL, Handshake.FAILED)) + if (_handshake.compareAndSet(HandshakeState.HANDSHAKE, HandshakeState.FAILED)) { if (LOG.isDebugEnabled()) LOG.debug("handshake failed {} {}", SslConnection.this, failure); @@ -852,7 +888,7 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr } catch (SSLException x) { - if (handshakeStatus == HandshakeStatus.NOT_HANDSHAKING && !isAllowMissingCloseMessage()) + if (handshakeStatus == HandshakeStatus.NOT_HANDSHAKING && isRequireCloseMessage()) throw x; LOG.ignore(x); return x; @@ -882,7 +918,7 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr } // finish of any previous flushes - if (BufferUtil.hasContent(_encryptedOutput) && !getEndPoint().flush(_encryptedOutput)) + if (BufferUtil.hasContent(_encryptedOutput) && !networkFlush(_encryptedOutput)) return false; boolean isEmpty = BufferUtil.isEmpty(appOuts); @@ -910,6 +946,9 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr continue; case NEED_UNWRAP: + // Workaround for Java 11 behavior. + if (isHandshakeInitial() && isOutboundDone()) + break; if (_fillState == FillState.IDLE) { int filled = fill(BufferUtil.EMPTY_BUFFER); @@ -927,6 +966,12 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr if (_encryptedOutput == null) _encryptedOutput = _bufferPool.acquire(_sslEngine.getSession().getPacketBufferSize(), _encryptedDirectBuffers); + if (_handshake.compareAndSet(HandshakeState.INITIAL, HandshakeState.HANDSHAKE)) + { + if (LOG.isDebugEnabled()) + LOG.debug("flush starting handshake {}", SslConnection.this); + } + // We call sslEngine.wrap to try to take bytes from appOut buffers and encrypt them into the _netOut buffer BufferUtil.compact(_encryptedOutput); int pos = BufferUtil.flipToFill(_encryptedOutput); @@ -952,7 +997,7 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr // if we have net bytes, let's try to flush them boolean flushed = true; if (BufferUtil.hasContent(_encryptedOutput)) - flushed = getEndPoint().flush(_encryptedOutput); + flushed = networkFlush(_encryptedOutput); if (LOG.isDebugEnabled()) LOG.debug("net flushed={}, ac={}", flushed, isEmpty); @@ -1291,7 +1336,7 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr private boolean isRenegotiating() { - if (_handshake.get() == Handshake.INITIAL) + if (!isHandshakeComplete()) return false; if (isTLS13()) return false;