Skip to content

fs: ValidPath() docs seem to contradict test case regarding "." #70155

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

Closed
mholt opened this issue Nov 1, 2024 · 11 comments
Closed

fs: ValidPath() docs seem to contradict test case regarding "." #70155

mholt opened this issue Nov 1, 2024 · 11 comments
Labels
Documentation Issues describing a change to documentation.

Comments

@mholt
Copy link

mholt commented Nov 1, 2024

Go version

go version go1.23.2 linux/amd64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/matt/.cache/go-build'
GOENV='/home/matt/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/matt/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/matt/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/lib/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/lib/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.23.2'
GODEBUG=''
GOTELEMETRY='on'
GOTELEMETRYDIR='/home/matt/.config/go/telemetry'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/home/matt/Dev/archiver/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build3734862726=/tmp/go-build -gno-record-gcc-switches'

What did you do?

func (f *ArchiveFS) Open(name string) (fs.File, error) {
	if !fs.ValidPath(name) {
		return nil, &fs.PathError{Op: "open", Path: name, Err: fs.ErrInvalid}
	}

Trying to implement fs.FS with the use of fs.ValidName().

What did you see happen?

A path of ./foo is rejected, even though a file system literally has a root directory named ".", and the docs for fs.ValidPath() state (emphasis mine):

Path names must not contain an element that is “.” or “..” or the empty string, except for the special case that the root directory is named “.”.

Here, the root directory is named ".", and the docs say that should be an exception that is allowed. Yet the test case seems to show otherwise: {"./x", false},

What did you expect to see?

I might be reading the docs wrong, but it seems like this should be allowed, especially since there are virtual file systems (like a tar archive) where there are literally root directories named "." (this can happen apparently when a tar archive is created in the same directory it is archiving).

@gopherbot gopherbot added the Documentation Issues describing a change to documentation. label Nov 1, 2024
@seankhliao
Copy link
Member

It allows for Open(".") and nothing else. Any path such as ./foo should be just foo

@seankhliao seankhliao closed this as not planned Won't fix, can't repro, duplicate, stale Nov 1, 2024
@mholt
Copy link
Author

mholt commented Nov 1, 2024

Thanks but that doesn't really address my questions.

Can the docs be reworded then? Because ./foo complies with what is allowed currently.

Also what about a directory that is literally named .? How is one supposed to specify that?

@ianlancetaylor
Copy link
Contributor

Directories literally named . (other than the root directory) are not supported (nor are directories literally named ..).

You are reading the docs wrong. You are not permitted to refer to a directory as . except for one single special case: opening the root directory, which you can do with the name ".".

@mholt
Copy link
Author

mholt commented Nov 1, 2024

Okay, thanks for clarifying. Maybe the doc could be reworded/clarified, that it's referring to the root of the file system, not a folder in the root named ".".

This policy still leaves us with the problem of what to do about literal files named "." as appear in some tar archives (for example: request your user data from GitHub, and the downloaded tar has a "." folder in its root). Deciding that it's "not supported" or "not permitted" seems arbitrary when there are folders/files named ".". Is there a technical reason that this limitation exists, or is it just because we like clean paths?

@ianlancetaylor
Copy link
Contributor

The technical reason is that the io/fs package is providing an abstraction of a simple unrooted file system. By not permitting names like . we avoid confusion over the fact that in a Unix file system dir and dir/. name the same entity.

The expectation is that translation from some other file system to io/fs will clean paths as necessary. If a tar archive contains a file ./x, then presenting an io/fs view of that tar archive will read ./x as x. So I don't quite see the problem you are describing.

@mholt
Copy link
Author

mholt commented Nov 1, 2024

The problem is an archive that looks like:

x
.
./x

now becomes untenable because this restriction in io/fs forces us to treat ./x as x, which conflicts with x.

@ianlancetaylor
Copy link
Contributor

In that tree are x and ./x intended to be different files? That is a hierarchy that can't be represented directly in io/fs, nor, for that matter, in a Unix file system.

@mholt
Copy link
Author

mholt commented Nov 1, 2024

Yep. I agree, that's just how it is though. (Perhaps the tar utility is to blame.)

Anyway, I will try to find a work around. (But I still recommend a clarification of the docs)

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/624595 mentions this issue: io/fs: clarify that "." may only be used for root

@mholt
Copy link
Author

mholt commented Nov 2, 2024

Looks good. 👍 Thank you!

gopherbot pushed a commit that referenced this issue Nov 5, 2024
For #70155

Change-Id: I648791c484e19bb12c6e4f84e2dc42eaaa4db546
Reviewed-on: https://go-review.googlesource.com/c/go/+/624595
Reviewed-by: Damien Neil <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Auto-Submit: Ian Lance Taylor <[email protected]>
Reviewed-by: David Chase <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Issues describing a change to documentation.
Projects
None yet
Development

No branches or pull requests

5 participants