Go: os: TempDir() returns path without validation

Created on 24 Mar 2017 · 7Comments · Source: golang/go

Please answer these questions before submitting your issue. Thanks!

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

go version go1.8 darwin/amd64

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/swalsh/go"
GORACE=""
GOROOT="/usr/local/Cellar/go/1.8/libexec"
GOTOOLDIR="/usr/local/Cellar/go/1.8/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/ld/hnhrnmwd19z1_xc7pbqrg6wr0000gn/T/go-build138991087=/tmp/go-build -gno-record-gcc-switches -fn
o-common"
CXX="clang++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

Used os.TempDir() in a project.

To reproduce:

Set environment variable "TMPDIR" to something nonsensical.
Write something that uses os.TempDir()
Attempt to use the resulting string returned by os.TempDir() for filesystem operations.

What did you expect to see?

os.TempDir() to return an error that the path doesn't exist.

What did you see instead?

os.TempDir() passing back a string without validation that the path exists via Stat()

Proposed patch:

diff --git a/src/os/file_unix.go b/src/os/file_unix.go
index 6e00f48..5f5a017 100644
--- a/src/os/file_unix.go
+++ b/src/os/file_unix.go
@@ -282,7 +282,7 @@ func Remove(name string) error {
 }

 // TempDir returns the default directory to use for temporary files.
-func TempDir() string {
+func TempDir() (s string, err error) {
        dir := Getenv("TMPDIR")
        if dir == "" {
                if runtime.GOOS == "android" {
@@ -291,7 +291,11 @@ func TempDir() string {
                        dir = "/tmp"
                }
        }
-       return dir
+       err := Stat(dir)
+       if err != nil {
+               return nil, err
+       }
+       return dir, nil
 }

Documentation FrozenDueToAge NeedsFix

Source

invisiblethreat

👎2

Most helpful comment

As Rob said, we can't change the signature.

We could update the docs to warn that the directory may not exist, but they're already pretty good:

TempDir returns the default directory to use for temporary files.

(It says "default" at least, hinting perhaps that it's a conventional value being returned and not something guaranteed? But kind of a stretch.)

We could add "It is not guaranteed to exist if the person running the program did something weird." but if you put warning labels on everything, people start to glaze over them.

Maybe just: "The directory may not exist."

Or.... we document exactly what the function does. That it returns the value of $TMPDIR if non-empty in the environment, otherwise returns the conventional path based on the operating system.

On Windows we use GetTempPath (https://msdn.microsoft.com/en-us/library/windows/desktop/aa364992(v=vs.85).aspx) which has the same behavior. ("Note that the function does not verify that the path exists, nor does it test to see if the current process has any kind of access rights to the path.")

I'll mark this as a documentation issue.

bradfitz on 25 Mar 2017

👍2

All 7 comments

This is more or less a dup of #12823 (vague error when TMPDIR is set to directory that doesn't exist).

It looks like the consensus was that it's not worth doing any validation work on the TempDir return value.

We provide os.TempDir and letting users to run arbitrary file operations on the returned value. On the other hand, the temp directory's status may change in the lifecycle of the Go program. It could be removed, created, mounted, unmounted, etc.

Yeah, I think we can't do anything here. Even if we fixed ioutil.TempDir to check for the case, there could be other cases where the code tries to use the temporary directory and fails. It doesn't make sense to check for this special case everywhere in package os.

ALTree on 24 Mar 2017

I appreciate the quick response. :)

I feel that there are bad assumptions being made about /tmp always existing the the days of containers and the strange things that people do. Using a Dockerfile with FROM scratch is a case that comes to mind.

I also don't understand the aversion to checking, what is user input, TMPDIR. The argument of things happening to the filesystem after execution has started is not unique to /tmp- it can happen anywhere in the FS, at any time, for any reason. Why not give the benefit of at least checking if an operation can succeed at the outset, rather than waiting for a nebulous failure at some later point in execution?

I appreciate your time, however, if you still feel that it's not worth the work, close this issue as "unfortunate" as well.

invisiblethreat on 24 Mar 2017

I don't have strong feelings about this (I linked the past discussion just for reference); so I'm not closing this; maybe it would be acceptable to at least validate TMPDIR before doing anything.

Labelling as needing-decision.

ALTree on 24 Mar 2017

Tempdir cannot change signature without breaking the Go 1 compatibility guidelines, so there is no clean way to fix this, if indeed it is a problem.

Leaving open for now in case anyone has insight, but I think this is just unfortunate. Don't set your TMPDIR do a nonsense value.

robpike on 24 Mar 2017

👍1

Look at it this way: the code that's calling os.TempDir is already checking whether it exists, when it tries to create a new file inside the directory. It's generally an anti-pattern to stat something before you try to use it; as @invisiblethreat says, people are doing all sorts of crazy things with their systems, and it's not implausible that e.g. /tmp is not listable but new files can be created inside it. Adding a call to stat /tmp before returning it could actually break these systems.

quentinmit on 24 Mar 2017

👍1