Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net/http: multipart ReadForm fails to remove temporary files on io.Copy error #16296

Closed
Egojard opened this issue Jul 8, 2016 · 8 comments
Closed

Comments

@Egojard
Copy link

Egojard commented Jul 8, 2016

I upload a rather large file to a http.Server as a value of a form in a http POST request. The server is a basic http.Server, set with its Server.ReadTimeout and Server.WriteTimeout at one second, i.e. way too short to handle the request. I am trying to get the file by calling
func (r *Request) FormFile(key string)

The function fails with a timeout error, which is perfectly expected, but it leaves in the temporary folder multipart-xxxxxx files. I think I have traced it back to mime\multipart\formdata.go, l 74:

file, err := ioutil.TempFile("", "multipart-")
            if err != nil {
                return nil, err
            }
            defer file.Close()
            _, err = io.Copy(file, io.MultiReader(&b, p))
            if err != nil {
                os.Remove(file.Name())
                return nil, err
            }

We notice that in this case, if io.Copy fails (which is what happens with my short timeouts) we are going to call os.Remove BEFORE calling file.Close, leading to a os.Remove failure. I confirmed this by adding some logs :

file, err := ioutil.TempFile("", "multipart-")
            if err != nil {
                log.Printf("Error at Tempfile: %+v\n", err)
                return nil, err
            }
            defer func() {
                log.Printf("File Closing : %s\n", file.Name())
                if fileCloseErr := file.Close(); fileCloseErr != nil {
                    log.Printf("File close failed : %+v\n", fileCloseErr)
                }
            }()
            _, err = io.Copy(file, io.MultiReader(&b, p))
            if err != nil {
                log.Printf("Error at io.Copy: %v\n", err)
                if osRemoveErr := os.Remove(file.Name()); osRemoveErr != nil {
                    log.Printf("os Remove failed: %+v\n", osRemoveErr)
                }
                return nil, err
            }

and this does confirm the error :

[2016-07-08 12:00:17] <Server> Error at io.Copy: multipart: Part Read: WSARecv tcp 127.0.0.1:8084: i/o timeout
[2016-07-08 12:00:17] <Server> os Remove failed: remove C:\cygwin64\tmp\multipart-183404555: Le processus ne peut pas accéder au fichier car ce fichier est utilisé par un autre processus.
[2016-07-08 12:00:17] <Server> File Closing : C:\cygwin64\tmp\multipart-183404555

First line is the expected timeout error
Second line is the attempt to remove the file (French part on second line being the classic Windows error "The process cannot access the file because it is being used by another process")
Third line is defered function closing the file after we tried to delete it.

  1. What version of Go are you using (go version)?
    go 1.6.2 windows/amd64
  2. What operating system and processor architecture are you using (go env)?
    windows 10 64 bits
    set GOARCH=amd64 set GOBIN= set GOEXE=.exe set GOHOSTARCH=amd64 set GOHOSTOS=windows set GOOS=windows
  3. What did you do?
    Tried to parse a file sent through a HTTP POST rquest with func (r *Request) FormFile(key string) on a http.Server with timeouts too short for the operation to suceed.
  4. What did you expect to see?
    multipart temporary files deleted
  5. What did you see instead?
    multipart files remain in temp folder.
@Egojard
Copy link
Author

Egojard commented Jul 8, 2016

Fix is basic, just manually close the file before os.Remove and move the defer file.Close() (or just call it manually a second time) below this first error management but I'd love to see this fixed for go 1.7. Given the time frame you may fix it faster on your side than if it gets processed as a first-time submission, but if you'd rather have me submit a patch I'll look into the process early next week.

@ianlancetaylor ianlancetaylor changed the title multipart ReadForm fails to remove temporary files on io.Copy error net/http: multipart ReadForm fails to remove temporary files on io.Copy error Jul 8, 2016
@ianlancetaylor ianlancetaylor added this to the Go1.8 milestone Jul 8, 2016
@ianlancetaylor
Copy link
Contributor

I think this is probably too late for 1.7, but @bradfitz can decide.

@bradfitz
Copy link
Contributor

bradfitz commented Jul 8, 2016

It's been like this for the past 7 releases, since Go 1.0 (https://github.com/golang/go/blob/release-branch.go1/src/pkg/mime/multipart/formdata.go#L78) so I think there's no rush doing this in Go 1.7.

It also only affects Windows, and leaving crap in $TMP should be fine in general: the operating system should be cleaning TMP regularly or as needed.

But we can fix it for Go 1.8.

@Egojard
Copy link
Author

Egojard commented Jul 8, 2016

Alright, thanks for the feedback!

@odeke-em
Copy link
Member

odeke-em commented Oct 5, 2016

@Egojard might you be interested in mailing a CL for this fix since the tree is open for Go1.8?

@gopherbot
Copy link

CL https://golang.org/cl/30410 mentions this issue.

@gopherbot
Copy link

CL https://golang.org/cl/33639 mentions this issue.

@gopherbot
Copy link

CL https://golang.org/cl/33640 mentions this issue.

gopherbot pushed a commit that referenced this issue Dec 1, 2016
…copy

Always close the file regardless of whether the copy succeeds or fails.
Pass along the close error if the copy succeeds

Updates #16296
Fixes #17965

Change-Id: Ib394655b91d25750f029f17b3846d985f673fb50
Reviewed-on: https://go-review.googlesource.com/30410
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-on: https://go-review.googlesource.com/33639
Reviewed-by: Chris Broadfoot <cbro@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
gopherbot pushed a commit that referenced this issue Dec 1, 2016
…copy

Always close the file regardless of whether the copy succeeds or fails.
Pass along the close error if the copy succeeds

Updates #16296

Change-Id: Ib394655b91d25750f029f17b3846d985f673fb50
Reviewed-on: https://go-review.googlesource.com/30410
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-on: https://go-review.googlesource.com/33640
Reviewed-by: Chris Broadfoot <cbro@golang.org>
@golang golang locked and limited conversation to collaborators Nov 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants