NullPointerException when getting parts from HttpServletRequest

[filiphr]

Jetty version(s) 12.0.5

Jetty Environment

ee10

Java version/vendor (use: java -version)

Picked up JAVA_TOOL_OPTIONS: -Duser.language=en -Duser.country=US openjdk version "17.0.10" 2024-01-16 OpenJDK Runtime Environment Temurin-17.0.10+7 (build 17.0.10+7) OpenJDK 64-Bit Server VM Temurin-17.0.10+7 (build 17.0.10+7, mixed mode)

OS type/version MacOS M3

Description

There is an NPE when HttpServletRequest#getParts is used for a manually configured Jetty server. The stacktrace is:

jakarta.servlet.ServletException: org.eclipse.jetty.http.BadMessageException: 400: bad multipart
    at org.eclipse.jetty.ee10.servlet.ServletApiRequest.getParts(ServletApiRequest.java:591)
    at com.flowable.content.rest.util.JettyTest$TestServlet.doPost(JettyTest.java:68)
    at jakarta.servlet.http.HttpServlet.service(HttpServlet.java:547)
    at jakarta.servlet.http.HttpServlet.service(HttpServlet.java:614)
    at org.eclipse.jetty.ee10.servlet.ServletHolder.handle(ServletHolder.java:736)
    at org.eclipse.jetty.ee10.servlet.ServletHandler$ChainEnd.doFilter(ServletHandler.java:1614)
    at org.eclipse.jetty.ee10.servlet.ServletHandler$MappedServlet.handle(ServletHandler.java:1547)
    at org.eclipse.jetty.ee10.servlet.ServletChannel.dispatch(ServletChannel.java:797)
    at org.eclipse.jetty.ee10.servlet.ServletChannel.handle(ServletChannel.java:428)
    at org.eclipse.jetty.ee10.servlet.ServletHandler.handle(ServletHandler.java:464)
    at org.eclipse.jetty.server.handler.ContextHandler.handle(ContextHandler.java:761)
    at org.eclipse.jetty.server.Server.handle(Server.java:179)
    at org.eclipse.jetty.server.internal.HttpChannelState$HandlerInvoker.run(HttpChannelState.java:594)
    at org.eclipse.jetty.server.internal.HttpConnection.onFillable(HttpConnection.java:424)
    at org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:322)
    at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:99)
    at org.eclipse.jetty.io.SelectableChannelEndPoint$1.run(SelectableChannelEndPoint.java:53)
    at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:971)
    at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.doRunJob(QueuedThreadPool.java:1201)
    at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run(QueuedThreadPool.java:1156)
    at java.base/java.lang.Thread.run(Thread.java:840)
Caused by: org.eclipse.jetty.http.BadMessageException: 400: bad multipart
    ... 21 more
Caused by: java.lang.NullPointerException: Cannot invoke "java.io.File.toPath()" because "filesDirectory" is null
    at org.eclipse.jetty.ee10.servlet.ServletMultiPartFormData.lambda$from$0(ServletMultiPartFormData.java:128)
    at org.eclipse.jetty.http.MultiPartFormData.from(MultiPartFormData.java:86)
    at org.eclipse.jetty.ee10.servlet.ServletMultiPartFormData.from(ServletMultiPartFormData.java:104)
    at org.eclipse.jetty.ee10.servlet.ServletMultiPartFormData.from(ServletMultiPartFormData.java:60)
    at org.eclipse.jetty.ee10.servlet.ServletApiRequest.getParts(ServletApiRequest.java:508)
    ... 20 more

Is there a reason why the ContextHandler is not using getServer().getContext().getTempDirectory()? It is in

https://github.com/jetty/jetty.project/blob/a683690678fd0323918bb5079341d10ad8979c89/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java#L1125-L1131

If the ServerContext is used there then null will not be returned since it does:

https://github.com/jetty/jetty.project/blob/a683690678fd0323918bb5079341d10ad8979c89/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java#L948-L951

and tmpDir is never null:

https://github.com/jetty/jetty.project/blob/a683690678fd0323918bb5079341d10ad8979c89/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java#L861

How to reproduce?

Run the following main class:

import java.io.IOException;
import java.net.URI;
import java.net.http.HttpClient;
import java.net.http.HttpRequest;
import java.net.http.HttpResponse;
import java.util.Collection;

import jakarta.servlet.MultipartConfigElement;
import jakarta.servlet.ServletContextEvent;
import jakarta.servlet.ServletContextListener;
import jakarta.servlet.ServletException;
import jakarta.servlet.ServletRegistration;
import jakarta.servlet.http.HttpServlet;
import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;
import jakarta.servlet.http.Part;

import org.eclipse.jetty.ee10.servlet.ServletContextHandler;
import org.eclipse.jetty.server.Server;
import org.eclipse.jetty.server.ServerConnector;

public class JettyTest {

    public static void main(String[] args) throws IOException, InterruptedException {
        Server server = new Server();
        //server.setTempDirectory(new File(System.getProperty("java.io.tmpdir")));
        ServerConnector connector = new ServerConnector(server);
        connector.setPort(0);
        server.addConnector(connector);
        ServletContextHandler contextHandler = new ServletContextHandler("/test");
        server.setHandler(contextHandler);
        contextHandler.addEventListener(new TestServletListener());

        try {
            server.start();
        } catch (Exception e) {
            throw new RuntimeException("Failed ot start server", e);
        }

        HttpRequest request = HttpRequest.newBuilder(URI.create("http://localhost:" + connector.getLocalPort() + "/test/service/form-data"))
                .header("Content-Type", "multipart/form-data; boundary=boundaryId")
                .POST(HttpRequest.BodyPublishers.ofString("--boundaryId\n"
                        + "Content-Disposition: form-data; name=\"file1\"; filename=\"a.txt\"\n"
                        + "Content-Type: text/plain; charset=ISO-8859-1\n"
                        + "Content-Transfer-Encoding: binary\n"
                        + "\n"
                        + "Content of a.txt\n"
                        + "--boundaryId--\n"))
                .build();
        HttpResponse<String> response = HttpClient.newHttpClient()
                .send(request, HttpResponse.BodyHandlers.ofString());

        System.out.println("Received status code: " + response.statusCode());
        System.out.println("Received response: " + response.body());
    }

    protected static class TestServlet extends HttpServlet {

        @Override
        protected void doPost(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
            Collection<Part> parts = request.getParts();
            response.setStatus(HttpServletResponse.SC_OK);
            response.getOutputStream().print("Received " + parts.size() + " parts");
        }
    }

    protected static class TestServletListener implements ServletContextListener {

        @Override
        public void contextInitialized(ServletContextEvent sce) {
            ServletRegistration.Dynamic registration = sce.getServletContext().addServlet("Test Servlet", new TestServlet());
            registration.addMapping("/service/*");
            registration.setMultipartConfig(new MultipartConfigElement((String) null));
            registration.setLoadOnStartup(1);
            registration.setAsyncSupported(true);
        }

    }

}

[joakime]

The ServletContextHandler has it's own temp directory. That follows the servlet spec rules, and does not use any Server level behavior, if it's undeclared by the ServletContext, then it is null. See ServletContextHandler.setTempDirectory(File).

Note that each ServletContext needs it's own temp directory per spec. So using the Server level temp directory isn't a good fit for this spec behavior.

Also note, that when you use new MultipartConfigElement((String)null) that has it's own location (unrelated to the ServletContext temp directory) and is forced to be "" if you pass null (like you are doing). Which is also a undefined location. See https://github.com/jakartaee/servlet/blob/6.0.0-RELEASE/api/src/main/java/jakarta/servlet/MultipartConfigElement.java#L39-L44

[filiphr]

@joakime sorry, I am not following, am I configuring something incorrectly or the notes were general notes?

Also note, that when you use new MultipartConfigElement((String)null) that has it's own location (unrelated to the ServletContext temp directory) and is forced to be "" if you pass null (like you are doing).

We are doing this on purpose. We want to have multi part config and we want to use the defaults from the Server. Actually, according to the spec https://jakarta.ee/specifications/servlet/6.0/jakarta-servlet-spec-6.0#_MultipartConfig it says

The location attribute of the jakarta.servlet.annotation.MultipartConfig and the <location> element of the <multipart-config> is interpreted as an absolute path and defaults to the value of the jakarta.servlet.context.tempdir. If a relative path is specified, it will be relative to the tempdir location. The test for absolute path vs relative path MUST be done via java.io.File.isAbsolute.

Do I need to configure the temp directory of the servlet? This used to work without issues with Jetty 11.

[joakime]

The jakarta.servlet.context.tempdir attribute is configured by calling ServletContextHandler.setTempDirectory(File). Which calls the ServletContextHandler.setAttribute(ServletContext.TEMPDIR, new File("/absolute/path/to/the/context/specific/temp/directory"))

https://github.com/jetty/jetty.project/blob/a683690678fd0323918bb5079341d10ad8979c89/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java#L294-L298

Without calling that method, you have no temp directory for that ServletContext, and no attribute jakarta.servlet.context.tempdir. This is what happens with the ServletContextHandler.

Take for example when you use a WebAppContext (and a full blown WAR or exploded webapp directory), the startup / initialization of the WEB-INF behaviors sets up the temp directory for that WebAppContext based on spec behaviors (this is done in the WebInfConfiguration).

https://github.com/jetty/jetty.project/blob/a683690678fd0323918bb5079341d10ad8979c89/jetty-ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/WebInfConfiguration.java#L130-L183

That is the behavior for ee10 environment (the behavior is slightly different for ee9/ee8, as it has to go through a ee9/ee8 nesting layer that doesn't exist for ee10)

Since you are using ServletContextHandler you have no WEB-INF behaviors, no jakarta.servlet.context.tempdir, etc. It is up to you to specify the temp directory by calling ServletContextHandler.setTempDirectory(File).

Note: each WebAppContext (and ServletContextHandler) has its own unique temp directory, it should not share the same directory (like what would exist at the Server level).

Here's another example, you have no default descriptor behaviors with ServletContextHandler as well ... In a new, unconfigured ServletContextHandler, you have no servlets, no filters, no listeners, no servlet attributes, no session support, no security support, barebones mimetypes, etc. If you create a WAR file with just an empty web.xml and deploy it, you'll have a DefaultServlet, a JettyJspServlet, session configurations, session configuration, security configuration, some constraints, a bunch of servlet attributes, mime-types declared, error-page declarations, locale declarations, etc. Basically what is declared in here -> https://github.com/jetty/jetty.project/blob/jetty-12.0.x/jetty-ee10/jetty-ee10-webapp/src/main/config/etc/webdefault-ee10.xml

Using ServletContextHandler puts all of this control in your hands, no magic, no defaults, no initialization, no bytecode scanning, no annotations, its up to you to specify the behaviors you want to use.

[joakime]

A proposal PR #11397 from @lorban has been submitted to attempt to address the contract issue discovered with Server.getContext().getTempDirectory(). Fixing that contract issues causes a new issue. As each ServletContextHandler to have it's own temp directory, that PR now makes the ServletContextHandler temp directory invalid for cases with multiple ServletContextHandler instances. Note: WebAppContext and its WebInfConfiguration is already doing the right thing.

[gregw]

Do I need to configure the temp directory of the servlet? This used to work without issues with Jetty 11

Did you use ServletContextHandler or WebAppContext in Jetty 11? The later does more default behaviours.

[filiphr]

Did you use ServletContextHandler or WebAppContext in Jetty 11?

We used the ServletContextHandler in Jetty 11. The only thing we did during the upgrade was to change the package name. The example I shared was the minimal reproducible example I could create.

The code we are using with Jetty 11 is

https://github.com/flowable/flowable-engine/blob/7ff37bcad780f1dd79c7dc752d5a468cb21d8559/modules/flowable-rest/src/test/java/org/flowable/rest/util/TestServerUtil.java#L43-L63

[joakime]

@filiphr another point of difference between Jetty 11 and Jetty 12 is the changes in the servlet spec with regards to the jakarta.servlet.MultipartConfigElement

This section of javadoc was added in Servlet 6.0. https://github.com/jakartaee/servlet/blob/6.0.0-RELEASE/api/src/main/java/jakarta/servlet/MultipartConfigElement.java#L109-L110

See:

• https://github.com/jakartaee/servlet/issues/111
• https://github.com/jakartaee/servlet/commit/72d509f5c6c4d9120e55e1978a229c71f3a16303

The default in the Servlet 5 spec (what is used in Jetty 11) if unconfigured behavior in Jetty 11 was to not use the File system temp location for multipart. The default in Servlet 6 spec means that a File system temp location is now required if you don't configure the MultipartConfigElement.fileSizeThreshold to -1 (to prevent the file temp from being required)

[filiphr]

Thanks for pointing that out @joakime. That's a good thing to know and for our tests we can perhaps use that.

I tried it out. However, if I do not set a temp dir it still leads to the same NPE.

[janbartel]

@lorban @joakime @filiphr interesting that you find a difference in behaviour between jetty-11 and jetty-12, which is something we need to resolve.

Your code worked in previous versions because the multipart parsing checked if there was a javax.servlet.context.tempdir context attribute set but if not (which is the default case) defaulted to using java.io.tmpdir instead. Each request remembered its temp files, and deleted them when the request exited, so there was no cross-contamination between contexts.

In jetty-12 multipart handling we have changed to using a call to ServletContextHandler.Context.getTempDirectory() and expecting it to never return null. From your stacktrace, it seems that it is indeed returning null. We just need to ensure that it falls back to 'java.io.tmpdir' to fix this problem.