Commons FileUpload
  1. Commons FileUpload
  2. FILEUPLOAD-136

FileUpload race condition with used with Jetty 6

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Critical Critical
    • Resolution: Won't Fix
    • Affects Version/s: 1.2
    • Fix Version/s: None
    • Labels:
      None
    • Environment:

      Running on Windows XP SP2 with Jetty 6 embedded and Firefox 2.0.0.4

      Description

      When running commons file upload with Jetty 6, ServletFileUpload.parseRequest spins and never returns when the user clicks the "stop" button in their browser while an upload is in progress.

      Reproduction Steps:

      • Create a simple servlet / html form which accepts a file upload using commons file upload (or use the example code below).
      • Upload a sufficiently large file that you have time to click the stop button before the upload completes.
      • Observe that the thread is now stuck within file upload.

      Other Information:

      Using jstack, I was able to get the following trace of where it is blocking. It looks like it is on a read() call that file upload is making.

      at org/mortbay/jetty/HttpParser$Input.blockForContent(HttpParser.java:922)
      at org/mortbay/jetty/HttpParser$Input.read(HttpParser.java:897)
      at org/apache/commons/fileupload/MultipartStream$ItemInputStream.makeAvailable(MultipartStream.java:959)
      at org/apache/commons/fileupload/MultipartStream$ItemInputStream.close(MultipartStream.java:910)
      at org/apache/commons/fileupload/util/Streams.copy(Streams.java:119)
      at org/apache/commons/fileupload/util/Streams.copy(Streams.java:64)
      at org/apache/commons/fileupload/FileUploadBase.parseRequest(FileUploadBase.java:354)
      at org/apache/commons/fileupload/servlet/ServletFileUpload.parseRequest(ServletFileUpload.java:126)
      at test/Main$1.handle(Main.java:43)
      at org/mortbay/jetty/handler/HandlerWrapper.handle(HandlerWrapper.java:139)
      at org/mortbay/jetty/Server.handle(Server.java:285)
      at org/mortbay/jetty/HttpConnection.handleRequest(HttpConnection.java:502)
      at org/mortbay/jetty/HttpConnection$RequestHandler.content(HttpConnection.java:835)
      at org/mortbay/jetty/HttpParser.parseNext(HttpParser.java:641)
      at org/mortbay/jetty/HttpParser.parseAvailable(HttpParser.java:208)
      at org/mortbay/jetty/HttpConnection.handle(HttpConnection.java:378)
      at org/mortbay/jetty/bio/SocketConnector$Connection.run(SocketConnector.java:226)
      at org/mortbay/thread/BoundedThreadPool$PoolThread.run(BoundedThreadPool.java:442)
      at jrockit/vm/RNI.c2java(IIII)V(Native Method)
      – end of trac

      Originally I thought this was an issue with our code, however, I have since isolated it to a simple test case. Bellow is a class file called Main which when run will instantiate an instance of Jetty on port 8080 and an HTML document that will post a file upload to the servlet. When the stop button is pressed, you will see that the line "Starting processing" is printed, but neither the "Exception occured in processing" or "Processing completed" are printed. I have a full eclipse project (jars and all) on my machine that I was planning on uploading with this ticket, however, I don't see a way to attach a file. Therefore, I have copied and pasted the two files bellow. Let me know if you want the full project.

      === Main.java ===
      /**

      • */
        package test;

      import java.io.IOException;
      import java.util.List;

      import javax.servlet.ServletException;
      import javax.servlet.http.HttpServletRequest;
      import javax.servlet.http.HttpServletResponse;

      import org.apache.commons.fileupload.FileItem;
      import org.apache.commons.fileupload.disk.DiskFileItemFactory;
      import org.apache.commons.fileupload.servlet.ServletFileUpload;
      import org.mortbay.jetty.Handler;
      import org.mortbay.jetty.Server;
      import org.mortbay.jetty.handler.AbstractHandler;

      /**

      • @author Keith Kowalczykowski
      • */
        public class Main {

      public static void main(String[] args) {
      Handler handler = new AbstractHandler() {

      public void handle(String arg0, HttpServletRequest arg1,
      HttpServletResponse arg2, int arg3) throws IOException,
      ServletException
      {
      System.out.println("Starting processing");
      try
      {
      // Create a factory for disk-based file items
      DiskFileItemFactory factory = new DiskFileItemFactory();

      // Create a new file upload handler
      ServletFileUpload upload = new ServletFileUpload(factory);

      // Parse the request
      List items = upload.parseRequest(arg1);

      for (int i = 0; i < items.size(); i++)

      { FileItem file_item = (FileItem) items.get(i); System.out.println("Field Name: " + file_item.getFieldName()); }

      }
      catch (Exception e)

      { e.printStackTrace(); System.out.println("Exception occured in processing"); }

      finally

      { System.out.println("Processing completed"); }

      }

      };

      try

      { Server server = new Server(8080); server.setHandler(handler); server.start(); }

      catch (Exception e)
      {

      }
      }
      }

      === HTML Document ===
      <html>
      <head>
      </head>
      <body>
      <form name="test" action="http://localhost:8080/" method="post" enctype="multipart/form-data">
      <input type="file" name="fileupload"/>
      <input type="submit"/>
      </form>
      </body>
      </html>

      1. TestJetty.java
        2 kB
        Maurice Codik Moscoso
      2. FileUploadTest.zip
        1.48 MB
        Keith Kowalczykowski

        Activity

        Hide
        Keith Kowalczykowski added a comment -

        Here is a full zip of the eclipse project.

        Show
        Keith Kowalczykowski added a comment - Here is a full zip of the eclipse project.
        Hide
        Maurice Codik Moscoso added a comment -

        I'm seeing this issue as well-- threads taking up 100% of CPU spinning in a loop, with jstack showing that same stack trace.

        is there any indication that this is a fileupload bug? its possible that its Jetty's fault... especially with the last few lines in that stack trace being in Jetty code.

        Show
        Maurice Codik Moscoso added a comment - I'm seeing this issue as well-- threads taking up 100% of CPU spinning in a loop, with jstack showing that same stack trace. is there any indication that this is a fileupload bug? its possible that its Jetty's fault... especially with the last few lines in that stack trace being in Jetty code.
        Hide
        Paul Benedict added a comment -

        Could it be related to this? https://issues.apache.org/jira/browse/FILEUPLOAD-140. The ticket describes about the socket blocking on read.

        Show
        Paul Benedict added a comment - Could it be related to this? https://issues.apache.org/jira/browse/FILEUPLOAD-140 . The ticket describes about the socket blocking on read.
        Hide
        Keith Kowalczykowski added a comment -

        >is there any indication that this is a fileupload bug? its possible that its Jetty's fault... especially with the last few lines in that stack trace being in Jetty code.

        Maurice,

        I'm not sure who's fault it is. When I originally posted this issue, I took a look at the file upload code and it is quite complex, so I couldn't really figure out what was going on. They perform a lot of hard for-loops that are supposed to break out if some condition occurs, but obviously it is not occurring in this case. Therefore, I'm not sure whether the file upload code is incorrect, and they were just lucky that it worked correctly under (presumably) tomcat. Or if it truly is a bug in Jetty. I was hoping that someone here could confirm that it is a Jetty bug before passing it on to them, as I have no way to describe the problem to the jetty guys, or proof that it is even their problem.

        Show
        Keith Kowalczykowski added a comment - >is there any indication that this is a fileupload bug? its possible that its Jetty's fault... especially with the last few lines in that stack trace being in Jetty code. Maurice, I'm not sure who's fault it is. When I originally posted this issue, I took a look at the file upload code and it is quite complex, so I couldn't really figure out what was going on. They perform a lot of hard for-loops that are supposed to break out if some condition occurs, but obviously it is not occurring in this case. Therefore, I'm not sure whether the file upload code is incorrect, and they were just lucky that it worked correctly under (presumably) tomcat. Or if it truly is a bug in Jetty. I was hoping that someone here could confirm that it is a Jetty bug before passing it on to them, as I have no way to describe the problem to the jetty guys, or proof that it is even their problem.
        Hide
        Maurice Codik Moscoso added a comment -

        I've tracked this down a little better:

        FileUpload's MultipartStream.Input class attempts to read from the underlying input stream in it's close() method. If close() is being called because the stream threw an exception, then this last close can have unpredictable results.

        In particular, Jetty's inputstream seems to go into an infinite loop if it gets read from after it throws EofException. The attached program reproduces this error condition.

        The output is as following:

        2007-07-17 10:50:48.999::INFO: Logging to STDERR via org.mortbay.log.StdErrLog
        2007-07-17 10:50:49.000::INFO: jetty-6.1.3
        2007-07-17 10:50:49.039::INFO: Started SocketConnector @ 0.0.0.0:10000
        sending data
        input stream is a org.mortbay.jetty.HttpParser$Input
        caught exception: org.mortbay.jetty.EofException
        trying to read again

        Notice that the "last read returned .." line is never printed. At this point, my CPU is pegged to 100% and jstack shows the following stack trace:

        Thread 30265: (state = IN_JAVA)

        • org.mortbay.jetty.HttpParser.parseNext() @bci=423, line=281 (Compiled frame; information may be imprecise)
        • org.mortbay.jetty.HttpParser$Input.blockForContent() @bci=88, line=925 (Compiled frame)
        • org.mortbay.jetty.HttpParser$Input.read(byte[], int, int) @bci=4, line=897 (Interpreted frame)
        • java.io.InputStream.read(byte[]) @bci=5, line=89 (Interpreted frame)
        • TestJetty$1.handle(java.lang.String, javax.servlet.http.HttpServletRequest, javax.servlet.http.HttpServletResponse, int) @bci=132, line=42 (Interpreted frame)
        • org.mortbay.jetty.handler.HandlerWrapper.handle(java.lang.String, javax.servlet.http.HttpServletRequest, javax.servlet.http.HttpServletResponse, int) @bci=23, line=139 (Interpreted frame)
        • org.mortbay.jetty.Server.handle(org.mortbay.jetty.HttpConnection) @bci=110, line=285 (Interpreted frame)
        • org.mortbay.jetty.HttpConnection.handleRequest() @bci=131, line=502 (Interpreted frame)
        • org.mortbay.jetty.HttpConnection$RequestHandler.content(org.mortbay.io.Buffer) @bci=23, line=835 (Interpreted frame)
        • org.mortbay.jetty.HttpParser.parseNext() @bci=2633, line=641 (Interpreted frame)
        • org.mortbay.jetty.HttpParser.parseAvailable() @bci=44, line=208 (Interpreted frame)
        • org.mortbay.jetty.HttpConnection.handle() @bci=122, line=378 (Interpreted frame)
        • org.mortbay.jetty.bio.SocketConnector$Connection.run() @bci=130, line=226 (Interpreted frame)
        • org.mortbay.thread.BoundedThreadPool$PoolThread.run() @bci=45, line=442 (Interpreted frame)

        At this point, I would call this both a file upload bug and a jetty bug. I'll file an issue in Jetty's JIRA, but leaving this open so that the read() during close() thing gets fixed.

        Show
        Maurice Codik Moscoso added a comment - I've tracked this down a little better: FileUpload's MultipartStream.Input class attempts to read from the underlying input stream in it's close() method. If close() is being called because the stream threw an exception, then this last close can have unpredictable results. In particular, Jetty's inputstream seems to go into an infinite loop if it gets read from after it throws EofException. The attached program reproduces this error condition. The output is as following: 2007-07-17 10:50:48.999::INFO: Logging to STDERR via org.mortbay.log.StdErrLog 2007-07-17 10:50:49.000::INFO: jetty-6.1.3 2007-07-17 10:50:49.039::INFO: Started SocketConnector @ 0.0.0.0:10000 sending data input stream is a org.mortbay.jetty.HttpParser$Input caught exception: org.mortbay.jetty.EofException trying to read again Notice that the "last read returned .." line is never printed. At this point, my CPU is pegged to 100% and jstack shows the following stack trace: Thread 30265: (state = IN_JAVA) org.mortbay.jetty.HttpParser.parseNext() @bci=423, line=281 (Compiled frame; information may be imprecise) org.mortbay.jetty.HttpParser$Input.blockForContent() @bci=88, line=925 (Compiled frame) org.mortbay.jetty.HttpParser$Input.read(byte[], int, int) @bci=4, line=897 (Interpreted frame) java.io.InputStream.read(byte[]) @bci=5, line=89 (Interpreted frame) TestJetty$1.handle(java.lang.String, javax.servlet.http.HttpServletRequest, javax.servlet.http.HttpServletResponse, int) @bci=132, line=42 (Interpreted frame) org.mortbay.jetty.handler.HandlerWrapper.handle(java.lang.String, javax.servlet.http.HttpServletRequest, javax.servlet.http.HttpServletResponse, int) @bci=23, line=139 (Interpreted frame) org.mortbay.jetty.Server.handle(org.mortbay.jetty.HttpConnection) @bci=110, line=285 (Interpreted frame) org.mortbay.jetty.HttpConnection.handleRequest() @bci=131, line=502 (Interpreted frame) org.mortbay.jetty.HttpConnection$RequestHandler.content(org.mortbay.io.Buffer) @bci=23, line=835 (Interpreted frame) org.mortbay.jetty.HttpParser.parseNext() @bci=2633, line=641 (Interpreted frame) org.mortbay.jetty.HttpParser.parseAvailable() @bci=44, line=208 (Interpreted frame) org.mortbay.jetty.HttpConnection.handle() @bci=122, line=378 (Interpreted frame) org.mortbay.jetty.bio.SocketConnector$Connection.run() @bci=130, line=226 (Interpreted frame) org.mortbay.thread.BoundedThreadPool$PoolThread.run() @bci=45, line=442 (Interpreted frame) At this point, I would call this both a file upload bug and a jetty bug. I'll file an issue in Jetty's JIRA, but leaving this open so that the read() during close() thing gets fixed.
        Show
        Maurice Codik Moscoso added a comment - filed http://jira.codehaus.org/browse/JETTY-393
        Hide
        Jochen Wiedmann added a comment -

        I do not think, that it's the task of the InputStream user to track whether an InputStream is still usable. In other words, if your diagnosis is right, then I wouldn't consider this a FileUpload bug.

        Show
        Jochen Wiedmann added a comment - I do not think, that it's the task of the InputStream user to track whether an InputStream is still usable. In other words, if your diagnosis is right, then I wouldn't consider this a FileUpload bug.
        Hide
        Maurice Codik Moscoso added a comment -

        I do agree in general, but it seems odd that MultipartStream is trying to read from the underlying stream during it's close() operation.

        Is there a reason why that method doesnt simply delegate to the inner stream's close()? Just glancing at the code now, it seems that the underlying stream is never even closed.

        Show
        Maurice Codik Moscoso added a comment - I do agree in general, but it seems odd that MultipartStream is trying to read from the underlying stream during it's close() operation. Is there a reason why that method doesnt simply delegate to the inner stream's close()? Just glancing at the code now, it seems that the underlying stream is never even closed.
        Hide
        Jochen Wiedmann added a comment -

        I assume, you are referring to the ItemInputStream, are you?

        The ItemInputStream is returned to the user, if he or she iterates over the multipart streams parts. The user may close this InputStream, but there's no requirement that he must close it. Even more, the user is not required to consume the InputStream's contents. For example, the user might read the first line, detects that he is not interested in this item and calls FileItemIterator.next().

        At that point, we are in the middle of a part's body. In order to skip this body, we need to find the next boundary, which is exactly what ItemInputStream.close() does for us.

        Show
        Jochen Wiedmann added a comment - I assume, you are referring to the ItemInputStream, are you? The ItemInputStream is returned to the user, if he or she iterates over the multipart streams parts. The user may close this InputStream, but there's no requirement that he must close it. Even more, the user is not required to consume the InputStream's contents. For example, the user might read the first line, detects that he is not interested in this item and calls FileItemIterator.next(). At that point, we are in the middle of a part's body. In order to skip this body, we need to find the next boundary, which is exactly what ItemInputStream.close() does for us.
        Hide
        Maurice Codik Moscoso added a comment -

        Ah, I see. That does make sense, but I would argue that its likely safer to do that "scan to the next item" when you call FileItemIterator.next(), not when you attempt to close an item's stream.

        Since a programmer is probably using close() inside a finally block, its possible to run into situations like the above: an exception is thrown, you say "close", and then you actually read some more in the background, which can cause unpredictable results. However, doing it when trying to iterate to the next item completely avoid this issue, since a developer is unlikely to be doing this after an exception is thrown.

        Just my 2c... I do agree that its certainly much more of a problem with jetty – code should never be able to go into wacky busy loops.

        Show
        Maurice Codik Moscoso added a comment - Ah, I see. That does make sense, but I would argue that its likely safer to do that "scan to the next item" when you call FileItemIterator.next(), not when you attempt to close an item's stream. Since a programmer is probably using close() inside a finally block, its possible to run into situations like the above: an exception is thrown, you say "close", and then you actually read some more in the background, which can cause unpredictable results. However, doing it when trying to iterate to the next item completely avoid this issue, since a developer is unlikely to be doing this after an exception is thrown. Just my 2c... I do agree that its certainly much more of a problem with jetty – code should never be able to go into wacky busy loops.
        Hide
        Jochen Wiedmann added a comment -

        I can follow your arguments, but I do not intend to change the code for that reason.

        Show
        Jochen Wiedmann added a comment - I can follow your arguments, but I do not intend to change the code for that reason.

          People

          • Assignee:
            Jochen Wiedmann
            Reporter:
            Keith Kowalczykowski
          • Votes:
            1 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development