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: Seek suceeds for directories where it should error #45046

Open
tmthrgd opened this issue Mar 16, 2021 · 7 comments
Open

os: Seek suceeds for directories where it should error #45046

tmthrgd opened this issue Mar 16, 2021 · 7 comments

Comments

@tmthrgd
Copy link
Contributor

@tmthrgd tmthrgd commented Mar 16, 2021

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

$ go version
go version go1.16.2 linux/amd64

Does this issue reproduce with the latest release?

Yes with tip.

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/tom/.cache/go-build"
GOENV="/home/tom/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/tom/go/pkg/mod"
GONOPROXY=...
GONOSUMDB=...
GOOS="linux"
GOPATH="/home/tom/go"
GOPRIVATE=...
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/tom/sdk/go1.16.2"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/tom/sdk/go1.16.2/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.16.2"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build14584737=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I was looking through the os directory reading source code and I noticed that Seeks EISDIR error condition is broken.

Seek checks if the os-specific seek method succeeded for a file that has a dirinfo:

go/src/os/file.go

Lines 239 to 241 in f02a26b

if e == nil && f.dirinfo != nil && r != 0 {
e = syscall.EISDIR
}

Unfortunately the seek method always clears dirinfo (#35767 and #37161):

go/src/os/file_unix.go

Lines 269 to 274 in 78f9015

if f.dirinfo != nil {
// Free cached dirinfo, so we allocate a new one if we
// access this file as a directory again. See #35767 and #37161.
f.dirinfo.close()
f.dirinfo = nil
}

This script should reproduce the error when run locally:

https://play.golang.org/p/brgpk5je9tP

As an aside: I'm not sure this is the best way handle Seek-ing a directory, it will only trigger if readdir was previously called and, on Linux at least, only after the seek has actually succeeded.

What did you expect to see?

Seek returned a syscall.EISDIR error.

What did you see instead?

Seek returned without error.

@cherrymui cherrymui added this to the Backlog milestone Mar 16, 2021
@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Mar 16, 2021

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 16, 2021

As far as I know this is working as intended. Seeking in a directory can be meaningful. In particular you can call Seek(1, 0) to get the current offset, and then later you can seek back to that location. The test of dirinfo in (*File).Seek is to handle the case of WIndows, in which the seek method does not clear the dirinfo field.

Is there a particular reason that Seek should fail on directories on Unix systems?

@tmthrgd
Copy link
Contributor Author

@tmthrgd tmthrgd commented Mar 17, 2021

Here’s a corrected test program, I missed the Readdirnames call:

https://play.golang.org/p/INPumuqPZ-V

Seek-ing in directories on Unix used to fail:

$ go1.14.15 run dir-seek.go
Seek failed for directory as expected: seek /home/tom: is a directory
$ go1.16.2 run dir-seek.go
Seek succeeded unexpectedly for directory with pos=9223372036854775807

I’ve got no objections to keeping the current behaviour, but I can see nothing in the changes that cleared dirinfo that indicated it was intentionally allowing this. I’m pretty sure it was just an accident that this started working, despite a previous explicit check, hence I figured it was the wrong behaviour.

@tmthrgd
Copy link
Contributor Author

@tmthrgd tmthrgd commented Mar 17, 2021

Also while Seek can be meaningful at a system call level, I don’t see how it can be meaningful at a Go level. The dir_unix.go code buffers a full 8KiB of directory information on each read. If you tried to use Seek to remember a position, you’d always end up at the end of one of those 8KiB blocks, if I’m not mistaken, not immediately after the directory entry you’d just read as you’d expect.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 17, 2021

Fair enough. I hadn't realized that this failed in earlier releases.

Looks like this changed in https://golang.org/cl/219143. See #35767 and #37161. The tests cases for those issues involve seeking to position 0, which has always been permitted. But, as you said, that CL changed the code so that the check to return EISDIR when seeking to a non-0 offset no longer fails.

Interestingly, returning EISDIR when seeking to a non-0 offset dates back to when the Seek method was first introduced, in d94c5ab.

I'm not quite sure what behavior we should enforce going forward. CC @randall77

@randall77
Copy link
Contributor

@randall77 randall77 commented Mar 18, 2021

I think we want Seek on directories to work for (0, io.SeekStart), (n, io.SeekStart) for n returned from a previous call to (0, io.SeekCur), and (0, io.SeekCur). So whence == io.SeekStart || (whence == io.SeekCur && off == 0). Can we return an immediate error for Seek for other cases? That might be the behavior for <=1.13 anyway.

Practically speaking, we can change the order of the tests in (*FIle).Seek to put the dirinfo test before the call to (*File).seek.

Quoting myself from CL 209961:

p.s. I hate the Readdirnames API.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 18, 2021

Historically we've only permitted the case where the result of the Seek winds up being 0. That is, historically we have not accepted (n, io.SeekStart) for a value returned by a previous call to (0, io.SeekCur). (Despite what I said above). I think it would be OK to go back to that. Which means I think it would be OK to only accept (0, io.SeekStart), since while there are other ways to get to 0 they are needlessly complicated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants