Skip to content

Commit 359b654

Browse files
committed
Fixes readStream method to avoid to memory leaks
1 parent 201b9e8 commit 359b654

File tree

5 files changed

+392
-5
lines changed

5 files changed

+392
-5
lines changed

core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
import org.apache.commons.lang3.StringUtils;
2626
import org.apache.logging.log4j.LogManager;
2727
import org.apache.logging.log4j.Logger;
28+
import org.apache.struts2.dispatcher.LocalizedMessage;
2829

2930
import java.io.File;
3031
import java.io.IOException;
@@ -139,6 +140,12 @@ protected void processFileField(DiskFileItem item) {
139140
} catch (IOException e) {
140141
LOG.warn("Failed to create temporary file for in-memory uploaded item: {}",
141142
normalizeSpace(item.getName()), e);
143+
144+
// Add the error to the errors list for proper user feedback
145+
LocalizedMessage errorMessage = buildErrorMessage(e.getClass(), e.getMessage(), new Object[]{item.getName()});
146+
if (!errors.contains(errorMessage)) {
147+
errors.add(errorMessage);
148+
}
142149
}
143150
} else {
144151
UploadedFile uploadedFile = StrutsUploadedFile.Builder

core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -82,12 +82,13 @@ protected void processUpload(HttpServletRequest request, String saveDir) throws
8282
}
8383

8484
private String readStream(InputStream inputStream) throws IOException {
85-
ByteArrayOutputStream result = new ByteArrayOutputStream();
86-
byte[] buffer = new byte[1024];
87-
for (int length; (length = inputStream.read(buffer)) != -1; ) {
88-
result.write(buffer, 0, length);
85+
try (ByteArrayOutputStream result = new ByteArrayOutputStream()) {
86+
byte[] buffer = new byte[1024];
87+
for (int length; (length = inputStream.read(buffer)) != -1; ) {
88+
result.write(buffer, 0, length);
89+
}
90+
return result.toString(StandardCharsets.UTF_8);
8991
}
90-
return result.toString(StandardCharsets.UTF_8);
9192
}
9293

9394
/**

core/src/test/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequestTest.java

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -491,6 +491,101 @@ public void unableParseRequest() throws IOException {
491491
.containsExactly("struts.messages.upload.error.FileUploadException");
492492
}
493493

494+
@Test
495+
public void cleanupDoesNotClearErrorsList() throws IOException {
496+
// given - create a scenario that generates errors
497+
String content = formFile("file1", "test1.csv", "1,2,3,4");
498+
mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8));
499+
500+
multiPart.setMaxSize("1"); // Very small to trigger error
501+
multiPart.parse(mockRequest, tempDir);
502+
503+
// Verify errors exist
504+
assertThat(multiPart.getErrors()).isNotEmpty();
505+
int originalErrorCount = multiPart.getErrors().size();
506+
507+
// when
508+
multiPart.cleanUp();
509+
510+
// then - errors should remain (cleanup doesn't clear errors)
511+
assertThat(multiPart.getErrors()).hasSize(originalErrorCount);
512+
}
513+
514+
@Test
515+
public void largeFileUploadHandling() throws IOException {
516+
// Test that large files are handled properly
517+
StringBuilder largeContent = new StringBuilder();
518+
for (int i = 0; i < 1000; i++) {
519+
largeContent.append("line").append(i).append(",");
520+
}
521+
522+
String content = formFile("largefile", "large.csv", largeContent.toString()) +
523+
endline + "--" + boundary + "--";
524+
525+
mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8));
526+
527+
// when
528+
multiPart.parse(mockRequest, tempDir);
529+
530+
// then - should complete without memory issues
531+
assertThat(multiPart.getErrors()).isEmpty();
532+
assertThat(multiPart.getFile("largefile")).hasSize(1);
533+
534+
// Cleanup should properly handle large files
535+
multiPart.cleanUp();
536+
assertThat(multiPart.uploadedFiles).isEmpty();
537+
}
538+
539+
@Test
540+
public void multipleFileUploadWithMixedContent() throws IOException {
541+
// Test mixed content with multiple files and parameters
542+
String content = formFile("file1", "test1.csv", "1,2,3,4") +
543+
formField("param1", "value1") +
544+
formFile("file2", "test2.csv", "5,6,7,8") +
545+
formField("param2", "value2") +
546+
formFile("file3", "test3.csv", "9,10,11,12") +
547+
formField("param3", "value3") +
548+
endline + "--" + boundary + "--";
549+
550+
mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8));
551+
552+
// when
553+
multiPart.parse(mockRequest, tempDir);
554+
555+
// then - verify all content was processed
556+
assertThat(multiPart.getErrors()).isEmpty();
557+
assertThat(multiPart.getFile("file1")).hasSize(1);
558+
assertThat(multiPart.getFile("file2")).hasSize(1);
559+
assertThat(multiPart.getFile("file3")).hasSize(1);
560+
assertThat(multiPart.getParameter("param1")).isEqualTo("value1");
561+
assertThat(multiPart.getParameter("param2")).isEqualTo("value2");
562+
assertThat(multiPart.getParameter("param3")).isEqualTo("value3");
563+
564+
// Store file paths for post-cleanup verification
565+
List<String> filePaths = new ArrayList<>();
566+
for (UploadedFile file : multiPart.getFile("file1")) {
567+
filePaths.add(file.getAbsolutePath());
568+
}
569+
for (UploadedFile file : multiPart.getFile("file2")) {
570+
filePaths.add(file.getAbsolutePath());
571+
}
572+
for (UploadedFile file : multiPart.getFile("file3")) {
573+
filePaths.add(file.getAbsolutePath());
574+
}
575+
576+
// when - cleanup
577+
multiPart.cleanUp();
578+
579+
// then - verify complete cleanup
580+
assertThat(multiPart.uploadedFiles).isEmpty();
581+
assertThat(multiPart.parameters).isEmpty();
582+
583+
// Verify files are deleted
584+
for (String filePath : filePaths) {
585+
assertThat(new File(filePath)).doesNotExist();
586+
}
587+
}
588+
494589
protected String formFile(String fieldName, String filename, String content) {
495590
return endline +
496591
"--" + boundary + endline +

core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java

Lines changed: 194 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,205 @@
1818
*/
1919
package org.apache.struts2.dispatcher.multipart;
2020

21+
import org.apache.commons.fileupload2.core.DiskFileItem;
22+
import org.apache.struts2.dispatcher.LocalizedMessage;
23+
import org.junit.Test;
24+
25+
import java.io.File;
26+
import java.io.IOException;
27+
import java.lang.reflect.Field;
28+
import java.nio.charset.StandardCharsets;
29+
import java.util.List;
30+
31+
import static org.apache.commons.lang3.StringUtils.normalizeSpace;
32+
import static org.assertj.core.api.Assertions.assertThat;
33+
2134
public class JakartaMultiPartRequestTest extends AbstractMultiPartRequestTest {
2235

2336
@Override
2437
protected AbstractMultiPartRequest createMultipartRequest() {
2538
return new JakartaMultiPartRequest();
2639
}
2740

41+
@Test
42+
public void temporaryFileCleanupForInMemoryUploads() throws IOException, NoSuchFieldException, IllegalAccessException {
43+
// given - small files that will be in-memory
44+
String content = formFile("file1", "test1.csv", "a,b,c,d") +
45+
formFile("file2", "test2.csv", "1,2,3,4") +
46+
endline + "--" + boundary + "--";
47+
48+
mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8));
49+
50+
// when
51+
multiPart.parse(mockRequest, tempDir);
52+
53+
// Access private field to verify temporary files are tracked
54+
Field tempFilesField = JakartaMultiPartRequest.class.getDeclaredField("temporaryFiles");
55+
tempFilesField.setAccessible(true);
56+
@SuppressWarnings("unchecked")
57+
List<File> temporaryFiles = (List<File>) tempFilesField.get(multiPart);
58+
59+
// Store file paths before cleanup for verification
60+
List<String> tempFilePaths = temporaryFiles.stream()
61+
.map(File::getAbsolutePath)
62+
.toList();
63+
64+
// Verify temporary files exist before cleanup
65+
assertThat(temporaryFiles).isNotEmpty();
66+
for (File tempFile : temporaryFiles) {
67+
assertThat(tempFile).exists();
68+
}
69+
70+
// when - cleanup
71+
multiPart.cleanUp();
72+
73+
// then - verify files are deleted and tracking list is cleared
74+
for (String tempFilePath : tempFilePaths) {
75+
assertThat(new File(tempFilePath)).doesNotExist();
76+
}
77+
assertThat(temporaryFiles).isEmpty();
78+
}
79+
80+
@Test
81+
public void cleanupMethodsCanBeOverridden() {
82+
// Create a custom implementation to test extensibility
83+
class CustomJakartaMultiPartRequest extends JakartaMultiPartRequest {
84+
boolean diskFileItemsCleanedUp = false;
85+
boolean temporaryFilesCleanedUp = false;
86+
87+
@Override
88+
protected void cleanUpDiskFileItems() {
89+
diskFileItemsCleanedUp = true;
90+
super.cleanUpDiskFileItems();
91+
}
92+
93+
@Override
94+
protected void cleanUpTemporaryFiles() {
95+
temporaryFilesCleanedUp = true;
96+
super.cleanUpTemporaryFiles();
97+
}
98+
}
99+
100+
CustomJakartaMultiPartRequest customMultiPart = new CustomJakartaMultiPartRequest();
101+
102+
// when
103+
customMultiPart.cleanUp();
104+
105+
// then
106+
assertThat(customMultiPart.diskFileItemsCleanedUp).isTrue();
107+
assertThat(customMultiPart.temporaryFilesCleanedUp).isTrue();
108+
}
109+
110+
@Test
111+
public void temporaryFileCreationFailureAddsError() throws IOException {
112+
// Create a custom implementation that simulates temp file creation failure
113+
class FaultyJakartaMultiPartRequest extends JakartaMultiPartRequest {
114+
@Override
115+
protected void processFileField(DiskFileItem item) {
116+
// Simulate in-memory upload that fails to create temp file
117+
if (item.isInMemory()) {
118+
try {
119+
// Simulate IOException during temp file creation
120+
throw new IOException("Simulated temp file creation failure");
121+
} catch (IOException e) {
122+
// Add the error to the errors list for proper user feedback
123+
LocalizedMessage errorMessage = buildErrorMessage(e.getClass(), e.getMessage(),
124+
new Object[]{item.getName()});
125+
if (!errors.contains(errorMessage)) {
126+
errors.add(errorMessage);
127+
}
128+
}
129+
} else {
130+
super.processFileField(item);
131+
}
132+
}
133+
}
134+
135+
FaultyJakartaMultiPartRequest faultyMultiPart = new FaultyJakartaMultiPartRequest();
136+
137+
// given - small file that would normally be in-memory
138+
String content = formFile("file1", "test1.csv", "a,b") +
139+
endline + "--" + boundary + "--";
140+
141+
mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8));
142+
143+
// when
144+
faultyMultiPart.parse(mockRequest, tempDir);
145+
146+
// then - verify error is properly captured
147+
assertThat(faultyMultiPart.getErrors())
148+
.hasSize(1)
149+
.first()
150+
.extracting(LocalizedMessage::getTextKey)
151+
.isEqualTo("struts.messages.upload.error.IOException");
152+
}
153+
154+
@Test
155+
public void temporaryFileCreationErrorsAreNotDuplicated() throws IOException {
156+
// Test that duplicate errors are not added to the errors list
157+
JakartaMultiPartRequest multiPartWithDuplicateErrors = new JakartaMultiPartRequest();
158+
159+
// Simulate adding the same error twice
160+
IOException testException = new IOException("Test exception");
161+
LocalizedMessage errorMessage = multiPartWithDuplicateErrors.buildErrorMessage(
162+
testException.getClass(), testException.getMessage(), new Object[]{"test.csv"});
163+
164+
// when - add same error twice
165+
multiPartWithDuplicateErrors.errors.add(errorMessage);
166+
if (!multiPartWithDuplicateErrors.errors.contains(errorMessage)) {
167+
multiPartWithDuplicateErrors.errors.add(errorMessage);
168+
}
169+
170+
// then - only one error should be present
171+
assertThat(multiPartWithDuplicateErrors.getErrors()).hasSize(1);
172+
}
173+
174+
@Test
175+
public void cleanupIsIdempotent() throws IOException {
176+
// given - process some files
177+
String content = formFile("file1", "test1.csv", "1,2,3,4") +
178+
endline + "--" + boundary + "--";
179+
180+
mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8));
181+
multiPart.parse(mockRequest, tempDir);
182+
183+
// when - call cleanup multiple times
184+
multiPart.cleanUp();
185+
multiPart.cleanUp();
186+
multiPart.cleanUp();
187+
188+
// then - should not throw exceptions and should be safe
189+
assertThat(multiPart.uploadedFiles).isEmpty();
190+
assertThat(multiPart.parameters).isEmpty();
191+
}
192+
193+
@Test
194+
public void endToEndMultipartProcessingWithCleanup() throws IOException {
195+
// Test complete multipart processing lifecycle
196+
String content = formFile("file1", "test1.csv", "1,2,3,4") +
197+
formField("param1", "value1") +
198+
formFile("file2", "test2.csv", "5,6,7,8") +
199+
formField("param2", "value2") +
200+
endline + "--" + boundary + "--";
201+
202+
mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8));
203+
204+
// when - full processing
205+
multiPart.parse(mockRequest, tempDir);
206+
207+
// then - verify everything was processed
208+
assertThat(multiPart.getErrors()).isEmpty();
209+
assertThat(multiPart.getFile("file1")).hasSize(1);
210+
assertThat(multiPart.getFile("file2")).hasSize(1);
211+
assertThat(multiPart.getParameter("param1")).isEqualTo("value1");
212+
assertThat(multiPart.getParameter("param2")).isEqualTo("value2");
213+
214+
// when - cleanup
215+
multiPart.cleanUp();
216+
217+
// then - verify complete cleanup
218+
assertThat(multiPart.uploadedFiles).isEmpty();
219+
assertThat(multiPart.parameters).isEmpty();
220+
}
221+
28222
}

0 commit comments

Comments
 (0)