Struts 2
  1. Struts 2
  2. WW-3802

FileUploadInterceptor makes CGI Parameter temporary files

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.1.2
    • Fix Version/s: 2.3.7
    • Component/s: Core Interceptors
    • Labels:
      None
    • Environment:

      JDK 1.6.0_31, Tomcat 7.0.25

      Description

      I uploaded file with cgi parameters.
      CGI parameter became a temporary file,
      and those files were not removed.

      FileUploadInterceptor makes not only upload file
      but also CGI Parameter temporary files.
      CGI Parameter temporary files are not removed.

      for example ...

      <form action="..." method="post" enctype="multipart/form-data">
      <input type="file" name="uploadFile" />
      <input type="text" name="data1" value="FOO"/>
      <input type="text" name="data2" value="BAR"/>
      </form>

      remained tmp files.

      [upload_47c7ddf7_136bf8a8b3b__8000_00000002.tmp]
      FOO

      [upload_47c7ddf7_136bf8a8b3b__8000_00000003.tmp]
      BAR

        Activity

        Hide
        Lukasz Lenart added a comment -

        Which MultiPartRequest do you use ? JakartaMultiPartRequest either PellMultiPartRequest ?

        Show
        Lukasz Lenart added a comment - Which MultiPartRequest do you use ? JakartaMultiPartRequest either PellMultiPartRequest ?
        Hide
        macha64 added a comment -

        I usually use JakartaMultiPartRequest,
        then CGI parameter tmp files remained.

        I used PellMultiPartRequest.
        good! CGI parameter tmp files were not created!

        [struts.properties]
        struts.multipart.handler=pell

        Show
        macha64 added a comment - I usually use JakartaMultiPartRequest, then CGI parameter tmp files remained. I used PellMultiPartRequest. good! CGI parameter tmp files were not created! [struts.properties] struts.multipart.handler=pell
        Hide
        Lukasz Lenart added a comment -

        Yeah... I think it's a problem in the commons-upload library as the JakartaMultiPartRequest directly passes request to the ServletFileUpload. Could you debug and confirm this ? Thanks a lot!

        Show
        Lukasz Lenart added a comment - Yeah... I think it's a problem in the commons-upload library as the JakartaMultiPartRequest directly passes request to the ServletFileUpload. Could you debug and confirm this ? Thanks a lot!
        Hide
        macha64 added a comment -

        Ok, I will try it. please wait.

        Show
        macha64 added a comment - Ok, I will try it. please wait.
        Hide
        Lukasz Lenart added a comment -

        Any update ?

        Show
        Lukasz Lenart added a comment - Any update ?
        Hide
        macha64 added a comment -

        oh,sorry.I am busy right now...
        I'll debug the problem next week.

        Show
        macha64 added a comment - oh,sorry.I am busy right now... I'll debug the problem next week.
        Hide
        Lukasz Lenart added a comment -

        Thanks a lot!

        Show
        Lukasz Lenart added a comment - Thanks a lot!
        Hide
        jagub zhang added a comment -

        I always use JakartaMultiPartRequest,
        and CGI parameter tmp files were deleted successful.

        In fact, JakartaMultiPartRequest save the every field to file.
        (
        at struts2-core 2.3.1.2 JakartaMultiPartRequest.java, line 157 -> fac.setSizeThreshold(0);
        )

        commons-upload library will delete tmp file -> DiskFileItem#finalize will delete the tmp file.

        Show
        jagub zhang added a comment - I always use JakartaMultiPartRequest, and CGI parameter tmp files were deleted successful. In fact, JakartaMultiPartRequest save the every field to file. ( at struts2-core 2.3.1.2 JakartaMultiPartRequest.java, line 157 -> fac.setSizeThreshold(0); ) commons-upload library will delete tmp file -> DiskFileItem#finalize will delete the tmp file.
        Hide
        jagub zhang added a comment -

        By the way, I find another problem about PellMultiPartRequest.

        if there is two upload field in html,
        and set the two different file with same filename into upload field.

        then, the first file were overwrited by the second file.

        I use pell-multipart-2.1.5.jar

        Show
        jagub zhang added a comment - By the way, I find another problem about PellMultiPartRequest. if there is two upload field in html, and set the two different file with same filename into upload field. then, the first file were overwrited by the second file. I use pell-multipart-2.1.5.jar
        Hide
        macha64 added a comment -

        I confirmed that DiskFileItem#finalize deletes the tmp files.
        but finalize may not be called.
        I think JakartaMultiPartRequest deletes the tmp files explicitly.
        I made a patch for JakartaMultiPartRequest.

        Show
        macha64 added a comment - I confirmed that DiskFileItem#finalize deletes the tmp files. but finalize may not be called. I think JakartaMultiPartRequest deletes the tmp files explicitly. I made a patch for JakartaMultiPartRequest.
        Hide
        jagub zhang added a comment -

        Hi macha64,
        I think the patch will be effective in normal case only.
        if any exception occured in parse(), the tmp files will remained yet.

        Show
        jagub zhang added a comment - Hi macha64, I think the patch will be effective in normal case only. if any exception occured in parse(), the tmp files will remained yet.
        Hide
        Lukasz Lenart added a comment -

        PrepapreOperations cleanUpRequest() method calls Dispatcher cleanUpRequest() method to check if it was an Upload request and cleans up all the files via MultiPartRequestWrapper. I'm thinking about delegating that operation directly to given implementations of MultiPartRequest.

        Show
        Lukasz Lenart added a comment - PrepapreOperations cleanUpRequest() method calls Dispatcher cleanUpRequest() method to check if it was an Upload request and cleans up all the files via MultiPartRequestWrapper. I'm thinking about delegating that operation directly to given implementations of MultiPartRequest.
        Hide
        Lukasz Lenart added a comment -

        I did some refactor, could you check the latest SNAPSHOT (when Hudson comments here) ?

        Show
        Lukasz Lenart added a comment - I did some refactor, could you check the latest SNAPSHOT (when Hudson comments here) ?
        Hide
        Lukasz Lenart added a comment -

        Build is ready, but Hudson did not comment, Infra was doing some JIRA updates all the day, here is the build:

        https://builds.apache.org/job/Struts2/477/

        Show
        Lukasz Lenart added a comment - Build is ready, but Hudson did not comment, Infra was doing some JIRA updates all the day, here is the build: https://builds.apache.org/job/Struts2/477/
        Hide
        jagub zhang added a comment -

        Lenart, I found a interesting thing.
        If use commons-fileupload-1.2.1.jar and commons-io-1.4.jar combination.
        CGI Parameters tmp file will be deleted soon. (just after JakartaMultiPartRequest#parse)

        Show
        jagub zhang added a comment - Lenart, I found a interesting thing. If use commons-fileupload-1.2.1.jar and commons-io-1.4.jar combination. CGI Parameters tmp file will be deleted soon. (just after JakartaMultiPartRequest#parse)
        Hide
        Lukasz Lenart added a comment -

        You mean, you have mismatch with dependencies ? Did you test the latest build ?

        Show
        Lukasz Lenart added a comment - You mean, you have mismatch with dependencies ? Did you test the latest build ?
        Hide
        jagub zhang added a comment - - edited

        I have test the build, and found a mistake about JakartaMultiPartRequest.java.
        You add [ item.delete(); ] at line 124 method processFileField, but the method is parse file field.
        I think the [ item.delete(); ] should add into method processNormalFormField.

        About mismatch with dependencies:
        I just have another old project which is started 2 years ago (struts 2.1.8).
        and found The cgi tmp files will be deleted soon. then I replace commons-fileupload-1.2.1.jar
        , commons-io-1.4.jar into struts 2.3.4 project, it worked successfully.

        PS: only fileupload-1.2.1 and io-1.4 combination can do it, I only want to know why?

        Show
        jagub zhang added a comment - - edited I have test the build, and found a mistake about JakartaMultiPartRequest.java. You add [ item.delete(); ] at line 124 method processFileField, but the method is parse file field. I think the [ item.delete(); ] should add into method processNormalFormField. About mismatch with dependencies: I just have another old project which is started 2 years ago (struts 2.1.8). and found The cgi tmp files will be deleted soon. then I replace commons-fileupload-1.2.1.jar , commons-io-1.4.jar into struts 2.3.4 project, it worked successfully. PS: only fileupload-1.2.1 and io-1.4 combination can do it, I only want to know why?
        Hide
        Lukasz Lenart added a comment -

        Thanks, I'll updated the code, thanks for pointing this out

        Regarding versions of libs take a look here [1], for 2.1.x (and all 2.2.x and 2.3.x) you must use fileupload 1.2.1 and io 1.3.2 (where 1.4 matches as well, though)

        [1] http://struts.apache.org/2.3.3/docs/file-upload.html

        Show
        Lukasz Lenart added a comment - Thanks, I'll updated the code, thanks for pointing this out Regarding versions of libs take a look here [1] , for 2.1.x (and all 2.2.x and 2.3.x) you must use fileupload 1.2.1 and io 1.3.2 (where 1.4 matches as well, though) [1] http://struts.apache.org/2.3.3/docs/file-upload.html
        Hide
        Hudson added a comment -

        Integrated in Struts2 #478 (See https://builds.apache.org/job/Struts2/478/)
        WW-3802 adds missing statement to delete CGI parameters (Revision 1339021)

        Result = SUCCESS
        lukaszlenart :
        Files :

        • /struts/struts2/trunk/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java
        Show
        Hudson added a comment - Integrated in Struts2 #478 (See https://builds.apache.org/job/Struts2/478/ ) WW-3802 adds missing statement to delete CGI parameters (Revision 1339021) Result = SUCCESS lukaszlenart : Files : /struts/struts2/trunk/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java
        Hide
        jagub zhang added a comment -

        About commons-fileupload-1.2.1.jar and commons-io-1.4.jar combination
        Sorry, I make a mistake,
        fileupload-1.2.2 and io-2.0 can also remove tmp files, if wait enough time.

        Show
        jagub zhang added a comment - About commons-fileupload-1.2.1.jar and commons-io-1.4.jar combination Sorry, I make a mistake, fileupload-1.2.2 and io-2.0 can also remove tmp files, if wait enough time.
        Hide
        Lukasz Lenart added a comment -

        The spec says that the file will be deleted when Item will be garbage collected

        Show
        Lukasz Lenart added a comment - The spec says that the file will be deleted when Item will be garbage collected

          People

          • Assignee:
            Lukasz Lenart
            Reporter:
            macha64
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development