From 269e885812d317bdd9169d6551ceb55af9115d47 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 3 Dec 2019 14:02:57 +1100 Subject: [PATCH 1/9] Issue #4383 - avoid NPE from MultiPart Cleanup Always assign _parts when constructed so it is never null. Signed-off-by: Lachlan Roberts --- .../eclipse/jetty/http/MultiPartFormInputStream.java | 11 +++-------- 1 file changed, 3 insertions(+), 8 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 a9757cfc999f..57619873f425 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 @@ -59,11 +59,10 @@ public class MultiPartFormInputStream { private static final Logger LOG = Log.getLogger(MultiPartFormInputStream.class); - private static final MultiMap EMPTY_MAP = new MultiMap<>(Collections.emptyMap()); private InputStream _in; private MultipartConfigElement _config; private String _contentType; - private MultiMap _parts; + private MultiMap _parts = new MultiMap<>(); private Throwable _err; private File _tmpDir; private File _contextTmpDir; @@ -345,7 +344,6 @@ public MultiPartFormInputStream(InputStream in, String contentType, MultipartCon { if (((ServletInputStream)in).isFinished()) { - _parts = EMPTY_MAP; _parsed = true; return; } @@ -358,7 +356,7 @@ public MultiPartFormInputStream(InputStream in, String contentType, MultipartCon */ public boolean isEmpty() { - if (_parts == null) + if (_parts.isEmpty()) return true; Collection> values = _parts.values(); @@ -379,7 +377,7 @@ public boolean isEmpty() @Deprecated public Collection getParsedParts() { - if (_parts == null) + if (_parts.isEmpty()) return Collections.emptyList(); Collection> values = _parts.values(); @@ -492,9 +490,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; From bb931e46330abdf58cbbcc1d6648936c4fe14e31 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Wed, 4 Dec 2019 12:06:25 +1100 Subject: [PATCH 2/9] Issue #4383 - declare some MultiPartFormInputStream fields as final Signed-off-by: Lachlan Roberts --- .../jetty/http/MultiPartFormInputStream.java | 21 +++++++------------ 1 file changed, 8 insertions(+), 13 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 57619873f425..5984e4470edb 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 @@ -59,13 +59,13 @@ public class MultiPartFormInputStream { private static final Logger LOG = Log.getLogger(MultiPartFormInputStream.class); - private InputStream _in; - private MultipartConfigElement _config; - private String _contentType; - private MultiMap _parts = new MultiMap<>(); + private final MultiMap _parts = new MultiMap<>(); + private final InputStream _in; + private final MultipartConfigElement _config; + private final File _contextTmpDir; + private final String _contentType; private Throwable _err; private File _tmpDir; - private File _contextTmpDir; private boolean _deleteOnExit; private boolean _writeFilesWithFilenames; private boolean _parsed; @@ -332,14 +332,10 @@ public String getContentDispositionFilename() public MultiPartFormInputStream(InputStream in, String contentType, MultipartConfigElement config, File contextTmpDir) { _contentType = contentType; - _config = config; - _contextTmpDir = contextTmpDir; - if (_contextTmpDir == null) - _contextTmpDir = new File(System.getProperty("java.io.tmpdir")); - - if (_config == null) - _config = new MultipartConfigElement(_contextTmpDir.getAbsolutePath()); + _contextTmpDir = (contextTmpDir != null) ? contextTmpDir : new File(System.getProperty("java.io.tmpdir")); + _config = (config != null) ? config : new MultipartConfigElement(_contextTmpDir.getAbsolutePath()); + _in = new BufferedInputStream(in); if (in instanceof ServletInputStream) { if (((ServletInputStream)in).isFinished()) @@ -348,7 +344,6 @@ public MultiPartFormInputStream(InputStream in, String contentType, MultipartCon return; } } - _in = new BufferedInputStream(in); } /** From 979f88f774e2e0f75a7847545af69bd2f08e2884 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Wed, 4 Dec 2019 14:15:03 +1100 Subject: [PATCH 3/9] Issue #4383 - mark other MultiPartFormInputStream fields as volatile Signed-off-by: Lachlan Roberts --- .../eclipse/jetty/http/MultiPartFormInputStream.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 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 5984e4470edb..dad32740ab71 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 @@ -64,12 +64,12 @@ public class MultiPartFormInputStream private final MultipartConfigElement _config; private final File _contextTmpDir; private final String _contentType; - private Throwable _err; - private File _tmpDir; - private boolean _deleteOnExit; - private boolean _writeFilesWithFilenames; - private boolean _parsed; - private int _bufferSize = 16 * 1024; + private volatile Throwable _err; + private volatile File _tmpDir; + private volatile boolean _deleteOnExit; + private volatile boolean _writeFilesWithFilenames; + private volatile boolean _parsed; + private volatile int _bufferSize = 16 * 1024; public class MultiPart implements Part { From 6888e17f4d92131c064e2d6b9a3d95fdc74aca32 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 10 Dec 2019 12:07:44 +1100 Subject: [PATCH 4/9] Issue #4383 - synchronize on getParts() and getPart() methods This will stop two threads from parsing at the same time. Signed-off-by: Lachlan Roberts --- .../jetty/http/MultiPartFormInputStream.java | 90 ++++++++++--------- .../org/eclipse/jetty/server/MultiParts.java | 1 + .../org/eclipse/jetty/server/Request.java | 2 +- .../org/eclipse/jetty/server/RequestTest.java | 54 ++++++----- 4 files changed, 84 insertions(+), 63 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 dad32740ab71..0f611f396d0e 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 @@ -35,6 +35,7 @@ import java.util.Collection; import java.util.Collections; import java.util.List; +import java.util.stream.Collectors; import javax.servlet.MultipartConfigElement; import javax.servlet.ServletInputStream; import javax.servlet.http.Part; @@ -339,29 +340,33 @@ public MultiPartFormInputStream(InputStream in, String contentType, MultipartCon if (in instanceof ServletInputStream) { if (((ServletInputStream)in).isFinished()) - { _parsed = true; - return; - } } } /** * @return whether the list of parsed parts is empty + * @deprecated use getParts().isEmpty() */ + @Deprecated public boolean isEmpty() { - if (_parts.isEmpty()) - return true; - - Collection> values = _parts.values(); - for (List partList : values) + synchronized (this) { - if (!partList.isEmpty()) - return false; - } + if (!_parsed) + throw new IllegalStateException(); + + if (_parts.isEmpty()) + return true; - return true; + for (List partList : _parts.values()) + { + if (!partList.isEmpty()) + return false; + } + + return true; + } } /** @@ -390,27 +395,31 @@ public Collection getParsedParts() */ public void deleteParts() { - MultiException err = null; - for (List parts : _parts.values()) + // TODO: Can we cancel parsing somehow instead of blocking. + synchronized (this) { - for (Part p : parts) + MultiException err = null; + for (List parts : _parts.values()) { - try + for (Part p : parts) { - ((MultiPart)p).cleanUp(); - } - catch (Exception e) - { - if (err == null) - err = new MultiException(); - err.add(e); + try + { + ((MultiPart)p).cleanUp(); + } + catch (Exception e) + { + if (err == null) + err = new MultiException(); + err.add(e); + } } } - } - _parts.clear(); + _parts.clear(); - if (err != null) - err.ifExceptionThrowRuntime(); + if (err != null) + err.ifExceptionThrowRuntime(); + } } /** @@ -421,18 +430,13 @@ public void deleteParts() */ public Collection getParts() throws IOException { - if (!_parsed) - parse(); - throwIfError(); - - Collection> values = _parts.values(); - List parts = new ArrayList<>(); - for (List o : values) + synchronized (this) { - List asList = LazyList.getList(o, false); - parts.addAll(asList); + if (!_parsed) + parse(); + throwIfError(); + return _parts.values().stream().flatMap(List::stream).collect(Collectors.toList()); } - return parts; } /** @@ -444,10 +448,13 @@ public Collection getParts() throws IOException */ public Part getPart(String name) throws IOException { - if (!_parsed) - parse(); - throwIfError(); - return _parts.getValue(name, 0); + synchronized (this) + { + if (!_parsed) + parse(); + throwIfError(); + return _parts.getValue(name, 0); + } } /** @@ -522,7 +529,6 @@ else if ("".equals(_config.getLocation())) while (true) { - len = _in.read(data); if (len > 0) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/MultiParts.java b/jetty-server/src/main/java/org/eclipse/jetty/server/MultiParts.java index 9a1f20d65cd1..c5cfb3447be8 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/MultiParts.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/MultiParts.java @@ -46,6 +46,7 @@ public interface MultiParts extends Closeable Part getPart(String name) throws IOException; + @Deprecated boolean isEmpty(); ContextHandler.Context getContext(); diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java index f02ad81dbdab..1727d40ee6ee 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java @@ -2310,7 +2310,7 @@ public Collection getParts() throws IOException, ServletException return getParts(null); } - private Collection getParts(MultiMap params) throws IOException + private synchronized Collection getParts(MultiMap params) throws IOException { if (_multiParts == null) _multiParts = (MultiParts)getAttribute(MULTIPARTS); diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java index d5ac2ed0ea81..1b3bc6557261 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java @@ -351,16 +351,23 @@ public void testMultiPart() throws Exception @Override public void requestDestroyed(ServletRequestEvent sre) { - MultiParts m = (MultiParts)sre.getServletRequest().getAttribute(Request.MULTIPARTS); - assertNotNull(m); - ContextHandler.Context c = m.getContext(); - assertNotNull(c); - assertTrue(c == sre.getServletContext()); - assertTrue(!m.isEmpty()); - assertTrue(testTmpDir.list().length == 2); - super.requestDestroyed(sre); - String[] files = testTmpDir.list(); - assertTrue(files.length == 0); + try + { + MultiParts m = (MultiParts)sre.getServletRequest().getAttribute(Request.MULTIPARTS); + assertNotNull(m); + ContextHandler.Context c = m.getContext(); + assertNotNull(c); + assertTrue(c == sre.getServletContext()); + assertTrue(!m.getParts().isEmpty()); + assertTrue(testTmpDir.list().length == 2); + super.requestDestroyed(sre); + String[] files = testTmpDir.list(); + assertTrue(files.length == 0); + } + catch (IOException e) + { + throw new RuntimeException(e); + } } }); _server.stop(); @@ -411,16 +418,23 @@ public void testUtilMultiPart() throws Exception @Override public void requestDestroyed(ServletRequestEvent sre) { - MultiParts m = (MultiParts)sre.getServletRequest().getAttribute(Request.MULTIPARTS); - assertNotNull(m); - ContextHandler.Context c = m.getContext(); - assertNotNull(c); - assertTrue(c == sre.getServletContext()); - assertTrue(!m.isEmpty()); - assertTrue(testTmpDir.list().length == 2); - super.requestDestroyed(sre); - String[] files = testTmpDir.list(); - assertTrue(files.length == 0); + try + { + MultiParts m = (MultiParts)sre.getServletRequest().getAttribute(Request.MULTIPARTS); + assertNotNull(m); + ContextHandler.Context c = m.getContext(); + assertNotNull(c); + assertTrue(c == sre.getServletContext()); + assertTrue(!m.getParts().isEmpty()); + assertTrue(testTmpDir.list().length == 2); + super.requestDestroyed(sre); + String[] files = testTmpDir.list(); + assertTrue(files.length == 0); + } + catch (IOException t) + { + throw new RuntimeException(t); + } } }); _server.stop(); From ad88b237b16bdff32bff6090f33db810053abe36 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 10 Dec 2019 14:07:14 +1100 Subject: [PATCH 5/9] avoid creating BufferedInputStream if ServletInputStream isFinished Signed-off-by: Lachlan Roberts --- .../org/eclipse/jetty/http/MultiPartFormInputStream.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) 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 0f611f396d0e..287354caa5c2 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 @@ -336,12 +336,17 @@ public MultiPartFormInputStream(InputStream in, String contentType, MultipartCon _contextTmpDir = (contextTmpDir != null) ? contextTmpDir : new File(System.getProperty("java.io.tmpdir")); _config = (config != null) ? config : new MultipartConfigElement(_contextTmpDir.getAbsolutePath()); - _in = new BufferedInputStream(in); if (in instanceof ServletInputStream) { if (((ServletInputStream)in).isFinished()) + { + _in = null; _parsed = true; + return; + } } + + _in = new BufferedInputStream(in); } /** From 1e63c8386e1a30f60e36747bffb7b3b7590d96d5 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 10 Dec 2019 14:52:39 +1100 Subject: [PATCH 6/9] synchronize on deprecated method getParsedParts() as well Signed-off-by: Lachlan Roberts --- .../jetty/http/MultiPartFormInputStream.java | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 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 287354caa5c2..0ca797be50c2 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 @@ -382,17 +382,20 @@ public boolean isEmpty() @Deprecated public Collection getParsedParts() { - if (_parts.isEmpty()) - return Collections.emptyList(); - - Collection> values = _parts.values(); - List parts = new ArrayList<>(); - for (List o : values) + synchronized (this) { - List asList = LazyList.getList(o, false); - parts.addAll(asList); + if (_parts.isEmpty()) + return Collections.emptyList(); + + Collection> values = _parts.values(); + List parts = new ArrayList<>(); + for (List o : values) + { + List asList = LazyList.getList(o, false); + parts.addAll(asList); + } + return parts; } - return parts; } /** From 9fdd454daa11480c6bcbf4908aaabb74ce7129bc Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 10 Dec 2019 15:06:44 +1100 Subject: [PATCH 7/9] delay creation of handler until the MultiPartParser is created Signed-off-by: Lachlan Roberts --- .../java/org/eclipse/jetty/http/MultiPartFormInputStream.java | 3 +-- 1 file changed, 1 insertion(+), 2 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 0ca797be50c2..0dd1ef6c81d7 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 @@ -497,7 +497,6 @@ protected void parse() _parsed = true; MultiPartParser parser = null; - Handler handler = new Handler(); try { // if its not a multipart request, don't parse it @@ -530,7 +529,7 @@ else if ("".equals(_config.getLocation())) contentTypeBoundary = QuotedStringTokenizer.unquote(value(_contentType.substring(bstart, bend)).trim()); } - parser = new MultiPartParser(handler, contentTypeBoundary); + parser = new MultiPartParser(new Handler(), contentTypeBoundary); byte[] data = new byte[_bufferSize]; int len; long total = 0; From 6d15071b467e3de320c64b638810528087cc9e81 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Mon, 16 Dec 2019 18:35:10 +1100 Subject: [PATCH 8/9] Issue #4383 - use atomic state for multipart cleanup - Removed synchronization for parsing by two threads. - Introduced an atomic state to decide when it is safe to remove the parts. The call to deleteParts will now cancel the parsing and only delete the parts when the parser exits. Signed-off-by: Lachlan Roberts --- .../jetty/http/MultiPartFormInputStream.java | 163 +++++++++++------- .../http/MultiPartFormInputStreamTest.java | 75 ++++++++ .../org/eclipse/jetty/server/Request.java | 2 +- 3 files changed, 181 insertions(+), 59 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 0dd1ef6c81d7..03a4d8a65a72 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 @@ -35,6 +35,7 @@ import java.util.Collection; import java.util.Collections; import java.util.List; +import java.util.concurrent.atomic.AtomicReference; import java.util.stream.Collectors; import javax.servlet.MultipartConfigElement; import javax.servlet.ServletInputStream; @@ -59,7 +60,16 @@ */ public class MultiPartFormInputStream { + private enum State + { + UNPARSED, + PARSING, + ERROR, + COMPLETED + } + private static final Logger LOG = Log.getLogger(MultiPartFormInputStream.class); + private final AtomicReference state = new AtomicReference<>(State.UNPARSED); private final MultiMap _parts = new MultiMap<>(); private final InputStream _in; private final MultipartConfigElement _config; @@ -356,22 +366,19 @@ public MultiPartFormInputStream(InputStream in, String contentType, MultipartCon @Deprecated public boolean isEmpty() { - synchronized (this) - { - if (!_parsed) - throw new IllegalStateException(); - - if (_parts.isEmpty()) - return true; - - for (List partList : _parts.values()) - { - if (!partList.isEmpty()) - return false; - } + if (!_parsed) + throw new IllegalStateException(); + if (_parts.isEmpty()) return true; + + for (List partList : _parts.values()) + { + if (!partList.isEmpty()) + return false; } + + return true; } /** @@ -382,20 +389,17 @@ public boolean isEmpty() @Deprecated public Collection getParsedParts() { - synchronized (this) - { - if (_parts.isEmpty()) - return Collections.emptyList(); + if (_parts.isEmpty()) + return Collections.emptyList(); - Collection> values = _parts.values(); - List parts = new ArrayList<>(); - for (List o : values) - { - List asList = LazyList.getList(o, false); - parts.addAll(asList); - } - return parts; + Collection> values = _parts.values(); + List parts = new ArrayList<>(); + for (List o : values) + { + List asList = LazyList.getList(o, false); + parts.addAll(asList); } + return parts; } /** @@ -403,31 +407,52 @@ public Collection getParsedParts() */ public void deleteParts() { - // TODO: Can we cancel parsing somehow instead of blocking. - synchronized (this) + while (true) + { + switch (state.get()) + { + case PARSING: + state.compareAndSet(State.PARSING, State.ERROR); + Thread.yield(); + continue; + + case UNPARSED: + if (!state.compareAndSet(State.UNPARSED, State.COMPLETED)) + continue; + break; + + case ERROR: + Thread.yield(); + continue; + + case COMPLETED: + break; + } + + break; + } + + MultiException err = null; + for (List parts : _parts.values()) { - MultiException err = null; - for (List parts : _parts.values()) + for (Part p : parts) { - for (Part p : parts) + try { - try - { - ((MultiPart)p).cleanUp(); - } - catch (Exception e) - { - if (err == null) - err = new MultiException(); - err.add(e); - } + ((MultiPart)p).cleanUp(); + } + catch (Exception e) + { + if (err == null) + err = new MultiException(); + err.add(e); } } - _parts.clear(); - - if (err != null) - err.ifExceptionThrowRuntime(); } + _parts.clear(); + + if (err != null) + err.ifExceptionThrowRuntime(); } /** @@ -438,13 +463,10 @@ public void deleteParts() */ public Collection getParts() throws IOException { - synchronized (this) - { - if (!_parsed) - parse(); - throwIfError(); - return _parts.values().stream().flatMap(List::stream).collect(Collectors.toList()); - } + if (!_parsed) + parse(); + throwIfError(); + return _parts.values().stream().flatMap(List::stream).collect(Collectors.toList()); } /** @@ -456,13 +478,10 @@ public Collection getParts() throws IOException */ public Part getPart(String name) throws IOException { - synchronized (this) - { - if (!_parsed) - parse(); - throwIfError(); - return _parts.getValue(name, 0); - } + if (!_parsed) + parse(); + throwIfError(); + return _parts.getValue(name, 0); } /** @@ -534,8 +553,15 @@ else if ("".equals(_config.getLocation())) int len; long total = 0; + if (!state.compareAndSet(State.UNPARSED, State.PARSING)) + throw new IllegalStateException("Could not start parsing " + state.get()); + while (true) { + State currentState = state.get(); + if (currentState != State.PARSING) + throw new IllegalStateException("Unexpected state " + currentState); + len = _in.read(data); if (len > 0) @@ -591,6 +617,27 @@ else if (len == -1) if (parser != null) parser.parse(BufferUtil.EMPTY_BUFFER, true); } + finally + { + while (true) + { + switch (state.get()) + { + case PARSING: + if (!state.compareAndSet(State.PARSING, State.COMPLETED)) + continue; + break; + + case ERROR: + state.compareAndSet(State.ERROR, State.COMPLETED); + break; + + default: + break; + } + break; + } + } } class Handler implements MultiPartParser.Handler 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 a8c1d9861190..0081e9f08973 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 @@ -25,6 +25,8 @@ import java.io.InputStream; import java.util.Base64; import java.util.Collection; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import javax.servlet.MultipartConfigElement; import javax.servlet.ReadListener; @@ -509,6 +511,79 @@ public void testPartTmpFileDeletion() throws Exception assertThat(stuff.exists(), is(false)); //tmp file was removed after cleanup } + @Test + public void testAsyncCleanUp() throws Exception + { + final CountDownLatch reading = new CountDownLatch(1); + final InputStream wrappedStream = new ByteArrayInputStream(createMultipartRequestString("myFile").getBytes()); + + // This stream won't allow the parser to exit because it will never return anything less than 0. + InputStream slowStream = new InputStream() { + @Override + public int read(byte[] b, int off, int len) throws IOException + { + return Math.max(0, super.read(b, off, len)); + } + + @Override + public int read() throws IOException + { + reading.countDown(); + return wrappedStream.read(); + } + }; + + MultipartConfigElement config = new MultipartConfigElement(_dirname, 1024, 1024, 50); + MultiPartFormInputStream mpis = new MultiPartFormInputStream(slowStream, _contentType, config, _tmpDir); + + // In another thread delete the parts when we detect that we have started parsing. + CompletableFuture cleanupError = new CompletableFuture<>(); + new Thread(() -> + { + try + { + assertTrue(reading.await(5, TimeUnit.SECONDS)); + mpis.deleteParts(); + cleanupError.complete(null); + } + catch (Throwable t) + { + cleanupError.complete(t); + } + }).start(); + + // The call to getParts should throw an error. + Throwable error = assertThrows(IllegalStateException.class, mpis::getParts); + assertThat(error.getMessage(), is("Unexpected state ERROR")); + + // There was no error with the cleanup. + assertNull(cleanupError.get()); + + // No tmp files are remaining. + String[] fileList = _tmpDir.list(); + assertNotNull(fileList); + assertThat(fileList.length, is(0)); + } + + @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(), is("Could not start parsing COMPLETED")); + + // No tmp files are remaining. + String[] fileList = _tmpDir.list(); + assertNotNull(fileList); + assertThat(fileList.length, is(0)); + } + @Test public void testLFOnlyRequest() throws Exception diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java index 1727d40ee6ee..f02ad81dbdab 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java @@ -2310,7 +2310,7 @@ public Collection getParts() throws IOException, ServletException return getParts(null); } - private synchronized Collection getParts(MultiMap params) throws IOException + private Collection getParts(MultiMap params) throws IOException { if (_multiParts == null) _multiParts = (MultiParts)getAttribute(MULTIPARTS); From f8be5008cbba75241aba38ef389c191b7fff52fc Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Wed, 18 Dec 2019 15:07:03 +1100 Subject: [PATCH 9/9] parsing thread must call deleteParts if parsing when first deleteParts called use synchronized state instead of atomic reference Signed-off-by: Lachlan Roberts --- .../jetty/http/MultiPartFormInputStream.java | 100 +++++++++--------- .../http/MultiPartFormInputStreamTest.java | 10 +- 2 files changed, 56 insertions(+), 54 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 03a4d8a65a72..b6e7a59b4703 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 @@ -35,7 +35,6 @@ import java.util.Collection; import java.util.Collections; import java.util.List; -import java.util.concurrent.atomic.AtomicReference; import java.util.stream.Collectors; import javax.servlet.MultipartConfigElement; import javax.servlet.ServletInputStream; @@ -64,12 +63,12 @@ private enum State { UNPARSED, PARSING, - ERROR, - COMPLETED + PARSED, + CLOSING, + CLOSED } private static final Logger LOG = Log.getLogger(MultiPartFormInputStream.class); - private final AtomicReference state = new AtomicReference<>(State.UNPARSED); private final MultiMap _parts = new MultiMap<>(); private final InputStream _in; private final MultipartConfigElement _config; @@ -79,8 +78,8 @@ private enum State private volatile File _tmpDir; private volatile boolean _deleteOnExit; private volatile boolean _writeFilesWithFilenames; - private volatile boolean _parsed; private volatile int _bufferSize = 16 * 1024; + private State state = State.UNPARSED; public class MultiPart implements Part { @@ -351,7 +350,7 @@ public MultiPartFormInputStream(InputStream in, String contentType, MultipartCon if (((ServletInputStream)in).isFinished()) { _in = null; - _parsed = true; + state = State.PARSED; return; } } @@ -366,9 +365,6 @@ public MultiPartFormInputStream(InputStream in, String contentType, MultipartCon @Deprecated public boolean isEmpty() { - if (!_parsed) - throw new IllegalStateException(); - if (_parts.isEmpty()) return true; @@ -407,29 +403,24 @@ public Collection getParsedParts() */ public void deleteParts() { - while (true) + synchronized (this) { - switch (state.get()) + switch (state) { - case PARSING: - state.compareAndSet(State.PARSING, State.ERROR); - Thread.yield(); - continue; - + case CLOSED: case UNPARSED: - if (!state.compareAndSet(State.UNPARSED, State.COMPLETED)) - continue; - break; + state = State.CLOSED; + return; - case ERROR: - Thread.yield(); - continue; + case PARSING: + state = State.CLOSING; + return; - case COMPLETED: + case PARSED: + case CLOSING: + state = State.CLOSED; break; } - - break; } MultiException err = null; @@ -463,8 +454,7 @@ public void deleteParts() */ public Collection getParts() throws IOException { - if (!_parsed) - parse(); + parse(); throwIfError(); return _parts.values().stream().flatMap(List::stream).collect(Collectors.toList()); } @@ -478,8 +468,7 @@ public Collection getParts() throws IOException */ public Part getPart(String name) throws IOException { - if (!_parsed) - parse(); + parse(); throwIfError(); return _parts.getValue(name, 0); } @@ -510,10 +499,22 @@ protected void throwIfError() throws IOException */ protected void parse() { - // have we already parsed the input? - if (_parsed) - return; - _parsed = true; + synchronized (this) + { + switch (state) + { + case UNPARSED: + state = State.PARSING; + break; + + case PARSED: + return; + + default: + _err = new IllegalStateException(state.name()); + return; + } + } MultiPartParser parser = null; try @@ -553,17 +554,18 @@ else if ("".equals(_config.getLocation())) int len; long total = 0; - if (!state.compareAndSet(State.UNPARSED, State.PARSING)) - throw new IllegalStateException("Could not start parsing " + state.get()); - while (true) { - State currentState = state.get(); - if (currentState != State.PARSING) - throw new IllegalStateException("Unexpected state " + currentState); + synchronized (this) + { + if (state != State.PARSING) + { + _err = new IllegalStateException(state.name()); + return; + } + } len = _in.read(data); - if (len > 0) { // keep running total of size of bytes read from input and throw an exception if exceeds MultipartConfigElement._maxRequestSize @@ -619,24 +621,26 @@ else if (len == -1) } finally { - while (true) + boolean cleanup = false; + synchronized (this) { - switch (state.get()) + switch (state) { case PARSING: - if (!state.compareAndSet(State.PARSING, State.COMPLETED)) - continue; + state = State.PARSED; break; - case ERROR: - state.compareAndSet(State.ERROR, State.COMPLETED); + case CLOSING: + cleanup = true; break; default: - break; + _err = new IllegalStateException(state.name()); } - break; } + + if (cleanup) + deleteParts(); } } 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 0081e9f08973..d32dd31020ee 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 @@ -554,7 +554,7 @@ public int read() throws IOException // The call to getParts should throw an error. Throwable error = assertThrows(IllegalStateException.class, mpis::getParts); - assertThat(error.getMessage(), is("Unexpected state ERROR")); + assertThat(error.getMessage(), is("CLOSING")); // There was no error with the cleanup. assertNull(cleanupError.get()); @@ -576,12 +576,10 @@ public void testParseAfterCleanUp() throws Exception // The call to getParts should throw because we have already cleaned up the parts. Throwable error = assertThrows(IllegalStateException.class, mpis::getParts); - assertThat(error.getMessage(), is("Could not start parsing COMPLETED")); + assertThat(error.getMessage(), is("CLOSED")); - // No tmp files are remaining. - String[] fileList = _tmpDir.list(); - assertNotNull(fileList); - assertThat(fileList.length, is(0)); + // Even though we called getParts() we never even created the tmp directory as we had already called deleteParts(). + assertFalse(_tmpDir.exists()); } @Test