Bug 63206 - Try to create parent directories before writing an uploaded file to disk
Summary: Try to create parent directories before writing an uploaded file to disk
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 8
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 8.5.38
Hardware: PC All
: P2 normal (vote)
Target Milestone: ----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-02-25 12:37 UTC by Andy Wilkinson
Modified: 2019-03-05 14:02 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Wilkinson 2019-02-25 12:37:09 UTC
When processing a multi-part upload and parsing the parts, a check is made that the output location is a directory. If it is not, an exception is thrown. By contrast, Jetty will try to create the directory [1] before writing the uploaded file into it.

I wonder if Tomcat could be modified to behave in the same way as Jetty? In addition to helping the user out when they have configured the output location before have forgotten to create it, it will also help in situations where files are being uploaded to a location within /tmp or similar and it has been deleted by tmpwatch due to a period of inactivity.

[1] https://github.com/eclipse/jetty.project/blob/34b2678e6d2b164088dcec0ef9e66f1148a8a527/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormInputStream.java#L522-L523
Comment 1 Christopher Schultz 2019-02-25 14:13:19 UTC
I'm a -1 on this unless it is a configurable option and disabled by default. Tomcat shouldn't be creating directories unless explicitly directed to do so.
Comment 2 Remy Maucherat 2019-02-25 14:48:16 UTC
(In reply to Christopher Schultz from comment #1)
> I'm a -1 on this unless it is a configurable option and disabled by default.
> Tomcat shouldn't be creating directories unless explicitly directed to do so.

+1 for that.
Comment 3 Andy Wilkinson 2019-02-25 15:03:04 UTC
> Tomcat shouldn't be creating directories unless explicitly directed to do so.

That seems a little arbitrary to apply that rule in this case. Tomcat already creates directories in several places. A few examples from a non-exhaustive search of the codebase:

 - JspCompilationContext will create the output directory if needed during JSP compilation [1]
 - Tomcat will create the base directory during startup if needed [2]
 - StandardContext will create its work directory if needed [3]

As far as I know, and as far as I can tell from looking at the code, in each of these cases it's the default behaviour to create the directory. I also think it's of benefit to Tomcat's usability that it does so. I can't see why the location used for multipart uploads should be treated any differently.

[1] https://github.com/apache/tomcat/blob/c4b006e3dd46ca8604b2c9418a4714b6261aab7c/java/org/apache/jasper/JspCompilationContext.java#L674
[2] https://github.com/apache/tomcat/blob/d9c4f3fcd622c04db8598aa25f0ab5781ea2421f/java/org/apache/catalina/startup/Tomcat.java#L813
[3] https://github.com/apache/tomcat/blob/b4ad8fa8e03b870ee320ac3239059abc08fe1f58/java/org/apache/catalina/core/StandardContext.java#L6053
Comment 4 Konstantin Kolinko 2019-02-25 15:13:20 UTC
I think the OP means the following:
If ${catalina.base}/temp directory does not exist, file upload fails.

Unlike ${catalina.base}/work, the "temp" directory is not created automatically at start time.

Tomcat release packages have a "safeToDelete.tmp" file in the temp directory to make sure that is is created, as either zip or tar file format does not allow empty directories.


> a configurable option

Implement as a Listener, configurable in server.xml?
Comment 5 Christopher Schultz 2019-02-25 15:39:13 UTC
(In reply to Andy Wilkinson from comment #3)
> > Tomcat shouldn't be creating directories unless explicitly directed to do so.
> 
> That seems a little arbitrary to apply that rule in this case.

I disagree.

> Tomcat already creates directories in several places. A few examples from a
> non-exhaustive search of the codebase:
> 
>  - JspCompilationContext will create the output directory if needed during
> JSP compilation [1]
>  - Tomcat will create the base directory during startup if needed [2]
>  - StandardContext will create its work directory if needed [3]

Those are all directories that Tomcat manages by itself. If the application, via <multipart-config> or @MultipartConfig, sets an output directory, then Tomcat should not be creating that on its own without a specific request to do so.

I don't find the distinction arbitrary at all.

> As far as I know, and as far as I can tell from looking at the code, in each
> of these cases it's the default behaviour to create the directory. I also
> think it's of benefit to Tomcat's usability that it does so. I can't see why
> the location used for multipart uploads should be treated any differently.

Because it can be set by the application and is not configured and/or managed by Tomcat.
Comment 6 Christopher Schultz 2019-02-25 15:43:15 UTC
(In reply to Konstantin Kolinko from comment #4)
> I think the OP means the following:
> If ${catalina.base}/temp directory does not exist, file upload fails.
> 
> Unlike ${catalina.base}/work, the "temp" directory is not created
> automatically at start time.

Auto-creation of the ${catalina.base}/temp directory would be okay with me, but not the creation of arbitrary "external" directories. IMO, each application should have its own segregated "uploads" directory, so maybe ${catalina.base}/work/[engine]/[host]/[app]/temp.

As of now, it would interfere with the package namespace for JSPs, so compiled JSP files ought to go into a "jsp" subdirectory. But that's a fairly major change.

> Implement as a Listener, configurable in server.xml?

Easy enough to do, but this is really an engine-specific option. Or maybe even host-specific.
Comment 7 Andy Wilkinson 2019-02-25 15:53:01 UTC
> Because it can be set by the application and is not configured and/or managed by Tomcat.

That was a distinction that you did not make when you stated the following:

> Tomcat shouldn't be creating directories unless explicitly directed to do so.

Even with that new distinction, I'm not sure that I agree. Why should a usability benefit that is applied to Tomcat itself not also applied to an application that is deployed to Tomcat? Put another way, what harm could be caused by creating the directory?

It's also possible that the application does not customise the location via <multipart-config> or @MultipartConfig. In that case the location is both configured and managed by Tomcat yet the directory will not be created. That seems unnecessarily inconsistent to me.
Comment 8 Mark Thomas 2019-02-25 16:51:35 UTC
There may be circumstances (e.g. when running under a security manager or due to file system permissions) where it will not be possible to create the directory even if the container tries to do so.

This feels like the sort of thing that an application developer should have to take responsibility for so that they give some thought to issues such as:
- where the files are uploaded to
- how much space is available
- file permissions for this location


Configuration can be per context as the relevant code already has access to the Context object.

My preference is for disabled by default with an option to enable. Also, at least an INFO message when the directory is created. Maybe WARN. Certainly not ERROR.
Comment 9 Mark Thomas 2019-03-05 13:22:50 UTC
Fixed in:
- trunk for 9.0.17 onwards
- 8.5.x for 8.5.39 onwards
- 7.0.x for 7.0.94 onwards

New attribute on Context: createUploadTargets
Defaults to false
Comment 10 Remy Maucherat 2019-03-05 14:02:13 UTC
(In reply to Andy Wilkinson from comment #3)
> > Tomcat shouldn't be creating directories unless explicitly directed to do so.
> 
> That seems a little arbitrary to apply that rule in this case. Tomcat
> already creates directories in several places. A few examples from a
> non-exhaustive search of the codebase:
> 
>  - JspCompilationContext will create the output directory if needed during
> JSP compilation [1]
>  - Tomcat will create the base directory during startup if needed [2]
>  - StandardContext will create its work directory if needed [3]

The examples are misleading:
- Per webapp folders will always be automatically created (obviously), this is part of the webapp deployment process
- For the last one (the main folder), this is probably a relic of old days and we might not do it right now as it's not actually very useful