Issue #4383 - avoid NPE from MultiPart Cleanup #4388

lachlan-roberts · 2019-12-03T04:31:55Z

Always assign _parts when constructed so it is never null.

Signed-off-by: Lachlan Roberts lachlan@webtide.com

Closes #4383

Always assign _parts when constructed so it is never null. Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

janbartel

Looks fine.

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

This will stop two threads from parsing at the same time. Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormInputStream.java

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

janbartel

As per offline discussion, lets remove the synchronization and just ensure that creation and cleaning of parts is sane.

jetty-server/src/main/java/org/eclipse/jetty/server/Request.java

- 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 <lachlan@webtide.com>

lachlan-roberts · 2019-12-16T07:41:35Z

@janbartel I added an atomic state to decide when it is safe to remove the parts.

When you call deleteParts() we will move to ERROR state which will cause an error in the parser before the next read, the parser will eventually move into COMPLETED state and that is when deleteParts() knows it is safe to delete the parts.

janbartel · 2019-12-17T23:17:32Z

jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormInputStream.java

+    private volatile File _tmpDir;
+    private volatile boolean _deleteOnExit;
+    private volatile boolean _writeFilesWithFilenames;
+    private volatile boolean _parsed;


You don't need _parsed if you have the state variable.

janbartel · 2019-12-17T23:17:48Z

jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormInputStream.java

    public boolean isEmpty()
    {
-        if (_parts == null)
+        if (!_parsed)


See previous comment.

janbartel · 2019-12-17T23:18:25Z

jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormInputStream.java

@@ -397,6 +407,31 @@ public boolean isEmpty()
     */
    public void deleteParts()
    {
+        while (true)


Really would prefer not to have these kind of loops.

janbartel · 2019-12-17T23:18:42Z

jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormInputStream.java

+                        continue;
+                    break;
+
+                case ERROR:


Maybe both ERROR and COMPLETED are both terminal states?

Just moving to ERROR state is not enough, the ERROR state is indicating that the parsing thread should exit, and the move to COMPLETED indicates that it has exited and will not create anymore parts.

janbartel · 2019-12-17T23:18:57Z

jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormInputStream.java

@@ -431,15 +466,7 @@ public void deleteParts()
        if (!_parsed)


See previous comment

janbartel · 2019-12-17T23:19:55Z

jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormInputStream.java

@@ -588,6 +617,27 @@ else if (len == -1)
            if (parser != null)
                parser.parse(BufferUtil.EMPTY_BUFFER, true);
        }
+        finally
+        {
+            while (true)


Again, would really prefer to avoid loops if possible.

…s called use synchronized state instead of atomic reference Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

lachlan-roberts · 2020-01-13T00:24:56Z

@janbartel nudge

joakime · 2020-01-17T13:42:18Z

@lachlan-roberts move these changes over to jetty-10.0.x in a new PR.

Issue #4383 - avoid NPE from MultiPart Cleanup

269e885

Always assign _parts when constructed so it is never null. Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

lachlan-roberts requested a review from janbartel December 3, 2019 04:31

janbartel approved these changes Dec 3, 2019

View reviewed changes

lachlan-roberts added 2 commits December 4, 2019 12:06

Issue #4383 - declare some MultiPartFormInputStream fields as final

bb931e4

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

Issue #4383 - mark other MultiPartFormInputStream fields as volatile

979f88f

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

lachlan-roberts force-pushed the jetty-9.4.x-4383-MultiPartNPE branch from a15e661 to 979f88f Compare December 4, 2019 03:15

lachlan-roberts added this to In progress in Jetty 9.4.25 via automation Dec 9, 2019

Issue #4383 - synchronize on getParts() and getPart() methods

6888e17

This will stop two threads from parsing at the same time. Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

lachlan-roberts requested a review from janbartel December 10, 2019 01:10

avoid creating BufferedInputStream if ServletInputStream isFinished

ad88b23

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

janbartel reviewed Dec 10, 2019

View reviewed changes

jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormInputStream.java Show resolved Hide resolved

janbartel requested changes Dec 10, 2019

View reviewed changes

jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormInputStream.java Show resolved Hide resolved

jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormInputStream.java Outdated Show resolved Hide resolved

Jetty 9.4.25 automation moved this from In progress to Review in progress Dec 10, 2019

synchronize on deprecated method getParsedParts() as well

1e63c83

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

lachlan-roberts requested a review from janbartel December 10, 2019 03:53

delay creation of handler until the MultiPartParser is created

9fdd454

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

janbartel requested changes Dec 11, 2019

View reviewed changes

jetty-server/src/main/java/org/eclipse/jetty/server/Request.java Outdated Show resolved Hide resolved

lachlan-roberts requested a review from janbartel December 16, 2019 07:41

janbartel requested changes Dec 17, 2019

View reviewed changes

parsing thread must call deleteParts if parsing when first deletePart…

f8be500

…s called use synchronized state instead of atomic reference Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

lachlan-roberts requested a review from janbartel December 27, 2019 06:18

joakime removed this from Review in progress in Jetty 9.4.25 Jan 13, 2020

joakime added this to In progress in Jetty 9.4.26 via automation Jan 13, 2020

joakime moved this from In progress to Review in progress in Jetty 9.4.26 Jan 13, 2020

joakime removed this from Review in progress in Jetty 9.4.26 Jan 15, 2020

joakime added this to In progress in Jetty 9.4.27 via automation Jan 15, 2020

joakime moved this from In progress to Review in progress in Jetty 9.4.27 Jan 15, 2020

joakime closed this Jan 17, 2020

Jetty 9.4.27 automation moved this from Review in progress to Done Jan 17, 2020

lachlan-roberts mentioned this pull request Jan 20, 2020

Issue #4383 - synchronize multiparts for cleanup from different thread #4498

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #4383 - avoid NPE from MultiPart Cleanup #4388

Issue #4383 - avoid NPE from MultiPart Cleanup #4388

lachlan-roberts commented Dec 3, 2019 •

edited by joakime

janbartel left a comment

janbartel left a comment

lachlan-roberts commented Dec 16, 2019

janbartel Dec 17, 2019

janbartel Dec 17, 2019

janbartel Dec 17, 2019

janbartel Dec 17, 2019

lachlan-roberts Dec 17, 2019

janbartel Dec 17, 2019

janbartel Dec 17, 2019

lachlan-roberts commented Jan 13, 2020

joakime commented Jan 17, 2020

Issue #4383 - avoid NPE from MultiPart Cleanup #4388

Issue #4383 - avoid NPE from MultiPart Cleanup #4388

Conversation

lachlan-roberts commented Dec 3, 2019 • edited by joakime

janbartel left a comment

Choose a reason for hiding this comment

janbartel left a comment

Choose a reason for hiding this comment

lachlan-roberts commented Dec 16, 2019

janbartel Dec 17, 2019

Choose a reason for hiding this comment

janbartel Dec 17, 2019

Choose a reason for hiding this comment

janbartel Dec 17, 2019

Choose a reason for hiding this comment

janbartel Dec 17, 2019

Choose a reason for hiding this comment

lachlan-roberts Dec 17, 2019

Choose a reason for hiding this comment

janbartel Dec 17, 2019

Choose a reason for hiding this comment

janbartel Dec 17, 2019

Choose a reason for hiding this comment

lachlan-roberts commented Jan 13, 2020

joakime commented Jan 17, 2020

lachlan-roberts commented Dec 3, 2019 •

edited by joakime