Details

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

      JBoss 4 / XP / Servlet / both Firefox and IE

      Description

      When parsing a request with the streaming API, some parameters loose their values. I can easily reproduce the problem when a lot of parameters are submitted (about 400 in a table). My code is as follows :

      final FileItemStream itemStream = anItemIterator.next();
      final String fieldName = itemStream.getFieldName();
      System.out.print("Field " + fieldName);
      InputStream stream = itemStream.openStream();
      final String value = Streams.asString(stream, "UTF-8");

      The last statement sometimes returns a value and sometimes not. Sometimes I can retreive all parameters, sometimes I loose 3 or 4 parameters. I reproduced the problem on two computers. I have a very small application with a servlet of 69 lines that reproduces the problem, the project is attached to this issue.

      1. test.fileupload.zip
        227 kB
        Vera Mickael
      2. test.fileupload.zip
        513 kB
        Laurent Goncalves
      3. MultipartStream.java
        34 kB
        Laurent Goncalves
      4. correction_with_test_cases_in_distribution.zip
        17 kB
        Laurent Goncalves

        Issue Links

          Activity

          Hide
          Vera Mickael added a comment -

          This project reproduces the problem on my platform on two computers.

          Just build the war with the build.xml ant file, the war is generated in the 'war' directory. The deploy the war in tomcat or jboss with assertions activated.

          Access webapp with the url http://loclahost:8080/test.fileupload

          Click the submit button at the end of the page, most of times an assertion is thrown and it should not.

          Show
          Vera Mickael added a comment - This project reproduces the problem on my platform on two computers. Just build the war with the build.xml ant file, the war is generated in the 'war' directory. The deploy the war in tomcat or jboss with assertions activated. Access webapp with the url http://loclahost:8080/test.fileupload Click the submit button at the end of the page, most of times an assertion is thrown and it should not.
          Hide
          Vera Mickael added a comment -

          add attachment

          Show
          Vera Mickael added a comment - add attachment
          Hide
          Laurent Goncalves added a comment -

          I think I've found the problem :

          The browser sends the request slowly.

          There is a special case which leads to a bug in the MultipartStream$ItemInputStream.makeAvailable() method :
          The buffer is filled with request's data ending on a part of the boundary. The end of the buffer only contains the begining of the boundary.
          1.The findSeparator method can't see a boundary in the buffer (true because it is a part of the boundary), so it sets the "pos" to "-1"
          2. Because there is no "available" byte in the buffer, the method makeAvailable() is called
          3. The makeAvailable() method copy unread part of the end of the buffer at the begining and call a read on the input Stream.
          here is the problem
          4. Thus the browser sends data slowly, the read() method only returns 19 bytes. 19 in my case, but the bug appear if the read method doesn't returns enough bytes to complete the boundary.
          5. The makeAvailable() method call the findSeparator() method which returns again pos=-1 because there isn't yet a complete boundary in the buffer
          6. The makeAvailable() method call available() method which returns 0 because pos=-1 again
          7. As makeAvailable() returns 0 the read() method returns -1 whereas it isn't the end of the stream !

          To correct the bug, I've changed the makeAvailable() method to force a loop on the reading of the input stream in this case.
          This is the new method :
          /**

          • Attempts to read more data.
          • @return Number of available bytes
          • @throws IOException An I/O error occurred.
            */
            private int makeAvailable() throws IOException {
            if (pos != -1) { return 0; }

          // Move the data to the beginning of the buffer.
          total += tail - head - pad;
          System.arraycopy(buffer, tail - pad, buffer, 0, pad);

          // Refill buffer with new data.
          head = 0;
          // **************** correction : the read is in a do/while structure to loop
          int bytesRead = 0;
          do
          {
          bytesRead = input.read(buffer, pad, bufSize - pad);
          // **************** end of the first part of the correction
          if (bytesRead == -1)

          { // The last pad amount is left in the buffer. // Boundary can't be in there so signal an error // condition. throw new MalformedStreamException( "Stream ended unexpectedly"); }

          notifier.noteBytesRead(bytesRead);
          tail = pad + bytesRead;
          findSeparator();
          // **************** 2nd part : the loop
          //separator was not found, it denotes a miss in the buffer
          // force other buffer's reading until end of stream or first separator found
          }
          while (pos == -1 && bytesRead >= 0);
          // **************** end of correction

          return available();
          }

          Show
          Laurent Goncalves added a comment - I think I've found the problem : The browser sends the request slowly. There is a special case which leads to a bug in the MultipartStream$ItemInputStream.makeAvailable() method : The buffer is filled with request's data ending on a part of the boundary. The end of the buffer only contains the begining of the boundary. 1.The findSeparator method can't see a boundary in the buffer (true because it is a part of the boundary), so it sets the "pos" to "-1" 2. Because there is no "available" byte in the buffer, the method makeAvailable() is called 3. The makeAvailable() method copy unread part of the end of the buffer at the begining and call a read on the input Stream. here is the problem 4. Thus the browser sends data slowly, the read() method only returns 19 bytes. 19 in my case, but the bug appear if the read method doesn't returns enough bytes to complete the boundary. 5. The makeAvailable() method call the findSeparator() method which returns again pos=-1 because there isn't yet a complete boundary in the buffer 6. The makeAvailable() method call available() method which returns 0 because pos=-1 again 7. As makeAvailable() returns 0 the read() method returns -1 whereas it isn't the end of the stream ! To correct the bug, I've changed the makeAvailable() method to force a loop on the reading of the input stream in this case. This is the new method : /** Attempts to read more data. @return Number of available bytes @throws IOException An I/O error occurred. */ private int makeAvailable() throws IOException { if (pos != -1) { return 0; } // Move the data to the beginning of the buffer. total += tail - head - pad; System.arraycopy(buffer, tail - pad, buffer, 0, pad); // Refill buffer with new data. head = 0; // **************** correction : the read is in a do/while structure to loop int bytesRead = 0; do { bytesRead = input.read(buffer, pad, bufSize - pad); // **************** end of the first part of the correction if (bytesRead == -1) { // The last pad amount is left in the buffer. // Boundary can't be in there so signal an error // condition. throw new MalformedStreamException( "Stream ended unexpectedly"); } notifier.noteBytesRead(bytesRead); tail = pad + bytesRead; findSeparator(); // **************** 2nd part : the loop //separator was not found, it denotes a miss in the buffer // force other buffer's reading until end of stream or first separator found } while (pos == -1 && bytesRead >= 0); // **************** end of correction return available(); }
          Hide
          Laurent Goncalves added a comment -

          After more unit testing, I've found another bug with the pad attribute used in the input.read(...) method.
          The pad is updated by the findSeparator() method with a maximum value given by keepRegion attribute.
          The good approach is to use the tail attribute in the input.read(...) method and update it in the loop.
          The final code of the makeAvailable() method must be :

          private int makeAvailable() throws IOException {
          if (pos != -1)

          { return 0; }

          // Move the data to the beginning of the buffer.
          total += tail - head - pad;
          System.arraycopy(buffer, tail - pad, buffer, 0, pad);

          // Refill buffer with new data.
          head = 0;
          tail = pad;
          int bytesRead = 0;
          do
          {
          bytesRead = input.read(buffer, tail, bufSize - tail);
          if (bytesRead != -1)

          { notifier.noteBytesRead(bytesRead); tail += bytesRead; findSeparator(); }

          // separator was not found, it denotes a miss in the buffer
          // force other buffer's reading until end of stream or first separator found
          }
          while (pos == -1 && bytesRead >= 0);

          return available();
          }

          Show
          Laurent Goncalves added a comment - After more unit testing, I've found another bug with the pad attribute used in the input.read(...) method. The pad is updated by the findSeparator() method with a maximum value given by keepRegion attribute. The good approach is to use the tail attribute in the input.read(...) method and update it in the loop. The final code of the makeAvailable() method must be : private int makeAvailable() throws IOException { if (pos != -1) { return 0; } // Move the data to the beginning of the buffer. total += tail - head - pad; System.arraycopy(buffer, tail - pad, buffer, 0, pad); // Refill buffer with new data. head = 0; tail = pad; int bytesRead = 0; do { bytesRead = input.read(buffer, tail, bufSize - tail); if (bytesRead != -1) { notifier.noteBytesRead(bytesRead); tail += bytesRead; findSeparator(); } // separator was not found, it denotes a miss in the buffer // force other buffer's reading until end of stream or first separator found } while (pos == -1 && bytesRead >= 0); return available(); }
          Hide
          Laurent Goncalves added a comment -

          the modified class

          Show
          Laurent Goncalves added a comment - the modified class
          Hide
          Jochen Wiedmann added a comment -

          Laurent, what's even more important than an updated class: Could you supply a unit test, that demonstrates the problem (and, in particular, demonstrates that your updates fix it).

          Show
          Jochen Wiedmann added a comment - Laurent, what's even more important than an updated class: Could you supply a unit test, that demonstrates the problem (and, in particular, demonstrates that your updates fix it).
          Hide
          Laurent Goncalves added a comment -

          Project modified containing unit tests for reproducing the problem.
          Unit tests are in the test/src source folder.
          Unit tests can be started from eclipse environment.
          File upload modified sources are in the src_fileupload folder.
          In this version the MultipartStream is modified, you can restore the old version of the makeAvailable() method, the old method is commented.
          With the new version of MultipartStream, all test case are successfull, if you restore the old version of the makeAvailable() method you will see failures.

          Show
          Laurent Goncalves added a comment - Project modified containing unit tests for reproducing the problem. Unit tests are in the test/src source folder. Unit tests can be started from eclipse environment. File upload modified sources are in the src_fileupload folder. In this version the MultipartStream is modified, you can restore the old version of the makeAvailable() method, the old method is commented. With the new version of MultipartStream, all test case are successfull, if you restore the old version of the makeAvailable() method you will see failures.
          Hide
          Jochen Wiedmann added a comment -

          Laurent, such Unit tests should be included into the commons-fileupload project. They should not be a separate project.

          Show
          Jochen Wiedmann added a comment - Laurent, such Unit tests should be included into the commons-fileupload project. They should not be a separate project.
          Hide
          Laurent Goncalves added a comment -

          "correction_with_test_cases_in_distribution.zip" => Here is the modifications i've done in the distribution.
          Test case is included to reproduce the problem.
          The old makeAvailable() method is commented.
          I've found and corrected a new bug with my previous correction, so makeAvailable() method has changed since my last post.

          Laurent

          Show
          Laurent Goncalves added a comment - "correction_with_test_cases_in_distribution.zip" => Here is the modifications i've done in the distribution. Test case is included to reproduce the problem. The old makeAvailable() method is commented. I've found and corrected a new bug with my previous correction, so makeAvailable() method has changed since my last post. Laurent
          Hide
          Jochen Wiedmann added a comment -

          I applied the src/test part of the correction_with_test_cases_in_distribution.zip, but not the src/java part.

          Yet, the tests are all working, including the testWithASlowInputStream()?

          Show
          Jochen Wiedmann added a comment - I applied the src/test part of the correction_with_test_cases_in_distribution.zip, but not the src/java part. Yet, the tests are all working, including the testWithASlowInputStream()?
          Hide
          Jochen Wiedmann added a comment -

          Laurent, are you aware of the changes in the trunk, related to FILEUPLOAD-135? Is it possible, that these are fixing this issue as well?

          Show
          Jochen Wiedmann added a comment - Laurent, are you aware of the changes in the trunk, related to FILEUPLOAD-135 ? Is it possible, that these are fixing this issue as well?
          Hide
          Kazuhito Ishigaki added a comment -

          I got same problem last week on Tomcat in public service.

          It happens NOT only slow input stream.
          I reproduced it on my local network.
          I watched the size of read bytes, It was 37 or 39 sometimes.
          It is enough to happen the Bug (because IE uses 40byte for boundary).
          The reason why it happens, I think the LimitedInputStream.
          There is no bug in the class, but it makes fragment.

          Show
          Kazuhito Ishigaki added a comment - I got same problem last week on Tomcat in public service. It happens NOT only slow input stream. I reproduced it on my local network. I watched the size of read bytes, It was 37 or 39 sometimes. It is enough to happen the Bug (because IE uses 40byte for boundary). The reason why it happens, I think the LimitedInputStream. There is no bug in the class, but it makes fragment.
          Hide
          Jochen Wiedmann added a comment -

          Is this reproducable? Did you try the latest snapshot? (With the fix for FILEUPLOAD-135 applied?)

          Show
          Jochen Wiedmann added a comment - Is this reproducable? Did you try the latest snapshot? (With the fix for FILEUPLOAD-135 applied?)
          Hide
          Jochen Wiedmann added a comment -

          I'd like to repeat my question, whether your problems are reproducable and possibly fixed in the latest version of the trunk? In other words, whether this bug is in fact a duplicate of FILEUPLOAD-135? If I get no reply, then I intend to close this issue as a duplicate within two weeks or so.

          Show
          Jochen Wiedmann added a comment - I'd like to repeat my question, whether your problems are reproducable and possibly fixed in the latest version of the trunk? In other words, whether this bug is in fact a duplicate of FILEUPLOAD-135 ? If I get no reply, then I intend to close this issue as a duplicate within two weeks or so.
          Hide
          Laurent Goncalves added a comment -

          Jochen, you're right : with the latest version on the trunk, the bug seems to be fixed.

          Show
          Laurent Goncalves added a comment - Jochen, you're right : with the latest version on the trunk, the bug seems to be fixed.
          Hide
          Jochen Wiedmann added a comment -

          Duplicate of FILEUPLOAD-135

          Show
          Jochen Wiedmann added a comment - Duplicate of FILEUPLOAD-135

            People

            • Assignee:
              Jochen Wiedmann
              Reporter:
              Vera Mickael
            • Votes:
              2 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development