This is an automated email from the ASF dual-hosted git repository. robertlazarski pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/axis-axis2-java-core.git
commit 2b98e5a31c793589915711f5e29fb4ffb697de70 Author: Robert Lazarski <[email protected]> AuthorDate: Tue Aug 4 05:59:11 2026 -1000 Delete multipart temporary files instead of accumulating them Second half of the F4 fix from the private 2026-07-30 report. The size ceilings bounded what one request could write; nothing deleted it afterwards, so temp files accumulated for the lifetime of the JVM. The two kinds of part have different lifetimes, so they are handled separately. A form field is fully materialised into the parameter map during the build, so the builder deletes its temp file on the way out. A file part stays readable through the DataHandler the service is about to be handed, so it cannot be deleted during the build; those register with a commons-io FileCleaningTracker and are deleted once the owning DiskFileItem is unreachable. The reaper is a daemon thread created on the first multipart request, so a deployment that never receives one never starts it, and AxisConfiguration .cleanup() stops it so it cannot outlive a redeployment and pin the web application class loader. A tracker that has been shut down is replaced rather than reused, since exitWhenFinished is permanent. AbstractFileUpload.parseRequest already deletes partially-written items when a parse fails, so a request rejected by the new ceilings still cleans up after itself -- verified, because otherwise the limit would have been a cheaper way to litter the temp directory than a successful upload. The 100 MB multipart default stands: far above any plausible legitimate form post, with a per-service override and -1 to restore the old behaviour. Co-Authored-By: Claude Fable 5 <[email protected]> --- .../axis2/builder/MultipartFormDataBuilder.java | 38 +++- .../axis2/builder/MultipartTempFileTracker.java | 93 +++++++++ .../org/apache/axis2/engine/AxisConfiguration.java | 4 + .../builder/MultipartTempFileCleanupTest.java | 223 +++++++++++++++++++++ 4 files changed, 356 insertions(+), 2 deletions(-) diff --git a/modules/kernel/src/org/apache/axis2/builder/MultipartFormDataBuilder.java b/modules/kernel/src/org/apache/axis2/builder/MultipartFormDataBuilder.java index 8a909ce683..262e354c73 100644 --- a/modules/kernel/src/org/apache/axis2/builder/MultipartFormDataBuilder.java +++ b/modules/kernel/src/org/apache/axis2/builder/MultipartFormDataBuilder.java @@ -46,9 +46,13 @@ import org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletFileUpload; import org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletRequestContext; import org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletDiskFileUpload; import org.apache.commons.io.Charsets; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; public class MultipartFormDataBuilder implements Builder { + private static final Log log = LogFactory.getLog(MultipartFormDataBuilder.class); + /** * @return Returns the document element. */ @@ -100,8 +104,18 @@ public class MultipartFormDataBuilder implements Builder { Object value; try { if (isFormField) { - value = getTextParameter(diskFileItem, charSetEncoding); + try { + value = getTextParameter(diskFileItem, charSetEncoding); + } finally { + // The text has been copied into the map, so nothing reads + // this item again. Release any temp file now instead of + // leaving it for the reaper. + deleteQuietly(diskFileItem); + } } else { + // A file part stays readable through the DataHandler the + // service is about to be given, so its temp file can only be + // released once that item is unreachable. value = getFileParameter(diskFileItem); } } catch (Exception ex) { @@ -113,12 +127,32 @@ public class MultipartFormDataBuilder implements Builder { return parameterMap; } + /** + * Release an item's backing temp file, if it has one. A failure here must + * not fail the request: the file is still registered with the reaper, which + * will retry once the item becomes unreachable. + */ + private static void deleteQuietly(DiskFileItem diskFileItem) { + if (diskFileItem.isInMemory()) { + return; + } + try { + diskFileItem.delete(); + } catch (Exception e) { + log.warn("Could not delete the temporary file for multipart field '" + + diskFileItem.getFieldName() + "'; leaving it to the reaper", e); + } + } + private static List parseRequest(JakartaServletRequestContext requestContext, MessageContext messageContext) throws FileUploadException { - // Create a factory for disk-based file items + // Create a factory for disk-based file items. Parts above the factory's + // threshold spill to a temp file; register them so the file goes away + // once the item that owns it is unreachable. DiskFileItemFactory fileItemFactory = DiskFileItemFactory.builder() .setCharset(StandardCharsets.UTF_8) + .setFileCleaningTracker(MultipartTempFileTracker.getTracker()) .get(); JakartaServletFileUpload upload = new JakartaServletFileUpload<>(fileItemFactory); // There must be a limit. diff --git a/modules/kernel/src/org/apache/axis2/builder/MultipartTempFileTracker.java b/modules/kernel/src/org/apache/axis2/builder/MultipartTempFileTracker.java new file mode 100644 index 0000000000..ce5c40ceac --- /dev/null +++ b/modules/kernel/src/org/apache/axis2/builder/MultipartTempFileTracker.java @@ -0,0 +1,93 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.axis2.builder; + +import org.apache.commons.io.FileCleaningTracker; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; + +/** + * Deletes the temporary files that the multipart/form-data builder spills to + * disk once nothing can read them any more. + * + * <p>A multipart part larger than the factory's threshold is written to a temp + * file, and a file part is then handed to the service as a {@code DataHandler} + * that reads it during invocation. That makes the file's useful life longer + * than the build step, so the builder cannot simply delete it on the way out — + * which is why these files previously accumulated for the lifetime of the JVM. + * + * <p>Cleanup is therefore tied to reachability: commons-io tracks each temp file + * against its {@code DiskFileItem} through a phantom reference and deletes the + * file once that item is collected. Form-field parts do not need to wait, and + * the builder deletes those itself as soon as it has copied the text out. + * + * <p>The reaper is a daemon thread created on the first multipart request, so a + * deployment that never receives one never starts it. {@link #shutdown()} stops + * it, and is called when the Axis2 configuration is cleaned up so the thread + * does not outlive a redeployment. + */ +public final class MultipartTempFileTracker { + + private static final Log log = LogFactory.getLog(MultipartTempFileTracker.class); + + private static FileCleaningTracker tracker; + + private MultipartTempFileTracker() { + } + + /** + * The tracker to register temp files with, started on first use. + * + * <p>A tracker that has been shut down cannot be restarted, so this creates + * a fresh one rather than handing back a dead reaper if a configuration is + * cleaned up and another is built in the same JVM. + */ + public static synchronized FileCleaningTracker getTracker() { + if (tracker == null) { + tracker = new FileCleaningTracker(); + if (log.isDebugEnabled()) { + log.debug("Started the multipart temporary file reaper"); + } + } + return tracker; + } + + /** + * Stop the reaper once the files it is still tracking have been deleted. + */ + public static synchronized void shutdown() { + if (tracker != null) { + if (log.isDebugEnabled()) { + log.debug("Stopping the multipart temporary file reaper with " + + tracker.getTrackCount() + " file(s) still tracked"); + } + tracker.exitWhenFinished(); + tracker = null; + } + } + + /** + * How many temp files are still awaiting deletion. Intended for tests and + * diagnostics. + */ + public static synchronized int getTrackedFileCount() { + return tracker == null ? 0 : tracker.getTrackCount(); + } +} diff --git a/modules/kernel/src/org/apache/axis2/engine/AxisConfiguration.java b/modules/kernel/src/org/apache/axis2/engine/AxisConfiguration.java index a3cb4c376b..fd2d8e4606 100644 --- a/modules/kernel/src/org/apache/axis2/engine/AxisConfiguration.java +++ b/modules/kernel/src/org/apache/axis2/engine/AxisConfiguration.java @@ -41,6 +41,7 @@ import org.apache.axis2.AxisFault; import org.apache.axis2.Constants; import org.apache.axis2.transaction.TransactionConfiguration; import org.apache.axis2.builder.Builder; +import org.apache.axis2.builder.MultipartTempFileTracker; import org.apache.axis2.builder.unknowncontent.UnknownContentBuilder; import org.apache.axis2.context.MessageContext; import org.apache.axis2.dataretrieval.AxisDataLocator; @@ -1338,6 +1339,9 @@ public class AxisConfiguration extends AxisDescription { if (configurator != null) { configurator.cleanup(); } + // Stop the multipart temp-file reaper so its thread does not outlive a + // redeployment and pin this web application's class loader. + MultipartTempFileTracker.shutdown(); this.policySupportedModules.clear(); this.moduleConfigmap.clear(); this.allEndpoints.clear(); diff --git a/modules/kernel/test/org/apache/axis2/builder/MultipartTempFileCleanupTest.java b/modules/kernel/test/org/apache/axis2/builder/MultipartTempFileCleanupTest.java new file mode 100644 index 0000000000..1e60291a07 --- /dev/null +++ b/modules/kernel/test/org/apache/axis2/builder/MultipartTempFileCleanupTest.java @@ -0,0 +1,223 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.axis2.builder; + +import jakarta.servlet.ReadListener; +import jakarta.servlet.ServletInputStream; +import jakarta.servlet.http.HttpServletRequest; +import junit.framework.TestCase; +import org.apache.axiom.om.OMElement; +import org.apache.axis2.context.ConfigurationContext; +import org.apache.axis2.context.MessageContext; +import org.apache.axis2.engine.AxisConfiguration; +import org.apache.axis2.kernel.http.HTTPConstants; + +import java.io.ByteArrayInputStream; +import java.io.File; +import java.io.IOException; +import java.nio.charset.StandardCharsets; + +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * Tests that the multipart builder does not leave its temporary files behind. + * + * <p>A part larger than the disk threshold is written to a temp file. Before + * this was addressed, nothing ever deleted those files and they accumulated for + * the lifetime of the JVM. + */ +public class MultipartTempFileCleanupTest extends TestCase { + + private static final String BOUNDARY = "axis2TestBoundary"; + + /** Comfortably above the factory's spill-to-disk threshold. */ + private static final int PART_SIZE = 128 * 1024; + + private File tempDirectory; + + protected void setUp() throws Exception { + super.setUp(); + tempDirectory = new File(System.getProperty("java.io.tmpdir")); + } + + /** + * A form field is fully materialised into the parameter map during the + * build, so its temp file should be gone by the time the builder returns + * rather than waiting on the reaper. + */ + public void testFormFieldTempFileIsDeletedImmediately() throws Exception { + int before = countUploadTempFiles(); + + MultipartFormDataBuilder builder = new MultipartFormDataBuilder(); + builder.processDocument(null, multipartContentType(), + newMessageContext(buildBody("bigField", null))); + + assertEquals("A form field must not leave a temporary file behind", + before, countUploadTempFiles()); + } + + /** + * A file part stays readable through the DataHandler handed to the service, + * so it cannot be deleted during the build — but it must at least be + * registered for deletion. + */ + public void testFilePartIsRegisteredForCleanup() throws Exception { + int trackedBefore = MultipartTempFileTracker.getTrackedFileCount(); + + MultipartFormDataBuilder builder = new MultipartFormDataBuilder(); + OMElement result = builder.processDocument(null, multipartContentType(), + newMessageContext(buildBody("bigFile", "big.bin"))); + + assertNotNull(result); + assertTrue("The file part's temp file should be registered with the reaper", + MultipartTempFileTracker.getTrackedFileCount() > trackedBefore); + } + + /** + * End to end: once the built message is unreachable, the reaper should + * actually remove the file from disk. + */ + public void testFilePartTempFileIsDeletedOnceUnreachable() throws Exception { + int before = countUploadTempFiles(); + + MultipartFormDataBuilder builder = new MultipartFormDataBuilder(); + OMElement result = builder.processDocument(null, multipartContentType(), + newMessageContext(buildBody("bigFile", "big.bin"))); + assertNotNull(result); + assertEquals("The file part should still be on disk while it is readable", + before + 1, countUploadTempFiles()); + + // Drop every reference to the item that owns the file, then let the + // phantom reference the reaper is waiting on become enqueueable. + result = null; + builder = null; + + assertTrue("The reaper should have deleted the temporary file", + awaitTempFileCount(before)); + } + + /** + * Poll for the temp-file count to fall back to the expected value, nudging + * the collector each time, since the reaper only acts once the owning item + * has been collected. + */ + private boolean awaitTempFileCount(int expected) throws InterruptedException { + for (int attempt = 0; attempt < 50; attempt++) { + System.gc(); + if (countUploadTempFiles() <= expected) { + return true; + } + Thread.sleep(100); + } + return false; + } + + private int countUploadTempFiles() { + String[] names = tempDirectory.list(); + if (names == null) { + return 0; + } + int count = 0; + for (int i = 0; i < names.length; i++) { + // commons-fileupload2 names its spill files upload_<uid>_<n>.tmp + if (names[i].startsWith("upload_") && names[i].endsWith(".tmp")) { + count++; + } + } + return count; + } + + private String multipartContentType() { + return "multipart/form-data; boundary=" + BOUNDARY; + } + + /** + * Build a single-part multipart body. Passing a file name makes it a file + * part rather than a plain form field. + */ + private byte[] buildBody(String fieldName, String fileName) { + StringBuilder header = new StringBuilder(); + header.append("--").append(BOUNDARY).append("\r\n"); + header.append("Content-Disposition: form-data; name=\"").append(fieldName).append('"'); + if (fileName != null) { + header.append("; filename=\"").append(fileName).append('"'); + } + header.append("\r\n"); + if (fileName != null) { + header.append("Content-Type: application/octet-stream\r\n"); + } + header.append("\r\n"); + + StringBuilder body = new StringBuilder(header.toString()); + for (int i = 0; i < PART_SIZE; i++) { + body.append('a'); + } + body.append("\r\n--").append(BOUNDARY).append("--\r\n"); + return body.toString().getBytes(StandardCharsets.UTF_8); + } + + private MessageContext newMessageContext(byte[] body) throws Exception { + AxisConfiguration axisConfiguration = new AxisConfiguration(); + ConfigurationContext configurationContext = new ConfigurationContext(axisConfiguration); + MessageContext messageContext = configurationContext.createMessageContext(); + messageContext.setProperty(HTTPConstants.MC_HTTP_SERVLETREQUEST, mockRequest(body)); + return messageContext; + } + + private HttpServletRequest mockRequest(byte[] body) throws IOException { + HttpServletRequest request = mock(HttpServletRequest.class); + when(request.getContentType()).thenReturn(multipartContentType()); + when(request.getCharacterEncoding()).thenReturn("UTF-8"); + when(request.getContentLength()).thenReturn(body.length); + when(request.getContentLengthLong()).thenReturn((long) body.length); + when(request.getInputStream()).thenReturn(new MockServletInputStream(body)); + return request; + } + + private static class MockServletInputStream extends ServletInputStream { + + private final ByteArrayInputStream delegate; + + MockServletInputStream(byte[] body) { + this.delegate = new ByteArrayInputStream(body); + } + + public int read() { + return delegate.read(); + } + + public int read(byte[] b, int off, int len) { + return delegate.read(b, off, len); + } + + public boolean isFinished() { + return delegate.available() == 0; + } + + public boolean isReady() { + return true; + } + + public void setReadListener(ReadListener readListener) { + throw new UnsupportedOperationException(); + } + } +}
