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: Form fields saved to "disk" may actually go to RAM #12783

Closed
hlandau opened this issue Sep 29, 2015 · 5 comments
Closed

mime/multipart: Form fields saved to "disk" may actually go to RAM #12783

hlandau opened this issue Sep 29, 2015 · 5 comments

Comments

@hlandau
Copy link

hlandau commented Sep 29, 2015

multipart.ReadForm is supposed to store up to maxMemory bytes in RAM and then the remainder on disk. The idea, presumably, is to prevent RAM exhaustion.

Temporary files are created using ioutil.TempFile. os.TempDir is used to determine the temporary directory to use. However, os.TempDir() returns $TMPDIR, or, failing that, "/tmp".

The problem is that on modern Linux systems, /tmp is a tmpfs and is thus hosted in RAM. This defeats the entire objective of saving large multipart parts to files.

/var/tmp should always be backed by disk. The FHS states the purpose of "/var/tmp" as "temporary files preserved between system reboots." multipart should use /var/tmp, not /tmp.

(Since there is no actual need for temporary multipart files to be preserved across reboots, it would be nice if the FHS provided something in between, thus allowing for periodic deletion on boot or so on, but alas it does not.)

Possible solutions:

  • Introduce an os.LargeTempDir() which returns a temporary directory to be used for large files. Change multipart to pass this value to ioutil.TempFile.
@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Sep 29, 2015
@bradfitz
Copy link
Contributor

tmpfs will go to disk (to swap) if there's no useful RAM. But that's beside the point...

The ParseMultipartForm mechanism (https://golang.org/pkg/net/http/#Request.ParseMultipartForm) that slurps it all up is a convenience mechanism. Anybody with exact requirements about bounding memory-vs-disk usage should use the lower-level streaming interface.

I don't want to make the simple interface more complicated, when sophisticated users should just be using the sophisticated interface in the first place.

@hlandau
Copy link
Author

hlandau commented Sep 29, 2015

By that logic, the file-saving functionality in mime/multipart is redundant and can be removed, as the 'maxMemory' argument serves no purpose, since it all ends up in swap anyway.

Either this is a bug in mime/multipart, or mime/multipart's API contains vestigal components.

Also, this has nothing to do with net/http as such. It's Reader.ReadForm that I'm talking about here.

Making mime/multipart use /var/tmp internally doesn't change or complicate the interface exposed by mime/multipart let alone net/http, so fixing this doesn't seem like a big issue.

@bradfitz
Copy link
Contributor

https://golang.org/doc/go1compat prevents us from removing things.

The standard library is full of duplicate ways to do things: e.g. http.Get vs making your own http.Client and http.Transport and your own http.Requests. It's a trade-off between convenience and control.

Feel free to bind /tmp wherever you want for your Go programs. Or set the TMPDIR environment variable.

@ianlancetaylor
Copy link
Contributor

As I understand it, the primary difference between /tmp and /var/tmp is that /tmp is cleared on reboot. So it seems appropriate to me that mime/multipart default to using /tmp. You can of course control it by setting the TMPDIR environment variable, as with most programs.

@hlandau
Copy link
Author

hlandau commented Sep 30, 2015

Barring a fix for this, the documentation for ReadForm is misleading and would cause most people to believe that large parts would go to disk-backed storage. It could be updated to point out the hazard of /tmp on modern Linux systems and advise people to set TMPDIR.

Since the size of swap space is usually bounded on Linux systems, it is more precious than regular disk space and thus use of swap vs. use of disk is not strictly equivalent.

For the benefit of others, this issue can be worked around by setting a default TMPDIR. This is at least safer than hoping the environment will do so:

if os.Getenv("TMPDIR") == "" {
  os.Setenv("TMPDIR", "/var/tmp")
}

TMPDIR is not used by Windows, so this code is inconsequential on that platform.

@golang golang locked and limited conversation to collaborators Oct 4, 2016
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

4 participants