From 37f85034422ce86438d52ab5d86f4d675689c8ff Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Wed, 15 Jan 2020 15:31:43 -0600 Subject: [PATCH 1/2] Issue #4383 - Minimal NPE prevention on MultiPart Signed-off-by: Joakim Erdfelt --- .../jetty/http/MultiPartFormInputStream.java | 7 +++++++ .../jetty/http/MultiPartFormInputStreamTest.java | 14 ++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormInputStream.java b/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormInputStream.java index 05c1dbf25e17..08f1e0b96e6e 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormInputStream.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormInputStream.java @@ -397,6 +397,13 @@ public Collection getParsedParts() */ public void deleteParts() { + if (_parts == null) + { + // If we call deleteParts at this point, we are considered CLOSED + _err = new IllegalStateException("CLOSED via call to deleteParts()"); + return; + } + MultiException err = null; for (List parts : _parts.values()) { diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartFormInputStreamTest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartFormInputStreamTest.java index 2308314886cd..2e7062c4c631 100644 --- a/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartFormInputStreamTest.java +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartFormInputStreamTest.java @@ -509,6 +509,20 @@ public void testPartTmpFileDeletion() throws Exception assertThat(stuff.exists(), is(false)); //tmp file was removed after cleanup } + @Test + public void testParseAfterCleanUp() throws Exception + { + final InputStream input = new ByteArrayInputStream(createMultipartRequestString("myFile").getBytes()); + MultipartConfigElement config = new MultipartConfigElement(_dirname, 1024, 1024, 50); + MultiPartFormInputStream mpis = new MultiPartFormInputStream(input, _contentType, config, _tmpDir); + + mpis.deleteParts(); + + // The call to getParts should throw because we have already cleaned up the parts. + Throwable error = assertThrows(IllegalStateException.class, mpis::getParts); + assertThat(error.getMessage(), containsString("CLOSED")); + } + @Test public void testLFOnlyRequest() throws Exception From eba24b6b83579dcf30c1a6625793bc01ced472f1 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Wed, 15 Jan 2020 15:48:51 -0600 Subject: [PATCH 2/2] Issue #4383 - Updates behavior to eliminate ISE Signed-off-by: Joakim Erdfelt --- .../jetty/http/MultiPartFormInputStream.java | 24 ++++++++----------- .../http/MultiPartFormInputStreamTest.java | 8 ++----- 2 files changed, 12 insertions(+), 20 deletions(-) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormInputStream.java b/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormInputStream.java index 08f1e0b96e6e..f1deb18681b7 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormInputStream.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormInputStream.java @@ -60,10 +60,10 @@ public class MultiPartFormInputStream { private static final Logger LOG = Log.getLogger(MultiPartFormInputStream.class); private static final MultiMap EMPTY_MAP = new MultiMap<>(Collections.emptyMap()); + private final MultiMap _parts; private InputStream _in; private MultipartConfigElement _config; private String _contentType; - private MultiMap _parts; private Throwable _err; private File _tmpDir; private File _contextTmpDir; @@ -341,16 +341,19 @@ public MultiPartFormInputStream(InputStream in, String contentType, MultipartCon if (_config == null) _config = new MultipartConfigElement(_contextTmpDir.getAbsolutePath()); + MultiMap parts = new MultiMap(); + if (in instanceof ServletInputStream) { if (((ServletInputStream)in).isFinished()) { - _parts = EMPTY_MAP; + parts = EMPTY_MAP; _parsed = true; - return; } } - _in = new BufferedInputStream(in); + if (!_parsed) + _in = new BufferedInputStream(in); + _parts = parts; } /** @@ -397,13 +400,6 @@ public Collection getParsedParts() */ public void deleteParts() { - if (_parts == null) - { - // If we call deleteParts at this point, we are considered CLOSED - _err = new IllegalStateException("CLOSED via call to deleteParts()"); - return; - } - MultiException err = null; for (List parts : _parts.values()) { @@ -439,6 +435,9 @@ public Collection getParts() throws IOException parse(); throwIfError(); + if (_parts.isEmpty()) + return Collections.emptyList(); + Collection> values = _parts.values(); List parts = new ArrayList<>(); for (List o : values) @@ -499,9 +498,6 @@ protected void parse() Handler handler = new Handler(); try { - // initialize - _parts = new MultiMap<>(); - // if its not a multipart request, don't parse it if (_contentType == null || !_contentType.startsWith("multipart/form-data")) return; diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartFormInputStreamTest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartFormInputStreamTest.java index 2e7062c4c631..f2f96a7a8c21 100644 --- a/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartFormInputStreamTest.java +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartFormInputStreamTest.java @@ -510,17 +510,13 @@ public void testPartTmpFileDeletion() throws Exception } @Test - public void testParseAfterCleanUp() throws Exception + public void testDeleteNPE() { final InputStream input = new ByteArrayInputStream(createMultipartRequestString("myFile").getBytes()); MultipartConfigElement config = new MultipartConfigElement(_dirname, 1024, 1024, 50); MultiPartFormInputStream mpis = new MultiPartFormInputStream(input, _contentType, config, _tmpDir); - mpis.deleteParts(); - - // The call to getParts should throw because we have already cleaned up the parts. - Throwable error = assertThrows(IllegalStateException.class, mpis::getParts); - assertThat(error.getMessage(), containsString("CLOSED")); + mpis.deleteParts(); // this should not be an NPE } @Test