-
Notifications
You must be signed in to change notification settings - Fork 18k
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
Comments
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. |
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. |
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. |
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. |
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. |
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:
The text was updated successfully, but these errors were encountered: