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

os: TempDir() returns path without validation #19695

Closed
invisiblethreat opened this issue Mar 24, 2017 · 7 comments
Closed

os: TempDir() returns path without validation #19695

invisiblethreat opened this issue Mar 24, 2017 · 7 comments

Comments

@invisiblethreat
Copy link

@invisiblethreat invisiblethreat commented Mar 24, 2017

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
 }
@ALTree

This comment has been minimized.

Copy link
Member

@ALTree ALTree commented Mar 24, 2017

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.

@invisiblethreat

This comment has been minimized.

Copy link
Author

@invisiblethreat invisiblethreat commented Mar 24, 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.

@ALTree

This comment has been minimized.

Copy link
Member

@ALTree ALTree commented Mar 24, 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 ALTree added this to the Go1.9 milestone Mar 24, 2017
@ALTree ALTree changed the title os.TempDir() returns path without validation os: TempDir() returns path without validation Mar 24, 2017
@robpike

This comment has been minimized.

Copy link
Contributor

@robpike robpike commented Mar 24, 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.

@quentinmit

This comment has been minimized.

Copy link
Contributor

@quentinmit quentinmit commented Mar 24, 2017

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.

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Mar 25, 2017

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.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jun 14, 2017

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

@gopherbot gopherbot closed this in 0b81c02 Jun 14, 2017
@golang golang locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.