From 8e6b6c5ac3a5d51f231feb4ec674e23331a5c2c0 Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Wed, 17 Jun 2026 15:54:36 +0100 Subject: [PATCH] Refine error handling in MultipartParser Closes gh-36947 --- .../converter/multipart/MultipartParser.java | 103 +++++++++++++----- .../converter/multipart/PartGenerator.java | 9 +- .../MultipartHttpMessageConverterTests.java | 2 +- .../multipart/MultipartParserTests.java | 17 ++- 4 files changed, 96 insertions(+), 35 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/http/converter/multipart/MultipartParser.java b/spring-web/src/main/java/org/springframework/http/converter/multipart/MultipartParser.java index 59839b1f5fb..42b3aec9e6b 100644 --- a/spring-web/src/main/java/org/springframework/http/converter/multipart/MultipartParser.java +++ b/spring-web/src/main/java/org/springframework/http/converter/multipart/MultipartParser.java @@ -16,7 +16,6 @@ package org.springframework.http.converter.multipart; -import java.io.IOException; import java.io.InputStream; import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; @@ -81,10 +80,28 @@ final class MultipartParser { } + /** + * Simple delegation to {@link State#data(DataBuffer)}. + */ void handleData(DataBuffer dataBuffer) { this.state.data(dataBuffer); } + /** + * Handle a parsing error cleaning resources in the state and in the listener + * unless the error is {@link HttpMessageConversionException} in which case + * it is simply propagated. + */ + void handleException(String message, @Nullable Throwable cause) { + if (cause instanceof HttpMessageConversionException ex) { + throw ex; + } + changeState(DisposedState.INSTANCE, null); + HttpMessageConversionException ex = new HttpMessageConversionException(message, cause); + this.listener.onError(ex); + throw ex; + } + private void changeState(State newState, @Nullable DataBuffer remainder) { if (logger.isTraceEnabled()) { logger.trace("Changed state: " + this.state + " -> " + newState); @@ -118,6 +135,7 @@ final class MultipartParser { return result; } + /** * Parse the given stream of bytes into events published to the given {@link PartListener}. * @param input the input stream @@ -141,9 +159,8 @@ final class MultipartParser { } parser.state.complete(); } - catch (IOException ex) { - parser.state.dispose(); - listener.onError(new HttpMessageConversionException("Could not decode multipart message", ex)); + catch (Throwable ex) { + parser.handleException("Could not decode multipart message", ex); } } @@ -155,13 +172,19 @@ final class MultipartParser { /** * Handle {@link HttpHeaders} for a part. + *

Expectations for exception handling are the same as for {@link #onBody}. */ void onHeaders(HttpHeaders headers); /** - * Handle a piece of data for a body part. + * Handle the next chunk of body data. + *

Implementations must release the input buffer. + *

Implementations must handle all exceptions, cleaning up resources, + * and wrapping the exception as {@link HttpMessageConversionException}. * @param buffer a chunk of body - * @param last whether this is the last chunk for the part + * @param last whether this is the last chunk for the part + * @throws HttpMessageConversionException if the buffer could not be + * handled due to exceeded limits or for any other reason */ void onBody(DataBuffer buffer, boolean last); @@ -171,7 +194,12 @@ final class MultipartParser { void onComplete(); /** - * Handle any error thrown during the parsing phase. + * Handle any error thrown during the parsing phase. The purpose of the + * method call is to allow cleaning up of resources. The listener does + * not need to throw or wrap and throw the error. + *

{@link #onHeaders} and {@link #onBody} are expected to handle their + * own exceptions, i.e. any exception those methods throw will not be + * passed here. */ void onError(Throwable error); @@ -194,10 +222,26 @@ final class MultipartParser { */ private interface State { + /** + * Handle the next chunk of data. + *

If this method raises any exception other than + * {@link HttpMessageConversionException}, it will be + * {@link MultipartParser#handleException handled} by the parser. + * An {@link HttpMessageConversionException} on the other hand is + * considered fully handled and allowed to propagate as is. + */ void data(DataBuffer buf); + /** + * Called when the current part is fully parsed. + *

Expecations for exception handling are the same as for {@link #data}. + */ void complete(); + /** + * Clean up resources held by the state. Called in case of errors or + * when switching to a new state. + */ default void dispose() { } @@ -241,8 +285,7 @@ final class MultipartParser { @Override public void complete() { - changeState(DisposedState.INSTANCE, null); - MultipartParser.this.listener.onError(new HttpMessageConversionException("Could not find first boundary")); + handleException("Could not find first boundary", null); } @Override @@ -312,7 +355,14 @@ final class MultipartParser { if (logger.isTraceEnabled()) { logger.trace("Emitting headers: " + headers); } - MultipartParser.this.listener.onHeaders(headers); + try { + MultipartParser.this.listener.onHeaders(headers); + } + catch (Throwable ex) { + // PartListener should have cleaned its state, clean our own + dispose(); + throw ex; + } } /** @@ -338,12 +388,9 @@ final class MultipartParser { if (count <= MultipartParser.this.maxHeadersSize) { return true; } - else { - MultipartParser.this.listener.onError( - new HttpMessageConversionException("Part headers exceeded the memory usage limit of " + - MultipartParser.this.maxHeadersSize + " bytes")); - return false; - } + MultipartParser.this.handleException( + "Part headers exceeded the limit of " + MultipartParser.this.maxHeadersSize + " bytes", null); + return false; } /** @@ -377,8 +424,7 @@ final class MultipartParser { @Override public void complete() { - changeState(DisposedState.INSTANCE, null); - MultipartParser.this.listener.onError(new HttpMessageConversionException("Could not find end of headers")); + MultipartParser.this.handleException("Could not find end of headers", null); } @Override @@ -493,25 +539,32 @@ final class MultipartParser { } len += previous.readableByteCount(); } - emit.forEach(buffer -> MultipartParser.this.listener.onBody(buffer, false)); + emit.forEach(buffer -> invokeListener(buffer, false)); } private void flush() { for (Iterator iterator = this.queue.iterator(); iterator.hasNext(); ) { DataBuffer buffer = iterator.next(); boolean last = !iterator.hasNext(); - MultipartParser.this.listener.onBody(buffer, last); + invokeListener(buffer, last); } this.queue.clear(); } + private void invokeListener(DataBuffer buffer, boolean last) { + try { + MultipartParser.this.listener.onBody(buffer, last); + } + catch (Throwable ex) { + dispose(); + throw ex; + } + } + @Override public void complete() { - changeState(DisposedState.INSTANCE, null); - String msg = "Could not find end of body (␍␊--" + - new String(MultipartParser.this.boundary, StandardCharsets.UTF_8) + - ")"; - MultipartParser.this.listener.onError(new HttpMessageConversionException(msg)); + MultipartParser.this.handleException("Could not find end of body (␍␊--" + + new String(MultipartParser.this.boundary, StandardCharsets.UTF_8) + ")", null); } @Override diff --git a/spring-web/src/main/java/org/springframework/http/converter/multipart/PartGenerator.java b/spring-web/src/main/java/org/springframework/http/converter/multipart/PartGenerator.java index 90757db2eca..5e1046e13ae 100644 --- a/spring-web/src/main/java/org/springframework/http/converter/multipart/PartGenerator.java +++ b/spring-web/src/main/java/org/springframework/http/converter/multipart/PartGenerator.java @@ -96,8 +96,8 @@ final class PartGenerator implements MultipartParser.PartListener { private static boolean isFormField(HttpHeaders headers) { MediaType contentType = headers.getContentType(); - return (contentType == null || MediaType.TEXT_PLAIN.equalsTypeAndSubtype(contentType)) && - headers.getContentDisposition().getFilename() == null; + return ((contentType == null || MediaType.TEXT_PLAIN.equalsTypeAndSubtype(contentType)) && + headers.getContentDisposition().getFilename() == null); } @Override @@ -133,9 +133,8 @@ final class PartGenerator implements MultipartParser.PartListener { } @Override - public void onError(Throwable error) { + public void onError(Throwable ex) { deleteParts(); - throw new HttpMessageConversionException("Cannot decode multipart body", error); } void addPart(Part part) { @@ -168,7 +167,7 @@ final class PartGenerator implements MultipartParser.PartListener { private interface State { /** - * Invoked when a {@link MultipartParser.PartListener#onBody(DataBuffer, boolean)} is received. + * Invoked when the parser receives additional data. */ void onBody(DataBuffer dataBuffer, boolean last); diff --git a/spring-web/src/test/java/org/springframework/http/converter/multipart/MultipartHttpMessageConverterTests.java b/spring-web/src/test/java/org/springframework/http/converter/multipart/MultipartHttpMessageConverterTests.java index 8cbc5988ad7..14200fd12ae 100644 --- a/spring-web/src/test/java/org/springframework/http/converter/multipart/MultipartHttpMessageConverterTests.java +++ b/spring-web/src/test/java/org/springframework/http/converter/multipart/MultipartHttpMessageConverterTests.java @@ -200,7 +200,7 @@ class MultipartHttpMessageConverterTests { void readMultipartInvalid() throws Exception { MockHttpInputMessage response = createMultipartResponse("garbage-1.multipart", "boundary"); assertThatThrownBy(() -> converter.read(ResolvableType.forClassWithGenerics(MultiValueMap.class, String.class, Part.class), response, null)) - .isInstanceOf(HttpMessageConversionException.class).hasMessage("Cannot decode multipart body"); + .isInstanceOf(HttpMessageConversionException.class).hasMessage("Could not find first boundary"); } @Test diff --git a/spring-web/src/test/java/org/springframework/http/converter/multipart/MultipartParserTests.java b/spring-web/src/test/java/org/springframework/http/converter/multipart/MultipartParserTests.java index 067ee7cd771..d324cb117e3 100644 --- a/spring-web/src/test/java/org/springframework/http/converter/multipart/MultipartParserTests.java +++ b/spring-web/src/test/java/org/springframework/http/converter/multipart/MultipartParserTests.java @@ -38,6 +38,7 @@ import org.springframework.http.converter.HttpMessageConversionException; import static java.nio.charset.StandardCharsets.UTF_8; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; /** * Tests for {@link MultipartParser}. @@ -74,7 +75,9 @@ class MultipartParserTests { @Test void noEndBoundary() throws Exception { TestListener listener = new TestListener(); - parse("no-end-boundary.multipart", "boundary", listener); + assertThatThrownBy(() -> parse("no-end-boundary.multipart", "boundary", listener)) + .isInstanceOf(HttpMessageConversionException.class) + .hasMessage("Could not find end of headers"); assertThat(listener.error).isInstanceOf(HttpMessageConversionException.class); } @@ -82,7 +85,9 @@ class MultipartParserTests { @Test void garbage() throws Exception { TestListener listener = new TestListener(); - parse("garbage-1.multipart", "boundary", listener); + assertThatThrownBy(() -> parse("garbage-1.multipart", "boundary", listener)) + .isInstanceOf(HttpMessageConversionException.class) + .hasMessage("Could not find first boundary"); assertThat(listener.error).isInstanceOf(HttpMessageConversionException.class); } @@ -90,7 +95,9 @@ class MultipartParserTests { @Test void noEndHeader() throws Exception { TestListener listener = new TestListener(); - parse("no-end-header.multipart", "boundary", listener); + assertThatThrownBy(() -> parse("no-end-header.multipart", "boundary", listener)) + .isInstanceOf(HttpMessageConversionException.class) + .hasMessage("Could not find end of headers"); assertThat(listener.error).isInstanceOf(HttpMessageConversionException.class); } @@ -98,7 +105,9 @@ class MultipartParserTests { @Test void noEndBody() throws Exception { TestListener listener = new TestListener(); - parse("no-end-body.multipart", "boundary", listener); + assertThatThrownBy(() -> parse("no-end-body.multipart", "boundary", listener)) + .isInstanceOf(HttpMessageConversionException.class) + .hasMessage("Could not find end of body (␍␊--boundary)"); assertThat(listener.error).isInstanceOf(HttpMessageConversionException.class); }