[
https://issues.apache.org/jira/browse/WW-5413?focusedWorklogId=1031662&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1031662
]
ASF GitHub Bot logged work on WW-5413:
--------------------------------------
Author: ASF GitHub Bot
Created on: 22/Jul/26 12:57
Start Date: 22/Jul/26 12:57
Worklog Time Spent: 10m
Work Description: Copilot commented on code in PR #1805:
URL: https://github.com/apache/struts/pull/1805#discussion_r3630364624
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java:
##########
@@ -0,0 +1,198 @@
+/*
+ * 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.struts2.dispatcher.multipart;
+
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.apache.struts2.StrutsException;
+
+import java.io.ByteArrayInputStream;
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.UUID;
+
+/**
+ * In-memory backed {@link UploadedFile} for small multipart uploads that
Commons FileUpload kept
+ * in memory ({@code DiskFileItem.isInMemory() == true}).
+ *
+ * <p>The content is held as a byte array and is written to a temporary file
only the first time a
+ * caller demands a {@link File} through {@link #getContent()} or {@link
#getAbsolutePath()} (lazy
+ * materialization). Callers reading through {@link #getInputStream()} never
touch the disk. The
+ * temporary file uses the secure {@code upload_<uuid>.tmp} naming and ignores
the user-supplied
+ * original filename.</p>
+ *
+ * <p><strong>Clustered deployments:</strong> the target temporary path is
resolved on the node that
+ * created this instance. If an un-materialized instance is serialized (for
example, session
+ * replication) and deserialized on another node, a later {@link
#getContent()} materializes to that
+ * originating node's path, which may not exist on the new node. Read via
{@link #getInputStream()},
+ * which never touches disk, when content must survive cross-node
replication.</p>
+ *
+ * @since 7.3.0
+ */
+public class StrutsInMemoryUploadedFile implements UploadedFile {
+
+ private static final long serialVersionUID = 1L;
+
+ private static final Logger LOG =
LogManager.getLogger(StrutsInMemoryUploadedFile.class);
+
+ private final byte[] content;
+ private final File targetFile;
+ private final String contentType;
+ private final String originalName;
+ private final String inputName;
+
+ private volatile transient File materializedFile;
+
+ private StrutsInMemoryUploadedFile(byte[] content, Path saveDir, String
contentType,
+ String originalName, String inputName) {
+ this.content = content;
+ String name = "upload_" + UUID.randomUUID().toString().replace("-",
"_") + ".tmp";
+ this.targetFile = saveDir.resolve(name).toFile();
+ this.contentType = contentType;
+ this.originalName = originalName;
+ this.inputName = inputName;
+ }
+
+ private synchronized File materialize() {
+ if (materializedFile == null) {
+ try {
+ Files.write(targetFile.toPath(), content);
+ } catch (IOException e) {
+ try {
+ Files.deleteIfExists(targetFile.toPath());
+ } catch (IOException suppressed) {
+ e.addSuppressed(suppressed);
+ }
+ throw new StrutsException("Could not materialize in-memory
uploaded file: " + targetFile.getName(), e);
+ }
+ materializedFile = targetFile;
+ LOG.debug("Materialized in-memory uploaded item to {}",
targetFile.getAbsolutePath());
+ }
+ return materializedFile;
+ }
Review Comment:
`Files.write(targetFile.toPath(), content)` will create-or-truncate an
existing path, which (in the worst case) can enable overwrite/symlink-style
attacks if something manages to pre-create that filename in the upload dir.
Consider opening the file with `StandardOpenOption.CREATE_NEW` (fail if it
exists) and aligning the creation semantics with the existing secure temp-file
creation approach used elsewhere (e.g., create atomically and avoid
overwriting). This also makes the “materialize exactly once” contract more
robust under unexpected filesystem state.
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java:
##########
@@ -0,0 +1,198 @@
+/*
+ * 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.struts2.dispatcher.multipart;
+
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.apache.struts2.StrutsException;
+
+import java.io.ByteArrayInputStream;
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.UUID;
+
+/**
+ * In-memory backed {@link UploadedFile} for small multipart uploads that
Commons FileUpload kept
+ * in memory ({@code DiskFileItem.isInMemory() == true}).
+ *
+ * <p>The content is held as a byte array and is written to a temporary file
only the first time a
+ * caller demands a {@link File} through {@link #getContent()} or {@link
#getAbsolutePath()} (lazy
+ * materialization). Callers reading through {@link #getInputStream()} never
touch the disk. The
+ * temporary file uses the secure {@code upload_<uuid>.tmp} naming and ignores
the user-supplied
+ * original filename.</p>
+ *
+ * <p><strong>Clustered deployments:</strong> the target temporary path is
resolved on the node that
+ * created this instance. If an un-materialized instance is serialized (for
example, session
+ * replication) and deserialized on another node, a later {@link
#getContent()} materializes to that
+ * originating node's path, which may not exist on the new node. Read via
{@link #getInputStream()},
+ * which never touches disk, when content must survive cross-node
replication.</p>
+ *
+ * @since 7.3.0
+ */
+public class StrutsInMemoryUploadedFile implements UploadedFile {
+
+ private static final long serialVersionUID = 1L;
+
+ private static final Logger LOG =
LogManager.getLogger(StrutsInMemoryUploadedFile.class);
+
+ private final byte[] content;
+ private final File targetFile;
+ private final String contentType;
+ private final String originalName;
+ private final String inputName;
+
+ private volatile transient File materializedFile;
+
+ private StrutsInMemoryUploadedFile(byte[] content, Path saveDir, String
contentType,
+ String originalName, String inputName) {
+ this.content = content;
+ String name = "upload_" + UUID.randomUUID().toString().replace("-",
"_") + ".tmp";
+ this.targetFile = saveDir.resolve(name).toFile();
+ this.contentType = contentType;
+ this.originalName = originalName;
+ this.inputName = inputName;
+ }
Review Comment:
The class claims thread-safety/lazy caching, but it stores the
caller-provided `byte[]` by reference. If that array is mutated after
construction (or shared across threads), `getInputStream()`, `length()`, and
eventual materialization can observe inconsistent content, violating
immutability and thread-safety expectations. Recommend defensively copying the
byte array on construction (and optionally rejecting `null` content early) so
`isMissing()` can reflect actual availability and you avoid
`NullPointerException` paths in `getInputStream()`/`length()`.
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java:
##########
@@ -0,0 +1,198 @@
+/*
+ * 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.struts2.dispatcher.multipart;
+
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.apache.struts2.StrutsException;
+
+import java.io.ByteArrayInputStream;
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.UUID;
+
+/**
+ * In-memory backed {@link UploadedFile} for small multipart uploads that
Commons FileUpload kept
+ * in memory ({@code DiskFileItem.isInMemory() == true}).
+ *
+ * <p>The content is held as a byte array and is written to a temporary file
only the first time a
+ * caller demands a {@link File} through {@link #getContent()} or {@link
#getAbsolutePath()} (lazy
+ * materialization). Callers reading through {@link #getInputStream()} never
touch the disk. The
+ * temporary file uses the secure {@code upload_<uuid>.tmp} naming and ignores
the user-supplied
+ * original filename.</p>
+ *
+ * <p><strong>Clustered deployments:</strong> the target temporary path is
resolved on the node that
+ * created this instance. If an un-materialized instance is serialized (for
example, session
+ * replication) and deserialized on another node, a later {@link
#getContent()} materializes to that
+ * originating node's path, which may not exist on the new node. Read via
{@link #getInputStream()},
+ * which never touches disk, when content must survive cross-node
replication.</p>
+ *
+ * @since 7.3.0
+ */
+public class StrutsInMemoryUploadedFile implements UploadedFile {
+
+ private static final long serialVersionUID = 1L;
+
+ private static final Logger LOG =
LogManager.getLogger(StrutsInMemoryUploadedFile.class);
+
+ private final byte[] content;
Review Comment:
The class claims thread-safety/lazy caching, but it stores the
caller-provided `byte[]` by reference. If that array is mutated after
construction (or shared across threads), `getInputStream()`, `length()`, and
eventual materialization can observe inconsistent content, violating
immutability and thread-safety expectations. Recommend defensively copying the
byte array on construction (and optionally rejecting `null` content early) so
`isMissing()` can reflect actual availability and you avoid
`NullPointerException` paths in `getInputStream()`/`length()`.
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java:
##########
@@ -181,20 +174,17 @@ protected void processNormalFormField(DiskFileItem item,
Charset charset) throws
* <ol>
* <li>Validating the file name and field name are not null/empty</li>
* <li>Determining if the file is stored in memory or on disk</li>
- * <li>For in-memory files: creating a temporary file and copying
content</li>
+ * <li>For in-memory files: wrapping the content in a {@link
StrutsInMemoryUploadedFile}
+ * that only writes to disk lazily, on demand</li>
* <li>For disk files: using the existing file directly</li>
* <li>Creating an {@link UploadedFile} abstraction</li>
* <li>Adding the file to the uploaded files collection</li>
* </ol>
- *
- * <p>Temporary files created for in-memory uploads are automatically
- * tracked for cleanup. Any errors during temporary file creation are
- * logged and added to the error list for user feedback.</p>
- *
+ *
* @param item the disk file item representing the uploaded file
- * @see #cleanUpTemporaryFiles()
+ * @throws IOException if an error occurs reading the in-memory item's
content
*/
- protected void processFileField(DiskFileItem item, String saveDir) {
+ protected void processFileField(DiskFileItem item, String saveDir) throws
IOException {
Review Comment:
In the in-memory branch, `item.get()` is now on the hot path with no local
error-to-`errors` translation (the previous eager-write path explicitly
converted `IOException` into a `LocalizedMessage`). If `item.get()` throws,
parsing may now fail with an exception rather than producing a collected upload
error like other failure modes. If the intent is “only materialization failures
move to consumption-time,” consider catching `IOException` specifically around
`item.get()` and mapping it to the existing upload error mechanism (or update
surrounding error-handling docs to reflect the new behavior).
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java:
##########
@@ -0,0 +1,198 @@
+/*
+ * 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.struts2.dispatcher.multipart;
+
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.apache.struts2.StrutsException;
+
+import java.io.ByteArrayInputStream;
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.UUID;
+
+/**
+ * In-memory backed {@link UploadedFile} for small multipart uploads that
Commons FileUpload kept
+ * in memory ({@code DiskFileItem.isInMemory() == true}).
+ *
+ * <p>The content is held as a byte array and is written to a temporary file
only the first time a
+ * caller demands a {@link File} through {@link #getContent()} or {@link
#getAbsolutePath()} (lazy
+ * materialization). Callers reading through {@link #getInputStream()} never
touch the disk. The
+ * temporary file uses the secure {@code upload_<uuid>.tmp} naming and ignores
the user-supplied
+ * original filename.</p>
+ *
+ * <p><strong>Clustered deployments:</strong> the target temporary path is
resolved on the node that
+ * created this instance. If an un-materialized instance is serialized (for
example, session
+ * replication) and deserialized on another node, a later {@link
#getContent()} materializes to that
+ * originating node's path, which may not exist on the new node. Read via
{@link #getInputStream()},
+ * which never touches disk, when content must survive cross-node
replication.</p>
+ *
+ * @since 7.3.0
+ */
+public class StrutsInMemoryUploadedFile implements UploadedFile {
+
+ private static final long serialVersionUID = 1L;
+
+ private static final Logger LOG =
LogManager.getLogger(StrutsInMemoryUploadedFile.class);
+
+ private final byte[] content;
+ private final File targetFile;
+ private final String contentType;
+ private final String originalName;
+ private final String inputName;
+
+ private volatile transient File materializedFile;
+
+ private StrutsInMemoryUploadedFile(byte[] content, Path saveDir, String
contentType,
+ String originalName, String inputName) {
+ this.content = content;
+ String name = "upload_" + UUID.randomUUID().toString().replace("-",
"_") + ".tmp";
+ this.targetFile = saveDir.resolve(name).toFile();
+ this.contentType = contentType;
+ this.originalName = originalName;
+ this.inputName = inputName;
+ }
+
+ private synchronized File materialize() {
+ if (materializedFile == null) {
+ try {
+ Files.write(targetFile.toPath(), content);
+ } catch (IOException e) {
+ try {
+ Files.deleteIfExists(targetFile.toPath());
+ } catch (IOException suppressed) {
+ e.addSuppressed(suppressed);
+ }
+ throw new StrutsException("Could not materialize in-memory
uploaded file: " + targetFile.getName(), e);
+ }
+ materializedFile = targetFile;
+ LOG.debug("Materialized in-memory uploaded item to {}",
targetFile.getAbsolutePath());
+ }
+ return materializedFile;
+ }
+
+ @Override
+ public InputStream getInputStream() {
+ return new ByteArrayInputStream(content);
+ }
+
+ @Override
+ public boolean isMissing() {
+ return false;
+ }
+
+ @Override
+ public Long length() {
+ return (long) content.length;
+ }
Review Comment:
The class claims thread-safety/lazy caching, but it stores the
caller-provided `byte[]` by reference. If that array is mutated after
construction (or shared across threads), `getInputStream()`, `length()`, and
eventual materialization can observe inconsistent content, violating
immutability and thread-safety expectations. Recommend defensively copying the
byte array on construction (and optionally rejecting `null` content early) so
`isMissing()` can reflect actual availability and you avoid
`NullPointerException` paths in `getInputStream()`/`length()`.
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java:
##########
@@ -215,40 +205,14 @@ protected void processFileField(DiskFileItem item, String
saveDir) {
List<UploadedFile> values = uploadedFiles.computeIfAbsent(fieldName, k
-> new ArrayList<>());
if (item.isInMemory()) {
- LOG.debug(() -> "Creating temporary file representing in-memory
uploaded item: " + normalizeSpace(item.getFieldName()));
- try {
- File tempFile = createTemporaryFile(item.getName(),
Path.of(saveDir));
-
- // Track the temporary file for explicit cleanup
- temporaryFiles.add(tempFile);
-
- // Write the in-memory content to the temporary file
- try (java.io.FileOutputStream fos = new
java.io.FileOutputStream(tempFile)) {
- fos.write(item.get());
- }
-
- UploadedFile uploadedFile = StrutsUploadedFile.Builder
- .create(tempFile)
- .withOriginalName(item.getName())
- .withContentType(item.getContentType())
- .withInputName(item.getFieldName())
- .build();
- values.add(uploadedFile);
-
- if (LOG.isDebugEnabled()) {
- LOG.debug("Created temporary file for in-memory uploaded
item: {} at {}",
- normalizeSpace(item.getName()),
tempFile.getAbsolutePath());
- }
- } 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);
- }
- }
+ LOG.debug(() -> "Keeping in-memory uploaded item without writing
to disk: " + normalizeSpace(item.getFieldName()));
+ UploadedFile uploadedFile = StrutsInMemoryUploadedFile.Builder
+ .create(item.get(), Path.of(saveDir))
+ .withOriginalName(item.getName())
+ .withContentType(item.getContentType())
+ .withInputName(item.getFieldName())
+ .build();
+ values.add(uploadedFile);
Review Comment:
In the in-memory branch, `item.get()` is now on the hot path with no local
error-to-`errors` translation (the previous eager-write path explicitly
converted `IOException` into a `LocalizedMessage`). If `item.get()` throws,
parsing may now fail with an exception rather than producing a collected upload
error like other failure modes. If the intent is “only materialization failures
move to consumption-time,” consider catching `IOException` specifically around
`item.get()` and mapping it to the existing upload error mechanism (or update
surrounding error-handling docs to reflect the new behavior).
Issue Time Tracking
-------------------
Worklog Id: (was: 1031662)
Time Spent: 20m (was: 10m)
> Multipart misbehavior with commons-io 2.16.0 and 2.16.1
> -------------------------------------------------------
>
> Key: WW-5413
> URL: https://issues.apache.org/jira/browse/WW-5413
> Project: Struts 2
> Issue Type: Bug
> Components: Core
> Affects Versions: 6.3.0
> Reporter: Riccardo Proserpio
> Assignee: Lukasz Lenart
> Priority: Major
> Fix For: 7.3.0
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> commons-io 2.16.0 has broken the implementation of
> DeferredFileOutputStream changing the behavior of its superclass
> ThresholdingOutputStream: IO-854
>
> The class is used by commons-fileupload DiskFileItem, that is used by Struts
> to handle multipart uploads. The issue causes each multipart part to be read
> as empty.
>
> A fix has been implemented in 2.16.1. However, the fix exposes an issue in
> how the getFile of JakartaMultiPartRequest uses DiskFileItem, that causes it
> to mishandle zero length inputs.
>
> The issue is related to WW-5088 and WW-5146
>
> Moreover, the fix implemented for this issues seems to be dubious and affects
> not only file uploads but every field encoded as multipart/form-data: by
> forcing the diskfileitem threshold to be -1, each and every field was written
> to the filesystem.
>
> The behavior of threadshold -1 was underspecified and inconsistent with the
> commons-io implementation, and has been specified in 2.16.1.
>
> To really fix the issue, I suggest to avoid specifying -1 on the
> DiskFileItemFactory and to properly handle the case when the
> DiskFileItem.isInMemory() returns true in the JakartaMultiPartRequest.getFile
> method: in this case getStoreLocation() is defined to return null and the
> bytes should be read from memory instead.
>
> Avoiding always spilling to disk each and every multipart part should also be
> a performance win, considering that multipart can also be used to transfer
> normal form inputs and not only files.
>
> What do you think?
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
