Navigation Menu

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

mime/multipart: TempFile file hangs around on disk after usage in multipart/formdata.go #20253

Closed
nilslice opened this issue May 5, 2017 · 20 comments
Assignees
Milestone

Comments

@nilslice
Copy link

nilslice commented May 5, 2017

Hey Go team,

I'd like to know if this is a bug or expected behavior. If it is a bug, I would like to write a patch to fix it.


Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go version go1.8 linux/amd64

What operating system and processor architecture are you using (go env)?

GOOS="linux"
GOARCH="amd64"

What did you do?

When uploading a file to a Go HTTP server, the call to req.ParseMultipartForm() subsequently calls readForm() and if the file size exceeds the maxMemory setting it creates a file on disk using the ioutil.TempFile() func, but the resulting file is never removed by the Go program. Unix behavior aside, based on the documentation for ioutil.TempFile(), the caller should remove the file once it is done using it. The file is only ever removed from the Go program if the io.Copy returns a non-nil error.

If possible, provide a recipe for reproducing the error.
A complete runnable program is good.

I would need to show a lot of code to demonstrate this, but you can see in the Go src here how the file is never removed: https://github.com/golang/go/blob/master/src/mime/multipart/formdata.go#L78

A link on play.golang.org is best.

What did you expect to see?

I expect that the temp file is removed after the file is copied to it's designated location on disk.

What did you see instead?

my /tmp directory is littered with multipart-XXXXXXXX files

@bradfitz
Copy link
Contributor

bradfitz commented May 5, 2017

When you're done with the multipart files, you can call:
https://golang.org/pkg/mime/multipart/#Form.RemoveAll

... which is accessible here:
https://golang.org/pkg/net/http/#Request.MultipartForm

But, yeah, maybe this could be more automatic. Maybe the multipart.File's concrete type could delete-on-Close.

Feel free to work on it!

@bradfitz bradfitz changed the title ioutil.TempFile file hangs around on disk after usage in multipart/formdata.go mime/multipart: TempFile file hangs around on disk after usage in multipart/formdata.go May 5, 2017
@bradfitz bradfitz added this to the Go1.10 milestone May 5, 2017
@nilslice
Copy link
Author

nilslice commented May 5, 2017

Thanks for the suggestions! I wasn't aware of Form.RemoveAll - good to know.

From my perspective, the complete process of dealing with HTML forms / other HTTP clients uploading files to my server shouldn't involve removing the temp files w/ RemoveAll. Based on the UX of a typical Go programmer, automatically cleaning up those files doesn't feel overly magical to me.

I'll work on it and submit if something clicks.

@kennygrant
Copy link
Contributor

kennygrant commented Nov 3, 2017

@nilslice still working on this?

We were hit by a similar problem (have small servers accepting file uploads, and tmp files filled the disk at times if not flushed regularly), and I too was unaware of Form.RemoveAll, but delete on close would be even better.

Happy to look at it if you're busy or leave it to you if you'd like to look at it.

@nilslice
Copy link
Author

nilslice commented Nov 3, 2017

@kennygrant - I have a partial fix, but have been swamped with other work. If it's imperative to your project that it's patched, go for it! Otherwise, I'd still like to finish it. But no problem whatsoever if you want to get the patch in.

@kennygrant
Copy link
Contributor

That's ok, I'll leave it to you if you have started. We have a workaround for now.

@rsc rsc modified the milestones: Go1.10, Go1.11 Nov 22, 2017
@Kagami
Copy link

Kagami commented Mar 17, 2018

@nilslice
Copy link
Author

@Kagami - it does appear to make the call, but the last time I tested it the files were still on disk.

The blame shows that the code has been in the tree for ~7 years, well before this issue was originally filed: https://github.com/golang/go/blame/678dede7bc22a7cccfe78c42711478a9b1ecf4d3/src/net/http/server.go#L1553-L1555

Have you tried this to verify that the files are cleaned up after the request?

I'm trying to get my CL out this month or early next (taking a couple days off from work to do long put-off open source tasks I have queued up!).

@andybons andybons added the NeedsFix The path to resolution is known, but the work has not been done. label Mar 19, 2018
@gopherbot
Copy link

Change https://golang.org/cl/113055 mentions this issue: mime/multipart: properly remove temp files in case of error

@lpar
Copy link

lpar commented Aug 14, 2018

If this won't be fully fixed in 1.10, how about mentioning the need to call RemoveAll in the documentation for ParseMultipartForm?

@dvdscripter
Copy link

I'm no expert but you should be able to remove files without closing them?

...
file, _, err := r.FormFile("file")
if err != nil {
	return err
}
defer file.Close()
...

Leaves no "multipart*" files behind.

@agnivade
Copy link
Contributor

@nilslice - Did you get a chance to work on this ? As per @Kagami's comment, it seems like the file is indeed removed.

Please also try with the 1.12beta1 to see if there has been any changes.

@nilslice
Copy link
Author

nilslice commented Dec 19, 2018

@agnivade - I have not recently had time to wrap this up. If anyone would like to take it, or if it is indeed no longer an issue, that's awesome 😄

@chris3torek
Copy link

Note that this is still a problem with code generated by swagger. It's hard to work around in all cases. Success cases are easy enough, as your swagger parameters include the original HTTP request so you can do:

form := params.HTTPRequest.MultipartForm
err = form.RemoveAll()
// handle err, if you like
form.File = nil // in case the bug is fixed in the future

However, this doesn't handle every possible error path (specifically any where your own handler never gets called due to an error detected inside swagger itself).

@tooptoop4
Copy link

@bradfitz where is remove in net/http/#Request.MultipartForm ?

@dvdscripter
Copy link

Can someone validate in another OS? https://play.golang.org/p/5n60s8qSf_k

  • go1.17.1 windows/amd64: file hanging
  • go1.17.1 linux/amd64: clear!

@neild
Copy link
Contributor

neild commented Aug 9, 2022

The HTTP/1 server calls Form.RemoveAll on completion of a request, but it looks to me like the HTTP/2 server does not. That should be fixed.

On Windows, I suspect the same issue involving file deletion as seen in #25965 may apply, even in the HTTP/1 path.

@gopherbot
Copy link

Change https://go.dev/cl/423194 mentions this issue: net/http: add a test to verify form tempfiles are deleted

@gopherbot
Copy link

Change https://go.dev/cl/423055 mentions this issue: http2: delete multipart form tempfiles after ServeHTTP returns

@neild neild self-assigned this Aug 11, 2022
gopherbot pushed a commit to golang/net that referenced this issue Aug 12, 2022
Do the same post-handler cleanup as the HTTP/1 server does.
No test here; test for HTTP/1 and HTTP/2 in CL 423194.

For golang/go#20253.

Change-Id: Iba54110ad2844571076c721d7ec19c39ba36de79
Reviewed-on: https://go-review.googlesource.com/c/net/+/423055
Reviewed-by: Robert Griesemer <gri@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
gopherbot pushed a commit that referenced this issue Aug 12, 2022
The HTTP/1 server deletes multipart form tempfiles after ServeHTTP
returns, but the HTTP/2 server does not. Add a test to verify
cleanup happens in both cases, temporarily disabled for the
HTTP/2 path.

For #20253
Updates #25965

Change-Id: Ib753f2761fe73b29321d9d4337dbb5090fd193c2
Reviewed-on: https://go-review.googlesource.com/c/go/+/423194
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@DasSkelett
Copy link

@neild is a backport of https://go.dev/cl/423055 planned?

@neild neild removed help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Aug 26, 2022
@neild neild modified the milestones: Unplanned, Go1.20 Aug 26, 2022
@neild
Copy link
Contributor

neild commented Aug 26, 2022

@neild is a backport of https://go.dev/cl/423055 planned?

No, this doesn't meet the backport criteria: It isn't a security issue and there is a workaround (call r.MultipartForm.RemoveAll manually).

Thanks for reminding me that this issue can be closed with https://go.dev/cl/423055 submitted, since it'll be fixed in 1.20.

@neild neild closed this as completed Aug 26, 2022
WeiminShang added a commit to WeiminShang/net that referenced this issue Nov 16, 2022
Do the same post-handler cleanup as the HTTP/1 server does.
No test here; test for HTTP/1 and HTTP/2 in CL 423194.

For golang/go#20253.

Change-Id: Iba54110ad2844571076c721d7ec19c39ba36de79
Reviewed-on: https://go-review.googlesource.com/c/net/+/423055
Reviewed-by: Robert Griesemer <gri@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@golang golang locked and limited conversation to collaborators Aug 26, 2023
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