Bug 57938 - HttpServletRequest.getParts causes NPE when allowCasualMultipartParsing set "true" and multipart field is empty
Summary: HttpServletRequest.getParts causes NPE when allowCasualMultipartParsing set "...
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 8
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 8.0.22
Hardware: PC All
: P2 major (vote)
Target Milestone: ----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-05-19 20:56 UTC by Onur
Modified: 2015-06-16 20:14 UTC (History)
1 user (show)



Attachments
Sample project to reproduce bug (10.59 KB, application/x-rar)
2015-05-19 20:56 UTC, Onur
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Onur 2015-05-19 20:56:40 UTC
Created attachment 32745 [details]
Sample project to reproduce bug

When sending a HTML form as multipart/form-data, if allowCasualMultipartParsing is set to True and any form field is empty, calling HttpServletRequest.getParts() causes NullPointerException.

19-May-2015 23:30:59.912 SEVERE [http-nio-8512-exec-14] org.apache.catalina.core.StandardWrapperValve.invoke Servlet.service() for servlet [post] in context with path [/test] threw exception
java.lang.NullPointerException
	java.lang.String.<init>(String.java:479)
	org.apache.tomcat.util.http.fileupload.disk.DiskFileItem.getString(DiskFileItem.java:321)
	org.apache.catalina.connector.Request.parseParts(Request.java:2758)
	org.apache.catalina.connector.Request.getParts(Request.java:2641)
	org.apache.catalina.connector.RequestFacade.getParts(RequestFacade.java:1083)
	com.multipart.emptytest.post.doPost(post.java:25) (<--- HttpServletRequest.getParts() here)
	javax.servlet.http.HttpServlet.service(HttpServlet.java:648)
	javax.servlet.http.HttpServlet.service(HttpServlet.java:729)
	org.apache.tomcat.websocket.server.WsFilter.doFilter(WsFilter.java:52)
	...


To Reproduce Bug:

1) Set allowCasualMultipartParsing to "true" in context.xml,
2) Create a HTML form with enctype="multipart/form-data",
3) Submit the form it to a servlet that calls HttpServletRequest.getParts(),
4) If you leave any field empty in the form you will get a NPE.

Expected Results: 

Whether there is an empty field or not HttpServletRequest.getParts() should complete without any exceptions. Empty fields should return empty Part objects.

Actual Results:

Got NPE.

Additional Information: 

Whether allowCasualMultipartParsing is true or false if the servlet is annotated as @MultipartConfig, error disappears and HttpServletRequest.getParts() behaves as expected. But since sole purpose of allowCasualMultipartParsing is the opportunity of using getParts() in a ServletFilter, this is not a solution.

A sample project to reproduce bug in attachment.
Comment 1 Onur 2015-05-19 23:07:39 UTC
Just figured out what the problem is. maxPostSize attribute of Connector in server.xml was set to -1. When allowCasualMultipartParsing is true and if there is no @MultpartConfig, a MultipartConfigElement is created in org.apache.catalina.connector.Request.parseParts using this code:


MultipartConfigElement mce = getWrapper().getMultipartConfigElement();

if (mce == null) {
    if(context.getAllowCasualMultipartParsing()) {
        mce = new MultipartConfigElement(null,
                                         connector.getMaxPostSize(),
                                         connector.getMaxPostSize(),
                                         connector.getMaxPostSize());
    } else {
                ...

The last parameter of MultipartConfigElement above is fileSizeTreshold which is (from javadoc) "the size threshold after which files will be written to disk". 

Although it says that "The maximum size in bytes of the POST which will be handled by the container FORM URL parameter parsing. The limit can be disabled by setting this attribute to a value less than or equal to 0. If not specified, this attribute is set to 2097152 (2 megabytes)." for maxPostSize in documents(https://tomcat.apache.org/tomcat-8.0-doc/config/http.html#Common_Attributes), setting maxPostSize to 0 blocks all post requests for example:

HTTP Status 500 - java.lang.IllegalStateException: org.apache.tomcat.util.http.fileupload.FileUploadBase$SizeLimitExceededException: the request was rejected because its size (235) exceeds the configured maximum (0)

So setting maxPostSize to 0 is not a solution for anyone who wants to allow unlimited size file uploads.


Posible quick fix is:

MultipartConfigElement mce = getWrapper().getMultipartConfigElement();

if (mce == null) {
    if(context.getAllowCasualMultipartParsing()) {
        mce = new MultipartConfigElement(null,
                                         connector.getMaxPostSize(),
                                         connector.getMaxPostSize(),
                                         Math.max(0,connector.getMaxPostSize())); //line 2671 in v8.0.22
    } else {
                ...



I set my maxPostSize to a reasonable value as a temporary fix. But this bug and misleading documentation of maxPostSize should be fixed.
Comment 2 Mark Thomas 2015-06-16 11:14:45 UTC
Thanks for the report. These issues have been fixed in trunk (9.0.x), 8.0.x (for 8.0.24 onwards) and 7.0.x (for 7.0.63 onwards).

I opted for a slightly different fix for the first issue. Negative values are now converted to 0 in the MultipartConfigElement constructor.

maxPostSize=0 was really a bug in how allowCasualMultipartParsing was implemented but I opted to switch the meaning if zero to a limit of zero since a) it is more intuitive and b) it aligns with maxSavePostSize.
Comment 3 Christopher Schultz 2015-06-16 19:57:38 UTC
(In reply to Mark Thomas from comment #2)
> maxPostSize=0 was really a bug in how allowCasualMultipartParsing was
> implemented but I opted to switch the meaning if zero to a limit of zero
> since a) it is more intuitive and b) it aligns with maxSavePostSize.

I'm interested (since I wrote it), what was the "real" bug?
Comment 4 Mark Thomas 2015-06-16 20:14:48 UTC
(In reply to Christopher Schultz from comment #3)
> (In reply to Mark Thomas from comment #2)
> > maxPostSize=0 was really a bug in how allowCasualMultipartParsing was
> > implemented but I opted to switch the meaning if zero to a limit of zero
> > since a) it is more intuitive and b) it aligns with maxSavePostSize.
> 
> I'm interested (since I wrote it), what was the "real" bug?

It didn't handle the 0 == no limit case