mirror of
https://github.com/apache/struts.git
synced 2026-08-06 07:06:58 +00:00
Fixes readStream method to avoid to memory leaks
This commit is contained in:
Lukasz Lenart
+7
@@ -25,6 +25,7 @@ import org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletDiskFileUpl
|
||||
import org.apache.commons.lang3.StringUtils;
|
||||
import org.apache.logging.log4j.LogManager;
|
||||
import org.apache.logging.log4j.Logger;
|
||||
import org.apache.struts2.dispatcher.LocalizedMessage;
|
||||
|
||||
import java.io.File;
|
||||
import java.io.IOException;
|
||||
@@ -139,6 +140,12 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest {
|
||||
} catch (IOException e) {
|
||||
LOG.warn("Failed to create temporary file for in-memory uploaded item: {}",
|
||||
normalizeSpace(item.getName()), e);
|
||||
|
||||
// Add the error to the errors list for proper user feedback
|
||||
LocalizedMessage errorMessage = buildErrorMessage(e.getClass(), e.getMessage(), new Object[]{item.getName()});
|
||||
if (!errors.contains(errorMessage)) {
|
||||
errors.add(errorMessage);
|
||||
}
|
||||
}
|
||||
} else {
|
||||
UploadedFile uploadedFile = StrutsUploadedFile.Builder
|
||||
|
||||
+6
-5
@@ -82,12 +82,13 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest {
|
||||
}
|
||||
|
||||
private String readStream(InputStream inputStream) throws IOException {
|
||||
ByteArrayOutputStream result = new ByteArrayOutputStream();
|
||||
byte[] buffer = new byte[1024];
|
||||
for (int length; (length = inputStream.read(buffer)) != -1; ) {
|
||||
result.write(buffer, 0, length);
|
||||
try (ByteArrayOutputStream result = new ByteArrayOutputStream()) {
|
||||
byte[] buffer = new byte[1024];
|
||||
for (int length; (length = inputStream.read(buffer)) != -1; ) {
|
||||
result.write(buffer, 0, length);
|
||||
}
|
||||
return result.toString(StandardCharsets.UTF_8);
|
||||
}
|
||||
return result.toString(StandardCharsets.UTF_8);
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
+95
@@ -491,6 +491,101 @@ abstract class AbstractMultiPartRequestTest {
|
||||
.containsExactly("struts.messages.upload.error.FileUploadException");
|
||||
}
|
||||
|
||||
@Test
|
||||
public void cleanupDoesNotClearErrorsList() throws IOException {
|
||||
// given - create a scenario that generates errors
|
||||
String content = formFile("file1", "test1.csv", "1,2,3,4");
|
||||
mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8));
|
||||
|
||||
multiPart.setMaxSize("1"); // Very small to trigger error
|
||||
multiPart.parse(mockRequest, tempDir);
|
||||
|
||||
// Verify errors exist
|
||||
assertThat(multiPart.getErrors()).isNotEmpty();
|
||||
int originalErrorCount = multiPart.getErrors().size();
|
||||
|
||||
// when
|
||||
multiPart.cleanUp();
|
||||
|
||||
// then - errors should remain (cleanup doesn't clear errors)
|
||||
assertThat(multiPart.getErrors()).hasSize(originalErrorCount);
|
||||
}
|
||||
|
||||
@Test
|
||||
public void largeFileUploadHandling() throws IOException {
|
||||
// Test that large files are handled properly
|
||||
StringBuilder largeContent = new StringBuilder();
|
||||
for (int i = 0; i < 1000; i++) {
|
||||
largeContent.append("line").append(i).append(",");
|
||||
}
|
||||
|
||||
String content = formFile("largefile", "large.csv", largeContent.toString()) +
|
||||
endline + "--" + boundary + "--";
|
||||
|
||||
mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8));
|
||||
|
||||
// when
|
||||
multiPart.parse(mockRequest, tempDir);
|
||||
|
||||
// then - should complete without memory issues
|
||||
assertThat(multiPart.getErrors()).isEmpty();
|
||||
assertThat(multiPart.getFile("largefile")).hasSize(1);
|
||||
|
||||
// Cleanup should properly handle large files
|
||||
multiPart.cleanUp();
|
||||
assertThat(multiPart.uploadedFiles).isEmpty();
|
||||
}
|
||||
|
||||
@Test
|
||||
public void multipleFileUploadWithMixedContent() throws IOException {
|
||||
// Test mixed content with multiple files and parameters
|
||||
String content = formFile("file1", "test1.csv", "1,2,3,4") +
|
||||
formField("param1", "value1") +
|
||||
formFile("file2", "test2.csv", "5,6,7,8") +
|
||||
formField("param2", "value2") +
|
||||
formFile("file3", "test3.csv", "9,10,11,12") +
|
||||
formField("param3", "value3") +
|
||||
endline + "--" + boundary + "--";
|
||||
|
||||
mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8));
|
||||
|
||||
// when
|
||||
multiPart.parse(mockRequest, tempDir);
|
||||
|
||||
// then - verify all content was processed
|
||||
assertThat(multiPart.getErrors()).isEmpty();
|
||||
assertThat(multiPart.getFile("file1")).hasSize(1);
|
||||
assertThat(multiPart.getFile("file2")).hasSize(1);
|
||||
assertThat(multiPart.getFile("file3")).hasSize(1);
|
||||
assertThat(multiPart.getParameter("param1")).isEqualTo("value1");
|
||||
assertThat(multiPart.getParameter("param2")).isEqualTo("value2");
|
||||
assertThat(multiPart.getParameter("param3")).isEqualTo("value3");
|
||||
|
||||
// Store file paths for post-cleanup verification
|
||||
List<String> filePaths = new ArrayList<>();
|
||||
for (UploadedFile file : multiPart.getFile("file1")) {
|
||||
filePaths.add(file.getAbsolutePath());
|
||||
}
|
||||
for (UploadedFile file : multiPart.getFile("file2")) {
|
||||
filePaths.add(file.getAbsolutePath());
|
||||
}
|
||||
for (UploadedFile file : multiPart.getFile("file3")) {
|
||||
filePaths.add(file.getAbsolutePath());
|
||||
}
|
||||
|
||||
// when - cleanup
|
||||
multiPart.cleanUp();
|
||||
|
||||
// then - verify complete cleanup
|
||||
assertThat(multiPart.uploadedFiles).isEmpty();
|
||||
assertThat(multiPart.parameters).isEmpty();
|
||||
|
||||
// Verify files are deleted
|
||||
for (String filePath : filePaths) {
|
||||
assertThat(new File(filePath)).doesNotExist();
|
||||
}
|
||||
}
|
||||
|
||||
protected String formFile(String fieldName, String filename, String content) {
|
||||
return endline +
|
||||
"--" + boundary + endline +
|
||||
|
||||
+194
@@ -18,6 +18,19 @@
|
||||
*/
|
||||
package org.apache.struts2.dispatcher.multipart;
|
||||
|
||||
import org.apache.commons.fileupload2.core.DiskFileItem;
|
||||
import org.apache.struts2.dispatcher.LocalizedMessage;
|
||||
import org.junit.Test;
|
||||
|
||||
import java.io.File;
|
||||
import java.io.IOException;
|
||||
import java.lang.reflect.Field;
|
||||
import java.nio.charset.StandardCharsets;
|
||||
import java.util.List;
|
||||
|
||||
import static org.apache.commons.lang3.StringUtils.normalizeSpace;
|
||||
import static org.assertj.core.api.Assertions.assertThat;
|
||||
|
||||
public class JakartaMultiPartRequestTest extends AbstractMultiPartRequestTest {
|
||||
|
||||
@Override
|
||||
@@ -25,4 +38,185 @@ public class JakartaMultiPartRequestTest extends AbstractMultiPartRequestTest {
|
||||
return new JakartaMultiPartRequest();
|
||||
}
|
||||
|
||||
@Test
|
||||
public void temporaryFileCleanupForInMemoryUploads() throws IOException, NoSuchFieldException, IllegalAccessException {
|
||||
// given - small files that will be in-memory
|
||||
String content = formFile("file1", "test1.csv", "a,b,c,d") +
|
||||
formFile("file2", "test2.csv", "1,2,3,4") +
|
||||
endline + "--" + boundary + "--";
|
||||
|
||||
mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8));
|
||||
|
||||
// when
|
||||
multiPart.parse(mockRequest, tempDir);
|
||||
|
||||
// Access private field to verify temporary files are tracked
|
||||
Field tempFilesField = JakartaMultiPartRequest.class.getDeclaredField("temporaryFiles");
|
||||
tempFilesField.setAccessible(true);
|
||||
@SuppressWarnings("unchecked")
|
||||
List<File> temporaryFiles = (List<File>) tempFilesField.get(multiPart);
|
||||
|
||||
// Store file paths before cleanup for verification
|
||||
List<String> tempFilePaths = temporaryFiles.stream()
|
||||
.map(File::getAbsolutePath)
|
||||
.toList();
|
||||
|
||||
// Verify temporary files exist before cleanup
|
||||
assertThat(temporaryFiles).isNotEmpty();
|
||||
for (File tempFile : temporaryFiles) {
|
||||
assertThat(tempFile).exists();
|
||||
}
|
||||
|
||||
// when - cleanup
|
||||
multiPart.cleanUp();
|
||||
|
||||
// then - verify files are deleted and tracking list is cleared
|
||||
for (String tempFilePath : tempFilePaths) {
|
||||
assertThat(new File(tempFilePath)).doesNotExist();
|
||||
}
|
||||
assertThat(temporaryFiles).isEmpty();
|
||||
}
|
||||
|
||||
@Test
|
||||
public void cleanupMethodsCanBeOverridden() {
|
||||
// Create a custom implementation to test extensibility
|
||||
class CustomJakartaMultiPartRequest extends JakartaMultiPartRequest {
|
||||
boolean diskFileItemsCleanedUp = false;
|
||||
boolean temporaryFilesCleanedUp = false;
|
||||
|
||||
@Override
|
||||
protected void cleanUpDiskFileItems() {
|
||||
diskFileItemsCleanedUp = true;
|
||||
super.cleanUpDiskFileItems();
|
||||
}
|
||||
|
||||
@Override
|
||||
protected void cleanUpTemporaryFiles() {
|
||||
temporaryFilesCleanedUp = true;
|
||||
super.cleanUpTemporaryFiles();
|
||||
}
|
||||
}
|
||||
|
||||
CustomJakartaMultiPartRequest customMultiPart = new CustomJakartaMultiPartRequest();
|
||||
|
||||
// when
|
||||
customMultiPart.cleanUp();
|
||||
|
||||
// then
|
||||
assertThat(customMultiPart.diskFileItemsCleanedUp).isTrue();
|
||||
assertThat(customMultiPart.temporaryFilesCleanedUp).isTrue();
|
||||
}
|
||||
|
||||
@Test
|
||||
public void temporaryFileCreationFailureAddsError() throws IOException {
|
||||
// Create a custom implementation that simulates temp file creation failure
|
||||
class FaultyJakartaMultiPartRequest extends JakartaMultiPartRequest {
|
||||
@Override
|
||||
protected void processFileField(DiskFileItem item) {
|
||||
// Simulate in-memory upload that fails to create temp file
|
||||
if (item.isInMemory()) {
|
||||
try {
|
||||
// Simulate IOException during temp file creation
|
||||
throw new IOException("Simulated temp file creation failure");
|
||||
} catch (IOException e) {
|
||||
// Add the error to the errors list for proper user feedback
|
||||
LocalizedMessage errorMessage = buildErrorMessage(e.getClass(), e.getMessage(),
|
||||
new Object[]{item.getName()});
|
||||
if (!errors.contains(errorMessage)) {
|
||||
errors.add(errorMessage);
|
||||
}
|
||||
}
|
||||
} else {
|
||||
super.processFileField(item);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
FaultyJakartaMultiPartRequest faultyMultiPart = new FaultyJakartaMultiPartRequest();
|
||||
|
||||
// given - small file that would normally be in-memory
|
||||
String content = formFile("file1", "test1.csv", "a,b") +
|
||||
endline + "--" + boundary + "--";
|
||||
|
||||
mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8));
|
||||
|
||||
// when
|
||||
faultyMultiPart.parse(mockRequest, tempDir);
|
||||
|
||||
// then - verify error is properly captured
|
||||
assertThat(faultyMultiPart.getErrors())
|
||||
.hasSize(1)
|
||||
.first()
|
||||
.extracting(LocalizedMessage::getTextKey)
|
||||
.isEqualTo("struts.messages.upload.error.IOException");
|
||||
}
|
||||
|
||||
@Test
|
||||
public void temporaryFileCreationErrorsAreNotDuplicated() throws IOException {
|
||||
// Test that duplicate errors are not added to the errors list
|
||||
JakartaMultiPartRequest multiPartWithDuplicateErrors = new JakartaMultiPartRequest();
|
||||
|
||||
// Simulate adding the same error twice
|
||||
IOException testException = new IOException("Test exception");
|
||||
LocalizedMessage errorMessage = multiPartWithDuplicateErrors.buildErrorMessage(
|
||||
testException.getClass(), testException.getMessage(), new Object[]{"test.csv"});
|
||||
|
||||
// when - add same error twice
|
||||
multiPartWithDuplicateErrors.errors.add(errorMessage);
|
||||
if (!multiPartWithDuplicateErrors.errors.contains(errorMessage)) {
|
||||
multiPartWithDuplicateErrors.errors.add(errorMessage);
|
||||
}
|
||||
|
||||
// then - only one error should be present
|
||||
assertThat(multiPartWithDuplicateErrors.getErrors()).hasSize(1);
|
||||
}
|
||||
|
||||
@Test
|
||||
public void cleanupIsIdempotent() throws IOException {
|
||||
// given - process some files
|
||||
String content = formFile("file1", "test1.csv", "1,2,3,4") +
|
||||
endline + "--" + boundary + "--";
|
||||
|
||||
mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8));
|
||||
multiPart.parse(mockRequest, tempDir);
|
||||
|
||||
// when - call cleanup multiple times
|
||||
multiPart.cleanUp();
|
||||
multiPart.cleanUp();
|
||||
multiPart.cleanUp();
|
||||
|
||||
// then - should not throw exceptions and should be safe
|
||||
assertThat(multiPart.uploadedFiles).isEmpty();
|
||||
assertThat(multiPart.parameters).isEmpty();
|
||||
}
|
||||
|
||||
@Test
|
||||
public void endToEndMultipartProcessingWithCleanup() throws IOException {
|
||||
// Test complete multipart processing lifecycle
|
||||
String content = formFile("file1", "test1.csv", "1,2,3,4") +
|
||||
formField("param1", "value1") +
|
||||
formFile("file2", "test2.csv", "5,6,7,8") +
|
||||
formField("param2", "value2") +
|
||||
endline + "--" + boundary + "--";
|
||||
|
||||
mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8));
|
||||
|
||||
// when - full processing
|
||||
multiPart.parse(mockRequest, tempDir);
|
||||
|
||||
// then - verify everything was processed
|
||||
assertThat(multiPart.getErrors()).isEmpty();
|
||||
assertThat(multiPart.getFile("file1")).hasSize(1);
|
||||
assertThat(multiPart.getFile("file2")).hasSize(1);
|
||||
assertThat(multiPart.getParameter("param1")).isEqualTo("value1");
|
||||
assertThat(multiPart.getParameter("param2")).isEqualTo("value2");
|
||||
|
||||
// when - cleanup
|
||||
multiPart.cleanUp();
|
||||
|
||||
// then - verify complete cleanup
|
||||
assertThat(multiPart.uploadedFiles).isEmpty();
|
||||
assertThat(multiPart.parameters).isEmpty();
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
+90
@@ -24,9 +24,13 @@ import org.assertj.core.api.InstanceOfAssertFactories;
|
||||
import org.junit.Test;
|
||||
|
||||
import java.io.IOException;
|
||||
import java.io.InputStream;
|
||||
import java.lang.reflect.InvocationTargetException;
|
||||
import java.lang.reflect.Method;
|
||||
import java.nio.charset.StandardCharsets;
|
||||
|
||||
import static org.assertj.core.api.Assertions.assertThat;
|
||||
import static org.assertj.core.api.Assertions.assertThatThrownBy;
|
||||
|
||||
public class JakartaStreamMultiPartRequestTest extends AbstractMultiPartRequestTest {
|
||||
|
||||
@@ -71,4 +75,90 @@ public class JakartaStreamMultiPartRequestTest extends AbstractMultiPartRequestT
|
||||
.containsExactly("struts.messages.upload.error.FileUploadSizeException");
|
||||
}
|
||||
|
||||
@Test
|
||||
public void readStreamProperlyHandlesResources() throws Exception {
|
||||
// Create a test input stream with known data
|
||||
byte[] testData = "test data for stream reading".getBytes(StandardCharsets.UTF_8);
|
||||
InputStream testStream = new java.io.ByteArrayInputStream(testData);
|
||||
|
||||
JakartaStreamMultiPartRequest streamMultiPart = new JakartaStreamMultiPartRequest();
|
||||
|
||||
// Use reflection to access private readStream method
|
||||
Method readStreamMethod = JakartaStreamMultiPartRequest.class.getDeclaredMethod("readStream", InputStream.class);
|
||||
readStreamMethod.setAccessible(true);
|
||||
|
||||
// when
|
||||
String result = (String) readStreamMethod.invoke(streamMultiPart, testStream);
|
||||
|
||||
// then
|
||||
assertThat(result).isEqualTo("test data for stream reading");
|
||||
}
|
||||
|
||||
@Test
|
||||
public void readStreamHandlesExceptionsProperly() throws Exception {
|
||||
// Create a stream that throws an exception
|
||||
InputStream faultyStream = new InputStream() {
|
||||
@Override
|
||||
public int read() throws IOException {
|
||||
throw new IOException("Simulated stream failure");
|
||||
}
|
||||
};
|
||||
|
||||
JakartaStreamMultiPartRequest streamMultiPart = new JakartaStreamMultiPartRequest();
|
||||
|
||||
// Use reflection to access private readStream method
|
||||
Method readStreamMethod = JakartaStreamMultiPartRequest.class.getDeclaredMethod("readStream", InputStream.class);
|
||||
readStreamMethod.setAccessible(true);
|
||||
|
||||
// when/then - should propagate the exception
|
||||
assertThatThrownBy(() -> readStreamMethod.invoke(streamMultiPart, faultyStream))
|
||||
.isInstanceOf(InvocationTargetException.class)
|
||||
.cause()
|
||||
.isInstanceOf(IOException.class)
|
||||
.hasMessage("Simulated stream failure");
|
||||
}
|
||||
|
||||
@Test
|
||||
public void readStreamHandlesEmptyStream() throws Exception {
|
||||
// Create an empty stream
|
||||
InputStream emptyStream = new java.io.ByteArrayInputStream(new byte[0]);
|
||||
|
||||
JakartaStreamMultiPartRequest streamMultiPart = new JakartaStreamMultiPartRequest();
|
||||
|
||||
// Use reflection to access private readStream method
|
||||
Method readStreamMethod = JakartaStreamMultiPartRequest.class.getDeclaredMethod("readStream", InputStream.class);
|
||||
readStreamMethod.setAccessible(true);
|
||||
|
||||
// when
|
||||
String result = (String) readStreamMethod.invoke(streamMultiPart, emptyStream);
|
||||
|
||||
// then
|
||||
assertThat(result).isEmpty();
|
||||
}
|
||||
|
||||
@Test
|
||||
public void readStreamHandlesLargeData() throws Exception {
|
||||
// Create a large data stream to test buffer handling
|
||||
StringBuilder largeData = new StringBuilder();
|
||||
for (int i = 0; i < 2000; i++) {
|
||||
largeData.append("line").append(i).append("\n");
|
||||
}
|
||||
|
||||
byte[] testData = largeData.toString().getBytes(StandardCharsets.UTF_8);
|
||||
InputStream largeStream = new java.io.ByteArrayInputStream(testData);
|
||||
|
||||
JakartaStreamMultiPartRequest streamMultiPart = new JakartaStreamMultiPartRequest();
|
||||
|
||||
// Use reflection to access private readStream method
|
||||
Method readStreamMethod = JakartaStreamMultiPartRequest.class.getDeclaredMethod("readStream", InputStream.class);
|
||||
readStreamMethod.setAccessible(true);
|
||||
|
||||
// when
|
||||
String result = (String) readStreamMethod.invoke(streamMultiPart, largeStream);
|
||||
|
||||
// then
|
||||
assertThat(result).isEqualTo(largeData.toString());
|
||||
assertThat(result.length()).isGreaterThan(1024); // Verify it's larger than internal buffer
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user