From 719c60d70dd860a30257125a42549d7c28f81d8e Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 31 Jan 2023 11:42:40 +1100 Subject: [PATCH] changes from review Signed-off-by: Lachlan Roberts --- .../org/eclipse/jetty/http/MultiPart.java | 102 ++++++++---------- .../jetty/ee10/servlet/ServletApiRequest.java | 13 +-- .../ee10/servlet/MultiPartServletTest.java | 1 - 3 files changed, 44 insertions(+), 72 deletions(-) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java index e3d67e7038f..eb22ad5f71d 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java @@ -18,7 +18,6 @@ import java.io.EOFException; import java.io.IOException; import java.io.OutputStream; import java.io.UncheckedIOException; -import java.nio.Buffer; import java.nio.ByteBuffer; import java.nio.charset.Charset; import java.nio.file.Files; @@ -41,6 +40,7 @@ import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.IO; import org.eclipse.jetty.util.QuotedStringTokenizer; import org.eclipse.jetty.util.SearchPattern; +import org.eclipse.jetty.util.StaticException; import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.Utf8StringBuilder; import org.eclipse.jetty.util.thread.AutoLock; @@ -121,18 +121,32 @@ public class MultiPart */ public abstract static class Part { + private static final Throwable SENTINEL_CLOSE_EXCEPTION = new StaticException("Closed"); + private final String name; private final String fileName; private final HttpFields fields; - protected Path path; + private final Content.Source contentSource; + private Path path; private boolean temporary = true; - private Content.Source content; public Part(String name, String fileName, HttpFields fields) + { + this(name, fileName, fields, null); + } + + private Part(String name, String fileName, HttpFields fields, Path path) { this.name = name; this.fileName = fileName; this.fields = fields != null ? fields : HttpFields.EMPTY; + this.path = path; + this.contentSource = newContentSource(); + } + + private Path getPath() + { + return path; } /** @@ -164,7 +178,7 @@ public class MultiPart /** *

Returns the content of this part as a {@link Content.Source}.

- *

Calling this method multiple times will return the same instance.

+ *

Calling this method multiple times will return the same instance, which can only be consumed once.

*

The content type and content encoding are specified in this part's * {@link #getHeaders() headers}.

*

The content encoding may be specified by the part named {@code _charset_}, @@ -176,9 +190,7 @@ public class MultiPart */ public Content.Source getContentSource() { - if (content == null) - content = newContentSource(); - return content; + return contentSource; } /** @@ -246,6 +258,7 @@ public class MultiPart */ public void writeTo(Path path) throws IOException { + this.temporary = false; if (this.path == null) { try (OutputStream out = Files.newOutputStream(path)) @@ -257,7 +270,6 @@ public class MultiPart else { this.path = Files.move(this.path, path, StandardCopyOption.REPLACE_EXISTING); - this.temporary = false; } } @@ -269,22 +281,22 @@ public class MultiPart public void close() { - try - { - if (temporary) - delete(); - } - catch (Throwable t) - { - if (LOG.isDebugEnabled()) - LOG.debug("Error closing part {}", this, t); - } + fail(SENTINEL_CLOSE_EXCEPTION); } public void fail(Throwable t) { - getContentSource().fail(t); - close(); + try + { + getContentSource().fail(t); + if (temporary) + delete(); + } + catch (Throwable x) + { + if (LOG.isDebugEnabled()) + LOG.debug("Error closing part {}", this, x); + } } } @@ -295,7 +307,6 @@ public class MultiPart public static class ByteBufferPart extends Part { private final List content; - private final long length; public ByteBufferPart(String name, String fileName, HttpFields fields, ByteBuffer... buffers) { @@ -305,16 +316,9 @@ public class MultiPart public ByteBufferPart(String name, String fileName, HttpFields fields, List content) { super(name, fileName, fields); - this.length = content.stream().mapToLong(Buffer::remaining).sum(); this.content = content; } - @Override - public long getLength() - { - return length; - } - @Override public Content.Source newContentSource() { @@ -329,7 +333,7 @@ public class MultiPart hashCode(), getName(), getFileName(), - length + getLength() ); } } @@ -340,19 +344,11 @@ public class MultiPart public static class ChunksPart extends Part { private final List content; - private final long length; public ChunksPart(String name, String fileName, HttpFields fields, List content) { super(name, fileName, fields); this.content = content; - this.length = content.stream().mapToLong(c -> c.getByteBuffer().remaining()).sum(); - } - - @Override - public long getLength() - { - return length; } @Override @@ -377,7 +373,7 @@ public class MultiPart hashCode(), getName(), getFileName(), - length + getLength() ); } } @@ -389,32 +385,18 @@ public class MultiPart { public PathPart(String name, String fileName, HttpFields fields, Path path) { - super(name, fileName, fields); - this.path = path; - } - - @Override - public long getLength() - { - try - { - return Files.size(path); - } - catch (IOException x) - { - throw new UncheckedIOException(x); - } + super(name, fileName, fields, path); } public Path getPath() { - return path; + return super.getPath(); } @Override public Content.Source newContentSource() { - return new PathContentSource(path); + return new PathContentSource(getPath()); } @Override @@ -425,7 +407,7 @@ public class MultiPart hashCode(), getName(), getFileName(), - path + getPath() ); } } @@ -459,7 +441,7 @@ public class MultiPart hashCode(), getName(), getFileName(), - content.getLength() + getLength() ); } } @@ -881,7 +863,7 @@ public class MultiPart } /** - * @return the maximum number of parts that can be parsed from the multipart content (-1 for unlimited parts). + * @return the maximum number of parts that can be parsed from the multipart content (0 for no parts allowed, -1 for unlimited parts). */ public long getMaxParts() { @@ -889,7 +871,7 @@ public class MultiPart } /** - * @param maxParts the maximum number of parts that can be parsed from the multipart content (-1 for unlimited parts). + * @param maxParts the maximum number of parts that can be parsed from the multipart content (0 for no parts allowed, -1 for unlimited parts). */ public void setMaxParts(long maxParts) { @@ -955,7 +937,7 @@ public class MultiPart else if (type == HttpTokens.Type.LF) { numParts++; - if (numParts > maxParts) + if (maxParts >= 0 && numParts > maxParts) throw new IllegalStateException(String.format("Form with too many keys [%d > %d]", numParts, maxParts)); notifyPartBegin(); diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java index 76cb554306e..3a1b79a2e31 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java @@ -14,7 +14,6 @@ package org.eclipse.jetty.ee10.servlet; import java.io.BufferedReader; -import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.InputStream; import java.io.InputStreamReader; @@ -577,9 +576,7 @@ public class ServletApiRequest implements HttpServletRequest { try (InputStream is = charsetPart.getInputStream()) { - ByteArrayOutputStream os = new ByteArrayOutputStream(); - IO.copy(is, os); - formCharset = os.toString(StandardCharsets.UTF_8); + formCharset = IO.toString(is, StandardCharsets.UTF_8); } } @@ -602,7 +599,6 @@ public class ServletApiRequest implements HttpServletRequest defaultCharset = StandardCharsets.UTF_8; long formContentSize = 0; - ByteArrayOutputStream os = null; for (Part p : parts) { if (p.getSubmittedFileName() == null) @@ -618,16 +614,11 @@ public class ServletApiRequest implements HttpServletRequest try (InputStream is = p.getInputStream()) { - if (os == null) - os = new ByteArrayOutputStream(); - IO.copy(is, os); - - String content = os.toString(charset == null ? defaultCharset : Charset.forName(charset)); + String content = IO.toString(is, charset == null ? defaultCharset : Charset.forName(charset)); if (_contentParameters == null) _contentParameters = new Fields(); _contentParameters.add(p.getName(), content); } - os.reset(); } } } diff --git a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java index e58ef4852ea..5bbccf73da8 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java @@ -466,5 +466,4 @@ public class MultiPartServletTest assertThat(responseContent, containsString("Parameter: part3=" + contentString)); assertThat(responseContent, not(containsString("Parameter: partFileName=" + contentString))); } - }