From cf5b86369d91dcba00cbd8e796b7bace2aeb5c75 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Wed, 12 Jan 2022 16:31:39 +0100 Subject: [PATCH 1/3] Explicitly close InputStream after resolution in RequestPartMethodArgumentResolver Closes gh-27773 --- ...essageConverterMethodArgumentResolver.java | 18 +++++- .../RequestPartMethodArgumentResolver.java | 17 ++++- ...equestPartMethodArgumentResolverTests.java | 64 ++++++++++++------- 3 files changed, 73 insertions(+), 26 deletions(-) diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/AbstractMessageConverterMethodArgumentResolver.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/AbstractMessageConverterMethodArgumentResolver.java index 1dbc559e2c..535ed21608 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/AbstractMessageConverterMethodArgumentResolver.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/AbstractMessageConverterMethodArgumentResolver.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2021 the original author or authors. + * Copyright 2002-2022 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -169,7 +169,7 @@ public abstract class AbstractMessageConverterMethodArgumentResolver implements HttpMethod httpMethod = (inputMessage instanceof HttpRequest ? ((HttpRequest) inputMessage).getMethod() : null); Object body = NO_VALUE; - EmptyBodyCheckingHttpInputMessage message; + EmptyBodyCheckingHttpInputMessage message = null; try { message = new EmptyBodyCheckingHttpInputMessage(inputMessage); @@ -196,6 +196,11 @@ public abstract class AbstractMessageConverterMethodArgumentResolver implements catch (IOException ex) { throw new HttpMessageNotReadableException("I/O error while reading input message", ex, inputMessage); } + finally { + if (message != null && message.hasBody()) { + closeStreamIfNecessary(message.getBody()); + } + } if (body == NO_VALUE) { if (httpMethod == null || !SUPPORTED_METHODS.contains(httpMethod) || @@ -298,6 +303,15 @@ public abstract class AbstractMessageConverterMethodArgumentResolver implements return arg; } + /** + * Allow for closing the body stream if necessary, + * e.g. for part streams in a multipart request. + */ + void closeStreamIfNecessary(InputStream body) { + // No-op by default: A standard HttpInputMessage exposes the HTTP request stream + // (ServletRequest#getInputStream), with its lifecycle managed by the container. + } + private static class EmptyBodyCheckingHttpInputMessage implements HttpInputMessage { diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/RequestPartMethodArgumentResolver.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/RequestPartMethodArgumentResolver.java index e828239d71..eff6744eba 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/RequestPartMethodArgumentResolver.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/RequestPartMethodArgumentResolver.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2021 the original author or authors. + * Copyright 2002-2022 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -16,6 +16,8 @@ package org.springframework.web.servlet.mvc.method.annotation; +import java.io.IOException; +import java.io.InputStream; import java.util.List; import javax.servlet.http.HttpServletRequest; @@ -180,4 +182,17 @@ public class RequestPartMethodArgumentResolver extends AbstractMessageConverterM return partName; } + @Override + void closeStreamIfNecessary(InputStream body) { + // RequestPartServletServerHttpRequest exposes individual part streams, + // potentially from temporary files -> explicit close call after resolution + // in order to prevent file descriptor leaks. + try { + body.close(); + } + catch (IOException ex) { + // ignore + } + } + } diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestPartMethodArgumentResolverTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestPartMethodArgumentResolverTests.java index 9cf34618a6..cbde3c20b2 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestPartMethodArgumentResolverTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestPartMethodArgumentResolverTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2021 the original author or authors. + * Copyright 2002-2022 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -16,6 +16,9 @@ package org.springframework.web.servlet.mvc.method.annotation; +import java.io.FilterInputStream; +import java.io.IOException; +import java.io.InputStream; import java.lang.reflect.Method; import java.nio.charset.StandardCharsets; import java.util.Arrays; @@ -83,6 +86,8 @@ public class RequestPartMethodArgumentResolverTests { private MultipartFile multipartFile2; + private CloseTrackingInputStream trackedStream; + private MockMultipartHttpServletRequest multipartRequest; private NativeWebRequest webRequest; @@ -116,7 +121,14 @@ public class RequestPartMethodArgumentResolverTests { reset(messageConverter); byte[] content = "doesn't matter as long as not empty".getBytes(StandardCharsets.UTF_8); - multipartFile1 = new MockMultipartFile("requestPart", "", "text/plain", content); + multipartFile1 = new MockMultipartFile("requestPart", "", "text/plain", content) { + @Override + public InputStream getInputStream() throws IOException { + CloseTrackingInputStream in = new CloseTrackingInputStream(super.getInputStream()); + trackedStream = in; + return in; + } + }; multipartFile2 = new MockMultipartFile("requestPart", "", "text/plain", content); multipartRequest = new MockMultipartHttpServletRequest(); multipartRequest.addFile(multipartFile1); @@ -182,8 +194,7 @@ public class RequestPartMethodArgumentResolverTests { @Test public void resolveMultipartFileList() throws Exception { Object actual = resolver.resolveArgument(paramMultipartFileList, null, webRequest, null); - boolean condition = actual instanceof List; - assertThat(condition).isTrue(); + assertThat(actual instanceof List).isTrue(); assertThat(actual).isEqualTo(Arrays.asList(multipartFile1, multipartFile2)); } @@ -191,8 +202,7 @@ public class RequestPartMethodArgumentResolverTests { public void resolveMultipartFileArray() throws Exception { Object actual = resolver.resolveArgument(paramMultipartFileArray, null, webRequest, null); assertThat(actual).isNotNull(); - boolean condition = actual instanceof MultipartFile[]; - assertThat(condition).isTrue(); + assertThat(actual instanceof MultipartFile[]).isTrue(); MultipartFile[] parts = (MultipartFile[]) actual; assertThat(parts.length).isEqualTo(2); assertThat(multipartFile1).isEqualTo(parts[0]); @@ -209,8 +219,7 @@ public class RequestPartMethodArgumentResolverTests { Object result = resolver.resolveArgument(paramMultipartFileNotAnnot, null, webRequest, null); - boolean condition = result instanceof MultipartFile; - assertThat(condition).isTrue(); + assertThat(result instanceof MultipartFile).isTrue(); assertThat(result).as("Invalid result").isEqualTo(expected); } @@ -225,8 +234,7 @@ public class RequestPartMethodArgumentResolverTests { webRequest = new ServletWebRequest(request); Object result = resolver.resolveArgument(paramPart, null, webRequest, null); - boolean condition = result instanceof Part; - assertThat(condition).isTrue(); + assertThat(result instanceof Part).isTrue(); assertThat(result).as("Invalid result").isEqualTo(expected); } @@ -243,8 +251,7 @@ public class RequestPartMethodArgumentResolverTests { webRequest = new ServletWebRequest(request); Object result = resolver.resolveArgument(paramPartList, null, webRequest, null); - boolean condition = result instanceof List; - assertThat(condition).isTrue(); + assertThat(result instanceof List).isTrue(); assertThat(result).isEqualTo(Arrays.asList(part1, part2)); } @@ -261,8 +268,7 @@ public class RequestPartMethodArgumentResolverTests { webRequest = new ServletWebRequest(request); Object result = resolver.resolveArgument(paramPartArray, null, webRequest, null); - boolean condition = result instanceof Part[]; - assertThat(condition).isTrue(); + assertThat(result instanceof Part[]).isTrue(); Part[] parts = (Part[]) result; assertThat(parts.length).isEqualTo(2); assertThat(part1).isEqualTo(parts[0]); @@ -357,8 +363,7 @@ public class RequestPartMethodArgumentResolverTests { assertThat(((Optional) actualValue).get()).as("Invalid result").isEqualTo(expected); actualValue = resolver.resolveArgument(optionalMultipartFile, null, webRequest, null); - boolean condition = actualValue instanceof Optional; - assertThat(condition).isTrue(); + assertThat(actualValue instanceof Optional).isTrue(); assertThat(((Optional) actualValue).get()).as("Invalid result").isEqualTo(expected); } @@ -399,8 +404,7 @@ public class RequestPartMethodArgumentResolverTests { assertThat(((Optional) actualValue).get()).as("Invalid result").isEqualTo(Collections.singletonList(expected)); actualValue = resolver.resolveArgument(optionalMultipartFileList, null, webRequest, null); - boolean condition = actualValue instanceof Optional; - assertThat(condition).isTrue(); + assertThat(actualValue instanceof Optional).isTrue(); assertThat(((Optional) actualValue).get()).as("Invalid result").isEqualTo(Collections.singletonList(expected)); } @@ -443,8 +447,7 @@ public class RequestPartMethodArgumentResolverTests { assertThat(((Optional) actualValue).get()).as("Invalid result").isEqualTo(expected); actualValue = resolver.resolveArgument(optionalPart, null, webRequest, null); - boolean condition = actualValue instanceof Optional; - assertThat(condition).isTrue(); + assertThat(actualValue instanceof Optional).isTrue(); assertThat(((Optional) actualValue).get()).as("Invalid result").isEqualTo(expected); } @@ -489,8 +492,7 @@ public class RequestPartMethodArgumentResolverTests { assertThat(((Optional) actualValue).get()).as("Invalid result").isEqualTo(Collections.singletonList(expected)); actualValue = resolver.resolveArgument(optionalPartList, null, webRequest, null); - boolean condition = actualValue instanceof Optional; - assertThat(condition).isTrue(); + assertThat(actualValue instanceof Optional).isTrue(); assertThat(((Optional) actualValue).get()).as("Invalid result").isEqualTo(Collections.singletonList(expected)); } @@ -572,6 +574,7 @@ public class RequestPartMethodArgumentResolverTests { Object actualValue = resolver.resolveArgument(parameter, mavContainer, webRequest, new ValidatingBinderFactory()); assertThat(actualValue).as("Invalid argument value").isEqualTo(argValue); assertThat(mavContainer.isRequestHandled()).as("The requestHandled flag shouldn't change").isFalse(); + assertThat(trackedStream != null && trackedStream.closed).isTrue(); } @@ -591,7 +594,7 @@ public class RequestPartMethodArgumentResolverTests { } - private final class ValidatingBinderFactory implements WebDataBinderFactory { + private static class ValidatingBinderFactory implements WebDataBinderFactory { @Override public WebDataBinder createBinder(NativeWebRequest webRequest, @Nullable Object target, @@ -606,6 +609,21 @@ public class RequestPartMethodArgumentResolverTests { } + private static class CloseTrackingInputStream extends FilterInputStream { + + public boolean closed = false; + + public CloseTrackingInputStream(InputStream in) { + super(in); + } + + @Override + public void close() { + this.closed = true; + } + } + + @SuppressWarnings("unused") public void handle( @RequestPart SimpleBean requestPart, From 6b3052200a2a476a3fdbc41dc4f3cbf2080dfc14 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Wed, 12 Jan 2022 16:35:42 +0100 Subject: [PATCH 2/3] Skip "data:" lines without content Closes gh-27923 --- .../ServerSentEventHttpMessageReader.java | 22 ++++++++------- ...ServerSentEventHttpMessageReaderTests.java | 28 ++++++++++--------- 2 files changed, 27 insertions(+), 23 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/http/codec/ServerSentEventHttpMessageReader.java b/spring-web/src/main/java/org/springframework/http/codec/ServerSentEventHttpMessageReader.java index d21ec4fe61..f012b92839 100644 --- a/spring-web/src/main/java/org/springframework/http/codec/ServerSentEventHttpMessageReader.java +++ b/spring-web/src/main/java/org/springframework/http/codec/ServerSentEventHttpMessageReader.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2021 the original author or authors. + * Copyright 2002-2022 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -42,6 +42,7 @@ import org.springframework.lang.Nullable; * * @author Sebastien Deleuze * @author Rossen Stoyanchev + * @author Juergen Hoeller * @since 5.0 */ public class ServerSentEventHttpMessageReader implements HttpMessageReader { @@ -140,22 +141,23 @@ public class ServerSentEventHttpMessageReader implements HttpMessageReader lines, ResolvableType valueType, boolean shouldWrap, Map hints) { - ServerSentEvent.Builder sseBuilder = shouldWrap ? ServerSentEvent.builder() : null; + ServerSentEvent.Builder sseBuilder = (shouldWrap ? ServerSentEvent.builder() : null); StringBuilder data = null; StringBuilder comment = null; for (String line : lines) { if (line.startsWith("data:")) { - data = (data != null ? data : new StringBuilder()); - if (line.charAt(5) != ' ') { - data.append(line, 5, line.length()); + int length = line.length(); + if (length > 5) { + int index = (line.charAt(5) != ' ' ? 5 : 6); + if (length > index) { + data = (data != null ? data : new StringBuilder()); + data.append(line, index, line.length()); + data.append('\n'); + } } - else { - data.append(line, 6, line.length()); - } - data.append('\n'); } - if (shouldWrap) { + else if (shouldWrap) { if (line.startsWith("id:")) { sseBuilder.id(line.substring(3).trim()); } diff --git a/spring-web/src/test/java/org/springframework/http/codec/ServerSentEventHttpMessageReaderTests.java b/spring-web/src/test/java/org/springframework/http/codec/ServerSentEventHttpMessageReaderTests.java index 82602a526e..9079ea3896 100644 --- a/spring-web/src/test/java/org/springframework/http/codec/ServerSentEventHttpMessageReaderTests.java +++ b/spring-web/src/test/java/org/springframework/http/codec/ServerSentEventHttpMessageReaderTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2020 the original author or authors. + * Copyright 2002-2022 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -40,6 +40,7 @@ import static org.assertj.core.api.Assertions.assertThat; * Unit tests for {@link ServerSentEventHttpMessageReader}. * * @author Sebastien Deleuze + * @author Juergen Hoeller */ public class ServerSentEventHttpMessageReaderTests extends AbstractLeakCheckingTests { @@ -66,7 +67,7 @@ public class ServerSentEventHttpMessageReaderTests extends AbstractLeakCheckingT MockServerHttpRequest request = MockServerHttpRequest.post("/") .body(Mono.just(stringBuffer( "id:c42\nevent:foo\nretry:123\n:bla\n:bla bla\n:bla bla bla\ndata:bar\n\n" + - "id:c43\nevent:bar\nretry:456\ndata:baz\n\n"))); + "id:c43\nevent:bar\nretry:456\ndata:baz\n\ndata:\n\ndata: \n\n"))); Flux events = this.reader .read(ResolvableType.forClassWithGenerics(ServerSentEvent.class, String.class), @@ -87,6 +88,8 @@ public class ServerSentEventHttpMessageReaderTests extends AbstractLeakCheckingT assertThat(event.comment()).isNull(); assertThat(event.data()).isEqualTo("baz"); }) + .consumeNextWith(event -> assertThat(event.data()).isNull()) + .consumeNextWith(event -> assertThat(event.data()).isNull()) .expectComplete() .verify(); } @@ -175,7 +178,7 @@ public class ServerSentEventHttpMessageReaderTests extends AbstractLeakCheckingT .verify(); } - @Test // gh-24389 + @Test // gh-24389 void readPojoWithCommentOnly() { MockServerHttpRequest request = MockServerHttpRequest.post("/") .body(Flux.just(stringBuffer(":ping\n"), stringBuffer("\n"))); @@ -221,7 +224,6 @@ public class ServerSentEventHttpMessageReaderTests extends AbstractLeakCheckingT @Test public void maxInMemoryLimit() { - this.reader.setMaxInMemorySize(17); MockServerHttpRequest request = MockServerHttpRequest.post("/") @@ -235,9 +237,8 @@ public class ServerSentEventHttpMessageReaderTests extends AbstractLeakCheckingT .verify(); } - @Test // gh-24312 + @Test // gh-24312 public void maxInMemoryLimitAllowsReadingPojoLargerThanDefaultSize() { - int limit = this.jsonDecoder.getMaxInMemorySize(); String fooValue = getStringOfSize(limit) + "and then some more"; @@ -259,13 +260,6 @@ public class ServerSentEventHttpMessageReaderTests extends AbstractLeakCheckingT .verify(); } - private static String getStringOfSize(long size) { - StringBuilder content = new StringBuilder("Aa"); - while (content.length() < size) { - content.append(content); - } - return content.toString(); - } private DataBuffer stringBuffer(String value) { byte[] bytes = value.getBytes(StandardCharsets.UTF_8); @@ -274,4 +268,12 @@ public class ServerSentEventHttpMessageReaderTests extends AbstractLeakCheckingT return buffer; } + private static String getStringOfSize(long size) { + StringBuilder content = new StringBuilder("Aa"); + while (content.length() < size) { + content.append(content); + } + return content.toString(); + } + } From 67c4b4182f787a821a9ff9013b4dddac541e6547 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Wed, 12 Jan 2022 16:36:18 +0100 Subject: [PATCH 3/3] Upgrade to Hibernate ORM 5.4.33, Hibernate Validator 6.2.1, Mockito 4.2, AssertJ 3.22, HtmlUnit 2.56, XMLUnit 2.8.4 --- build.gradle | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/build.gradle b/build.gradle index da794c5b62..6012af4ccc 100644 --- a/build.gradle +++ b/build.gradle @@ -123,8 +123,8 @@ configure(allprojects) { project -> dependency "net.sf.ehcache:ehcache:2.10.6" dependency "org.ehcache:jcache:1.0.1" dependency "org.ehcache:ehcache:3.4.0" - dependency "org.hibernate:hibernate-core:5.4.32.Final" - dependency "org.hibernate:hibernate-validator:6.2.0.Final" + dependency "org.hibernate:hibernate-core:5.4.33.Final" + dependency "org.hibernate:hibernate-validator:6.2.1.Final" dependency "org.webjars:webjars-locator-core:0.48" dependency "org.webjars:underscorejs:1.8.3" @@ -191,14 +191,14 @@ configure(allprojects) { project -> dependency "org.junit.support:testng-engine:1.0.1" dependency "org.hamcrest:hamcrest:2.1" dependency "org.awaitility:awaitility:3.1.6" - dependency "org.assertj:assertj-core:3.21.0" - dependencySet(group: 'org.xmlunit', version: '2.8.3') { + dependency "org.assertj:assertj-core:3.22.0" + dependencySet(group: 'org.xmlunit', version: '2.8.4') { entry 'xmlunit-assertj' entry('xmlunit-matchers') { exclude group: "org.hamcrest", name: "hamcrest-core" } } - dependencySet(group: 'org.mockito', version: '4.1.0') { + dependencySet(group: 'org.mockito', version: '4.2.0') { entry('mockito-core') { exclude group: "org.hamcrest", name: "hamcrest-core" } @@ -206,10 +206,10 @@ configure(allprojects) { project -> } dependency "io.mockk:mockk:1.12.1" - dependency("net.sourceforge.htmlunit:htmlunit:2.55.0") { + dependency("net.sourceforge.htmlunit:htmlunit:2.56.0") { exclude group: "commons-logging", name: "commons-logging" } - dependency("org.seleniumhq.selenium:htmlunit-driver:2.55.0") { + dependency("org.seleniumhq.selenium:htmlunit-driver:2.56.0") { exclude group: "commons-logging", name: "commons-logging" } dependency("org.seleniumhq.selenium:selenium-java:3.141.59") {