golang/go

io/fs: add ReadLinkFS interface

zombiezen opened this issue ยท 100 comments

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

$ go version
go version go1.17.3 linux/amd64

Does this issue reproduce with the latest release?

Yes

What did you do?

Walked a directory with fs.WalkDir and encountered a symlink that I wanted to read.

What did you expect to see?

A function fs.ReadLink that behaves like os.Readlink, but operates on an fs.FS. Design sketch:

package fs

// ReadLink returns the destination of the named symbolic link.
//
// If fsys does not implement ReadLinkFS, then ReadLink returns an error.
func ReadLink(fsys FS, name string) (string, error)

// ReadLinkFS is the interface implemented by a file system that supports symbolic links.
type ReadLinkFS interface {
  FS

  // ReadLink returns the destination of the named symbolic link.
  ReadLink(name string) (string, error)
}

I would also want the file system returned by os.DirFS to have an implementation that calls os.Readlink. IIUC archive/zip.Reader would probably also benefit from an implementation.

An open question in my mind is whether the returned destination should be a slash-separated path or kept as-is. I think for consistency it probably should convert to a slash-separated path, but I'm not sure if this has problems on Windows.

What did you see instead?

No such API exists.

Other details

I have bandwidth to contribute an implementation of this, but I understand we're in the freeze and the earliest this could go in is Go 1.19.

This is somewhat related to #45470, but I'm not proposing changing any existing semantics, just adding a new method.

An open question in my mind is whether the returned destination should be a slash-separated path or kept as-is.

I would say it should be slash-separated and also relative to the same FS: links to absolute paths should be made relative, and links to paths above the FS root (or on an entirely different volume) should be rejected.

rsc commented

Rewriting the link is tricky and not rewriting the link is also tricky. It's unclear to me what we should do here, if anything.

rsc commented

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
โ€” rsc for the proposal review group

As a data point, for the application I was writing, rewriting the link to be relative is effectively what I did anyway. I wanted to create a zip archive of an on-disk directory, so absolute paths were rewritten to be relative to the DirFS root. Returning an error if the path could not be represented was acceptable.

rsc commented

So the API for ReadLink is that it must return a path relative to the link, and if it can't do that, then it returns an error?
It seems like most symlinks are absolute, though, and most DirFS(foo) will not use foo = "/", so that will make most symlinks result in errors?

I'm trying to understand how useful this will be in practice, to balance against the cost. Will it be useful in practice? Or will people just be frustrated that 99% of symlinks aren't usable?

So the API for ReadLink is that it must return a path relative to the link, and if it can't do that, then it returns an error?

Yes.

It seems like most symlinks are absolute, though, and most DirFS(foo) will not use foo = "/", so that will make most symlinks result in errors?

I would expect the ReadLink method to internally transform absolute link paths to relative ones (for example, by using filepath.Rel and then checking that the returned path does not begin with ../.) That would allow symlinks to succeed when they are to other locations under the FS root โ€” they would only fail if they are absolute symlinks that would otherwise refer to locations completely outside the FS tree.

So, for example, os.DirFS("/") on Unix would be able to resolve symlinks anywhere on the filesystem, even if those symlinks are absolute.

I would expect the ReadLink method to internally transform absolute link paths to relative ones (for example, by using filepath.Rel and then checking that the returned path does not begin with ../.) That would allow symlinks to succeed when they are to other locations under the FS root โ€” they would only fail if they are absolute symlinks that would otherwise refer to locations completely outside the FS tree.

Does this extend to non local-disk filesystems?

@rsc:

So the API for ReadLink is that it must return a path relative to the link, and if it can't do that, then it returns an error?

That was sufficient for my application. The root of my DirFS could contain multiple projects (think a GOPATH-like setup), so symlinks would usually resolve within the io/fs filesystem.

It seems like most symlinks are absolute, though, [...]

I'm not 100% convinced of that, but I don't have evidence to dispute your claim.

FWIW the cases that I was concerned with in a DevTools context:

  • Symlinks within the same directory. A common case is multiple aliases for a single on-disk program, like Busybox.
  • IIRC old versions of Git would use symlinks to represent symbolic references like HEAD instead of a file.

I'm trying to understand how useful this will be in practice, to balance against the cost. Will it be useful in practice? Or will people just be frustrated that 99% of symlinks aren't usable?

Agreed, I think weighing this tradeoff is the trickiest part of this proposal.

Relative links are IMO the most useful for a consumer of this API. I could imagine an implementation of ReadLink also returning absolute paths in the case where the returned path is above the root io/fs directory, but this might add complexity when the paths aren't slash-separated. However, I tend to agree with @bcmills that being strict about this is probably better.

You're probably already considering this, but it's just FS.ReadLink that would be picky about the link target. The implicit interface of DirFS.Open is to follow symlinks (again, #45470 tracks spelling out that behavior for other filesystems), and symlinks are already visible in directory listings.


@hherman1:

Does this extend to non local-disk filesystems?

I'm proposing the slash-separated relative path restriction would extend to non-local-disk filesystems, yes. How each implementation meets this contract is up to the individual filesystem.

rsc commented

@zombiezen, thanks for the use cases. It seems fairly unobtrusive to add ReadLinkFS and fs.ReadLink, so the cost seems low and the benefit > 0.

@bcmills:

I would expect the ReadLink method to internally transform absolute link paths to relative ones (for example, by using filepath.Rel and then checking that the returned path does not begin with ../.) That would allow symlinks to succeed when they are to other locations under the FS root โ€” they would only fail if they are absolute symlinks that would otherwise refer to locations completely outside the FS tree.

So, for example, os.DirFS("/") on Unix would be able to resolve symlinks anywhere on the filesystem, even if those symlinks are absolute.

Rewriting symlinks may run into problems. I've been burned enough that I'm a bit wary about that. Should we start with just erroring out on the absolute ones?

I think it would be fine to start by erroring out on absolute links, and perhaps define a specific error (or error type) for symlinks that refer to locations outside of the FS.

From the perspective of fs.ReadLinkFS, the requirement would be that every returned path is relative to the passed-in name and below the FS root. (It would be up to the specific FS implementation to decide whether to achieve that by rewriting absolute links or rejecting them.)

rsc commented

OK, so it sounds like we agree on ReadLink and ReadLinkFS but with the restriction that the returned link must be relative, and absolute symlinks return errors instead. Do I have that right?

IIUC the full constraint for the returned link is relative and underneath the root of FS. Otherwise, yes, that matches my understanding.

rsc commented

Based on the discussion above, this proposal seems like a likely accept.
โ€” rsc for the proposal review group

rsc commented

No change in consensus, so accepted. ๐ŸŽ‰
This issue now tracks the work of implementing the proposal.
โ€” rsc for the proposal review group

Excellent! I'm working on a CL now.

Implementation question: I would very much like to be able to use filepath.Clean and filepath.Rel in the os.DirFS implementation, but that would introduce a cyclic dependency AFAICT. What's the best way to use those functions?

In principle you could move the functions into an internal package imported by both the os package and the path/filepath package. But try to avoid that if you can. Better to keep the os package focused on the simplest possible approach. In particular I don't see why the os package would need to use Clean.

Change https://go.dev/cl/385534 mentions this issue: io/fs: add ReadLinkFS interface

I have use-cases which require the ability to have an absolute path returned, namely, taking an fs.FS and taring it up (tons of symlinks to /bin/busybox), would it be possible to make it so that an absolute path can be returned if opted into?

@kaniini Let's do this version now and then work out whether it can be extended in some sensible way. Since the io/fs interface doesn't use absolute paths (https://pkg.go.dev/io/fs#ValidPath) it's not really obvious how a ReadLink version of io/fs should handle absolute paths.

Sure, I have a workaround that will work for now. Just noting that there are usecases :)

mvdan commented

I have a use case for an io/fs.FS abstraction supporting symlinks, similar to the proposal author here. However, I want almost the opposite of ReadLink - I want an Lstat, as fs.StatFS.Stat is effectively equivalent to os.Stat in that it follows symlinks.

It seems to me that, if an io/fs.FS supports symlinks, it should support both ReadLink and Lstat. Without Lstat, it's very hard to get the FileInfo for just a symlink if you have its path: using ReadDir on its parent directory won't always work, as some paths aren't canonical, like short path names on Windows.

I realise I'm late to this proposal and it's already accepted, but I would imagine that it would be better to instead have:

type SymlinkFS interface {
	FS

	// ReadLink returns the destination of the named symbolic link.
	ReadLink(name string) (string, error)

	// Lstat returns a FileInfo describing the file without following any symbolic links.
	// If there is an error, it should be of type *PathError.
	Lstat(name string) (FileInfo, error)
}

An alternative is to add an LstatFS next to ReadLinkFS and StatFS, but I can't imagine why one would have ReadLink without being able to have Lstat or vice versa.

I have a use case for an io/fs.FS abstraction supporting symlinks, similar to the proposal author here. However, I want almost the opposite of ReadLink - I want an Lstat, as fs.StatFS.Stat is effectively equivalent to os.Stat in that it follows symlinks.

I initially thought the same. However, after playing with these ideas for a bit, I don't think this is necessary or desirable.

"Following a symlink" is a concept that can only apply to mounted filesystems. Inside your go code, you might be able to do some guessing in cases where your symlinks are relative and don't cross your "filesystem barrier", but as soon as it gets more complicated than that, following becomes impossible.

I guess the right call here is to only return information about the requested path, and not following. Together with the ReadLink(name string) (string, error) introduced in https://go-review.googlesource.com/c/go/+/385534/, this should be sufficient.

It's a bit confusing that the function in io.File is called Stat(), while it is essentially an Lstat, but that interface is probably too widely used to be changed.

mvdan commented

@flokli you are right that the lstat and readlink operations are all that one needs to fully support symbolic links - stat can be implemented by combining both. The only reason I suggested to add Lstat instead of repurposing the existing Stat is for backwards compatibility. The io/fs docs do not say anything about following symlinks, but the fact that the method is called FS.Stat, plus how it uses os.Stat underneath which follows symlinks, makes me think that changing FS.Stat to not follow symlinks would break a few too many programs.

It's a bit confusing that the function in io.File is called Stat(), while it is essentially an Lstat, but that interface is probably too widely used to be changed.

Arguably this is an argument in favor of adding Lstat to not follow symlinks, rather than to repurpose Stat to not follow symlinks, further adding to the confusion :)

But also, I think you've got a valid point about the older APIs: os.File.Stat should be clarified to mention that it does not follow symlinks. Its godoc is nearly identical to os.Stat, which does follow symlinks.

What is the status on this one? The CL seems to be stale for more than a year. I have a use case for ReadLinkFS in the new AddFS apis for zip and tar. If we can identify the bits that are missing to implement this maybe we could finish the work to get this merged.

Happy to jump in and finish the work if no one is going to work on it in the near future.

What is the status on this one? The CL seems to be stale for more than a year.

Sorry about that. I need some guidance on how to implement the relative path restrictions in the os package without cyclic dependency issues. I sketched out a potential solution in the CL, but I'd like folks from the project to weigh in on whether this is desirable.

I have a use case for ReadLinkFS in the new AddFS apis for zip and tar. If we can identify the bits that are missing to implement this maybe we could finish the work to get this merged.

Happy to jump in and finish the work if no one is going to work on it in the near future.

If I get guidance, I think I can finish the CL.

kindly CC @bcmills

mvdan commented

I realise that my comment at #49580 (comment) arrived weeks after the proposal was accepted, but I was hoping to get the extra Lstat method included or at least discussed as part of this proposal. If the interface shipped as-is in Go 1.22, adding an extra method in a later Go release would be a breaking change.

Just wanted to acknowledge @mvdan's message: I don't think it would be much more complexity to add it. That said, since I'm more of a casual contributor I don't feel comfortable making the judgement call on it. Would either @bcmills or @ianlancetaylor be willing to weigh in here as to whether Lstat should be included in the io/fs interface (and IIUC the interface should be renamed io/fs.SymlinkFS)?

I'm not in the proposal review group, but I've flagged this question for them to discuss.

rsc commented

Moving proposal back to Active to talk about SymlinkFS.

rsc commented

Assuming we add ReadLink and Lstat together in SymlinkFS, have all remaining concerns about this proposal been addressed?

rsc commented

It sounds like the API would be:

type SymlinkFS interface {
	FS

	// ReadLink returns the destination of the named symbolic link.
	ReadLink(name string) (string, error)

	// Lstat returns a FileInfo describing the file without following any symbolic links.
	// If there is an error, it should be of type *PathError.
	Lstat(name string) (FileInfo, error)
}

along with toplevel

func ReadLink(fsys FS, name string) (string, error)
func Lstat(fsys FS, name string) (FileInfo, error)

That matches my understanding. ๐Ÿ‘ I can add that to the CL.

The types listed here basically match the interface we are using internally in our projects, so they would be fine for us.

mvdan commented

Thank you for considering Lstat even though I arrived late to the proposal process :)

rsc commented

Based on the discussion above, this proposal seems like a likely accept.
โ€” rsc for the proposal review group

Can someone clarify for me why the proposed readlink function would modify the symlinks at all (or rather, why that's mandated)? In the filesystem, symlinks are mostly effectively just special files, and dangling symlinks are a valid use case, for example.

As a more concrete example, I've got an implementation of FS for a Git repo for which symlinks have been a pain, but an interface that puts extra constraints on what I can return about symlinks doesn't really resolve the problem for me (symlinks might point outside the repo, to non-existent files, etc, and those are all valid and often useful to the consumer; in those cases, Lstat would succeed but Stat would error and that's reasonable and matches the behavior on a normal system).

mvdan commented

@tianon where do you see that modification being documented or implemented?

I guess I'm probably just misunderstanding the discussions here around the use of Rel and Clean (and absolute path restrictions) and now I realize those are probably specific to a particular implementation of the interface ๐Ÿคฆ apologies!

rsc commented

No change in consensus, so accepted. ๐ŸŽ‰
This issue now tracks the work of implementing the proposal.
โ€” rsc for the proposal review group

The CL is ready for review with the new changes.

@zombiezen Is that https://go.dev/cl/385534 ? It seems to have picked up a merge conflict.

Yes, that's the one. I'll go fix that.

dsnet commented

I apologize for bikeshedding now, but should we declare two interfaces instead of one?

type ReadLinkFS interface {
    ReadLink(name string) (string, error)
}
type StatLinkFS interface {
    StatLink(name string) (FileInfo, error)
}

Arguments for this:

  1. It follows the current pattern where each interface type with a FS suffix only contains a single method
    and the prefix is named on the method itself.
  2. If we ever support writable interfaces, I fear that SymlinkFS would take a valuable name for the Symlink method itself.
mvdan commented

My thinking was to have both in the same interface because, on the reading side, I can't think why an implementation would be able to provide one method but not the other. They implement two sides of the same feature, so I wouldn't split them up unless we have good reason to.

Your arguments about following precedent are interesting. I can't say I disagree with them, but I'm also not terribly concerned if we aren't consistent.

Speaking of late bikeshedding... I just noticed that the accepted design at #49580 (comment) uses the names ReadLink and Lstat, whereas os uses Readlink (one uppercase) and Lstat. Did we do that on purpose? If the point is to be more user friendly, then perhaps something like Joe's StatLink is more self-describing than Lstat. I only suggested Lstat to mirror os, but we already aren't mirroring it with ReadLink.

dsnet commented

I just noticed that the accepted design at #49580 (comment) uses the names ReadLink and Lstat, whereas os uses Readlink (one uppercase) and Lstat. Did we do that on purpose?

I assumed yes since there was a light precedent set forth by fs to rename legacy C-like names. For example, we added os.File.ReadDir and codified that method in the fs.ReadDirFile interface instead of using the legacy os.File.Readdir (with lower case dir) name.

dsnet commented

The relevance of my concern arises because I'm working on a package that extends io/fs to support full writeability. I had defined an interface called SymlinkFS that just includes the Symlink method. I wanted to design the package to be forwards compatible with what was potentially being added to the io/fs package, and the name conflict over SymlinkFS came up.

@ianlancetaylor @bcmills With the release freeze coming, what's left to do for this CL? IMO this is ready to go, barring any changes based on @dsnet's comments.

Looking forward to SymlinkFS support; I have had a case recently writing a routine to copy some contents from an io/fs.FS to another format, and due to the lack of symlink support, needed to port it to a different filesystem abstraction.

dsnet commented

I was chatting with @mvdan and I was emphasizing that my main concern was point 2 in my earlier comment (which I should have listed first):

If we ever support writable interfaces, I fear that SymlinkFS would take a valuable name for the Symlink method itself.

To elaborate further, this arises as I was working on a project that needed writable virtual FS interfaces. While working on that, I had a clone of the "io/fs" API but with additional interfaces to support writing. One of my declarations was SymlinkFS to, well, create symlinks. Thus, the addition of an interface called SymlinkFS is stealing a good name from that possible future world.

@mvdan and I propose:

type ReadLinkFS interface {
    ReadLink(name string) (string, error)
    StatLink(name string) (FileInfo, error)
}

which:

  • Renames SymlinkFS as ReadLinkFS to avoid stealing the SymlinkFS name.
  • Combines ReadLink and StatLink into the same interface as proposed by @mvdan in #49580 (comment). The naming of the interfaces takes after the ReadLink method and StatLink comes along for the ride. Stat is still a "read" operation, so this doesn't seem that objectionable.
  • Renames Lstat as StatLink to be consistent in naming style as ReadLink and to C-style names in a modern Go API.

@dsnet LGTM. I can make that change to the CL.

mvdan commented

@ianlancetaylor @rsc #49580 (comment) tweaks the names of the interface of one of the methods compared to the accepted version as of #49580 (comment); does that sound good?

It's unclear to me if we want to re-open the proposal as a "likely accept" for a week or two to see if anyone disagrees. In any case I'd love to have final agreement in the next week or two so that we can merge the updated CL before the 1.23 freeze.

Another point in favor of calling this new interface ReadLinkFS: it has symmetry with the existing ReadFileFS and ReadDirFS interfaces.

rsc commented

We've had enough questions about SymlinkFS that it seems like we should wait until after Go 1.23 to try to add this.

mvdan commented

I'm saddened by the delay, particularly given that we hopefully have consensus and the patch is ready, but we are only two weeks away from the release at this point, so I can't argue with that.

However, can we at least reach consensus on the revised interface for the accepted proposal, with the intent to add it as-is to 1.24? I say this because multiple downstream projects are blocked by the interface uncertainty. At least two of mine need to use something like ReadLinkFS as part of their API, but they can't declare their own methods on top of io/fs.FS because if the method signatures end up differing from the final accepted and merged proposal in Go, then they would be incompatible. At least I could start adding them next week if I can have some certainty that Go 1.24 will have exactly the interface we are proposing here.

An alternative would be to only add the interface to 1.23, and all the other code implementing and using it in 1.24. That would be a strict version of what I'm asking in the paragraph above, and I don't see any risk as long as we are able to reach consensus in two weeks - and I think we already have it today.

rsc commented

It sounds like the new new API is

type ReadLinkFS interface {
	FS

	// ReadLink returns the destination of the named symbolic link.
	ReadLink(name string) (string, error)

	// StatLink returns a FileInfo describing the file without following any symbolic links.
	// If there is an error, it should be of type *PathError.
	StatLink(name string) (FileInfo, error)
}

along with toplevel

func ReadLink(fsys FS, name string) (string, error)
func StatLink(fsys FS, name string) (FileInfo, error)

It remains an error for ReadLink to return an absolute path.

I am also not entirely clear on whether ReadLink can return a path beginning with ../.
The discussion in late 2021 seemed to suggest returning paths relative to the root of the FS, but that's clearly incorrect: real symlinks are relative to the place where they exist in the file system. So for example a symlink x->y in /home/rsc/bin directory means that /home/rsc/bin/x and /home/rsc/bin/y denote the same file. ReadLink(os.DirFS("/home/rsc"), "bin/x") returning "y" has to mean "/home/rsc/bin/y" not "/home/rsc/y". Similarly ReadLink(os.DirFS("/"), "home/rsc/bin/x")) returning "y" also has to mean "/home/rsc/bin/y". The meaning differs based on the base fsys root.

But then it becomes less clear exactly when .. is allowed. For example again taking those two examples,

ReadLink(os.DirFS("/"), "home/rsc/bin/x") = "../../rob/bin/x"
ReadLink(os.DirFS("/home/rsc"), "bin/x") = "../../rob/bin/x"

Is one of these permitted and one of them disallowed? That's a bit strange, and it becomes stranger still with SubFS. Does SubFS have to know that ReadLinkFS exists and munge the answers?

It's just not clear to me at all that we've arrived at a good place yet. Symlinks are horrible.

@rsc Replies inline.

It sounds like the new new API is [...] along with toplevel [...] It remains an error for ReadLink to return an absolute path.

Correct. The intent is that it would be easier to relax the absolute path restriction later if we decide to go that way. It's unclear to me at least how absolute paths would work for Windows systems in particular, where such paths are not slash-separated. The current rules were chosen because such symlink targets can be easily resolved as path.Join(path.Dir(symlinkPath), symlinkTarget).

I am also not entirely clear on whether ReadLink can return a path beginning with ../.

It can, as long as that path is contained within the filesystem. There are several tests in the CL that check for this case.

The discussion in late 2021 seemed to suggest returning paths relative to the root of the FS, but that's clearly incorrect: real symlinks are relative to the place where they exist in the file system. So for example a symlink x->y in /home/rsc/bin directory means that /home/rsc/bin/x and /home/rsc/bin/y denote the same file. ReadLink(os.DirFS("/home/rsc"), "bin/x") returning "y" has to mean "/home/rsc/bin/y" not "/home/rsc/y". Similarly ReadLink(os.DirFS("/"), "home/rsc/bin/x")) returning "y" also has to mean "/home/rsc/bin/y". The meaning differs based on the base fsys root.

I'm not sure that was the consensus, but regardless, agreed that relative-to-root-of-FS is wrong. The CL implements relative-to-directory-of-symlink behavior. From the doc comment in the CL on ReadLinkFS: "Link destinations will always be slash-separated paths relative to the link's directory. The link destination is guaranteed to be a path inside FS."

But then it becomes less clear exactly when .. is allowed. For example again taking those two examples,

ReadLink(os.DirFS("/"), "home/rsc/bin/x") = "../../rob/bin/x"
ReadLink(os.DirFS("/home/rsc"), "bin/x") = "../../rob/bin/x"

Is one of these permitted and one of them disallowed?

The first one is permitted. The second one is disallowed under the current rules.

That's a bit strange, and it becomes stranger still with SubFS. Does SubFS have to know that ReadLinkFS exists and munge the answers?

Yes, and that is implemented in the CL. SubFS does not change the answers, but it does limit which ReadLinks will return successfully. DirFS will change OS-specific paths into slash-separated paths before returning, but it does that uniformly.

Symlinks are horrible.

+1. ๐Ÿ˜†

FWIW, I'm pretty happy with where the API is now, given the underlying complexity.

rsc commented

Trying to get back to this, I wrote back in May that it sounded like the API was:

type ReadLinkFS interface {
	FS

	// ReadLink returns the destination of the named symbolic link.
	ReadLink(name string) (string, error)

	// StatLink returns a FileInfo describing the file without following any symbolic links.
	// If there is an error, it should be of type *PathError.
	StatLink(name string) (FileInfo, error)
}

along with toplevel

func ReadLink(fsys FS, name string) (string, error)
func StatLink(fsys FS, name string) (FileInfo, error)

Because ReadLink can only return a relative symlink, it should not return a symlink that ..'s above the root. Then SubFS has to disallow a different dot-dot depth but otherwise it can leave the results alone.

Today we have the property that if /tmp/x -> ../etc or /tmp/x -> /etc, then

fs := os.DirFS("/tmp")
_, err := fs.Open("x/passwd")

works just fine. That has to remain true even if we add the new SymlinkFS interface. Now, if /tmp/asdf is a plain file, then fs.ReadLink("asdf") and fs.StatLink("asdf") return errors. I suppose that ReadLink("x") and StatLink("x") return errors as well, it's just that the error is "bad symlink" as opposed to "not a symlink".

This all seems terrible, but I don't see a way to hold back the flood. It seems like we have to add symlinks in some form, and these semantics seem like the best of a bunch of bad alternatives. People who care about restricting symlink use can use the APIs being designed in #67002 instead of os.DirFS.

Do I have this right?

That is correct with two small corrections:

Now, if /tmp/asdf is a plain file, then fs.ReadLink("asdf") and fs.StatLink("asdf") return errors.

ReadLink returns an error, but StatLink succeeds and returns a plain file FileInfo. This matches the behavior of Lstat.

I suppose that ReadLink("x") and StatLink("x") return errors as well, it's just that the error is "bad symlink" as opposed to "not a symlink".

Similarly, ReadLink returns an error, but StatLink does not, but otherwise operates as you describe.

neild commented

Should os.DirFS("/") return absolute symlinks from ReadLink?

I'm trying to work out what (if anything) should happen with links-to-links. Consider this filesystem:

  • a => b/../../file
  • b => c/d
  • d/e/ is a directory
  • file is a file

What does ReadLink("a") return?

As specified above, it returns an error, because b/../../file escapes .. But if we resolve the full link chain, it doesn't escape: b resolves to c/d and the ../.. ascends back to the fs root.

neild commented

With apologies for adding suggestions to an accepted proposal, some thoughts on further consideration:

In the presence of symlinks, it isn't possible to lexically determine whether a path like b/../../file escapes from the root directory or not. (As shown by my example above.)

Lexically cleaning a path containing .. components on a Unix or Unix-like filesystem can change its meaning. path.Clean("b/../file") is file, but if b is a link to c/d the path should resolve to c/file.

Therefore, I think that fs.ReadLink should return relative paths unchanged. If the path escapes from the filesystem root, the caller can decide what to do with that fact.

Absolute paths are different, since we have no way to distinguish between an absolute path relative to the FS root and one relative to the external filesystem. I think that fs.ReadLink should be permitted to return an absolute path, but only if that path is relative to the root of fs. This allows for an fs.FS backed by a zip file containing absolute link targets, but requires that os.DirFS.ReadLink return an error for absolute targets unless the DirFS root is /.

To summarize:

  • ReadLink may return paths containing .. components which lexically escape the filesystem root.
  • ReadLink may return absolute paths.
  • On non-Windows: os.DirFS.ReadLink returns an error when resolving a link with an absolute target, unless the fs root is /.
  • On Windows: DirFS.ReadLink always returns an error when resolving a link with an absolute target (c:\file), but allows a link with a rooted target (\file) if and only if the filesystem root is absolute.
tianon commented

That's really close to treating symlinks as effectively plain-text files and ReadLink being the interface for reading them, which is an interpretation I'm in favor of that also happens to match how they were represented in filesystems historically (and in some cases still are: https://en.wikipedia.org/wiki/Symbolic_link#Storage_of_symbolic_links -- essentially text files with an extra bit to denote that they're special).

Saving validation for the time of dereferencing also makes it easier to compose separate instances of io/fs.FS together -- for example, in Debian packaging it's very common for the xxx.debian.tar.gz file to contain (dangling!) symlinks that reference files in the xxx.orig.tar.gz file (as canonically that .debian.tar.gz file is extracted as debian/ inside the extracted copy of the .orig.tar.gz file when building/inspecting the sources).

rsc commented

ReadLink cannot return an absolute link - those are forbidden in FS generally.

I agree that ReadLink should return the paths unchanged when it succeeds, but sometimes it should fail, for example if the path is absolute or contains too many .. elements. Your point about b/../../../c is interesting though. For the FS abstraction, we could simply disallow .. anywhere but the beginning of the path.

What we don't want is FS implementations returning symlinks obviously out of their trees, because then callers need some way to "narrow" the links.

Or we could decide not to do SymlinkFS after all. ๐Ÿ˜„

neild commented

I'm still a bit unclear on what the expected use cases for ReadLink are.

The most likely one I can think of is for generalized copy: os.CopyFS (or something like it) could create symlinks in its destination directory corresponding to ones in the source FS. I'm not sure how important it is to prevent symlinks escaping the root in this case.

A case which I don't think exists is opening the link target within the FS. If you want to open the link target, you can just open the link itself and let the FS handle resolution. Anything else is impossibly tricky to get right.

I like the idea of forbidding .. components anywhere other than the beginning of the path. Unix and Windows symlink resolution differs in subtle ways, but I think limiting .. components to the start of the path may avoid those differences. This means that we don't need to worry about whether FS symlinks are using Unix or Windows semantics.

If we do that, it's tempting to forbid . components. But retaining them is probably harmless, and if we forbid them then the FS implementation either needs to strip them or return errors when present.

So perhaps:

  • ReadLink may return paths containing .. components, but only at the start of the path.
  • ReadLink may not return absolute paths.
  • ReadLink may return paths containing . components.

For the curious, the differences between Unix and Windows (as best as I've been able to determine):

On Unix, a path is resolved step by step in the filesystem. "a/../b" opens a, and then .., and then b. If a is a symlink, the .. is resolved within the context of the symlink target.

On Windows, paths are lexically cleaned before resolution. "a/../b" is lexically transformed into "b", and will open b even if a is a symlink or doesn't exist at all.

On Windows, when a path contains a symlink, the symlink target is lexically combined with the remaining path and the result cleaned before resolution proceeds. So if a is a link to x/../y, then opening a/b will open the link a, create the path x/../y/b, lexically clean that path down to y/b, and then open y/b. x, despite appearing in the link target, doesn't get used at all.

(It is very possible that I've got some of the details for Windows wrong. This is all determined by experimentation; I haven't found documentation on this.)

I believe that if we limit ourselves to link targets that contain .. only at the start of the path, Unix and Windows behave the same, because lexical cleaning will never remove .. components from the start of a path.

rsc commented

@zombiezen and @tianon, if we adopt the restrictions in @neild's comment above this one, does that still work for your use case? Thanks.

tianon commented

Unfortunately not -- my use cases are related to reading/traversing symlinks from existing data in tarballs and Git repositories, especially to represent, transform, or traverse/parse the existing data (generated by other tools and processes) as-is, so frankly any restrictions on the contents of symlinks is going to be a blocker for me. Which I guess is fine, but it means I won't be able to use or implement this standard interface. ๐Ÿ™ˆ

For example, I do need to be able to read symlinks to absolute paths, and in the context of my application, I'll know to what root they're absolute (even though the io/fs.FS instance I'm reading them from may not be able to reasonably know), which honestly matches how symlinks work in real filesystems. ๐Ÿ™ˆ

My mental model of io/fs.FS instances is that each one is similar to a "mount" on a normal system, and that a hierarchy of them is thus totally reasonable usage. Using Sub is thus effectively half of a bind-mount (just the half representing the source, not the destination), and "dangling" symlinks are perfectly reasonable (their target is simply outside the FS, and whether to deny, allow, or transform that request in some way is the prerogative of the code doing something with the symlink contents).

In my view, we already have the code/restrictions for the use cases where it matters -- Open, Stat, etc, where the FS instance itself is expected to deal with/resolve symlinks and return the "final" contents transparently as if symlinks were not involved. So from that perspective, it feels really odd to have the "raw" interface that allows us to access/inspect the symlinks themselves also apply those restrictions.

rsc commented

The fact is that io/fs does not deal in absolute paths, by design. If you need symlinks involving absolute paths, then using io/fs is probably not for you.

I'm really starting to lean against adding SymlinkFS at all. It's just too complicated too fast.

Apologies for the delay in my response; I have been thinking over the implications of what's been said in the last few comments.

For my original use case, that set of restrictions would work. IIRC most of the symlinks I encountered were targeting files in the same directory as the link.

To add a new wrinkle, now that I've been using the Nix Archive Format in a number of projects, I am starting to sympathize with @tianon's comment about treating symlinks as plain text files and passing the content through as-is, especially when considering some of the subtleties that @neild mentioned. Being able to read absolute symlinks is really useful in some use cases that I've found since I originally filed the proposal, so this approach is beginning to grow on me.

@rsc I agree with the sentiment that if we're introducing more restrictions (as @neild reasonably proposes), then the complexity is not worth the weight of adding the ReadLink method. A plain-text passthrough is simpler than the existing implementation and would reduce the complexity, but it does come with a cost of potentially being hard to consume for applications. I would absolutely understand if that doesn't fly either, and we want to scrap this proposal.

That said, I think StatLink (having a Stat that is guaranteed to differentiate symlinks in a directory) provides value without significant downside. If there's agreement on that point, I could spin StatLink out into a much more targeted proposal.

(Thank you everyone for the thoughtful input. I really appreciate it and I'm glad we're catching these issues.)

rsc commented

It seems odd to have StatLink/Lstat (not sure which name is right) but not ReadLink.
If we do include ReadLink it seems like we have to say that it returns opaque data that should not be interpreted at all. That may be possible if you are writing 'tar' but otherwise it seems hard not to interpret. Any thoughts on the safety concerns @neild?

neild commented

I can't think of any safety concerns to returning opaque data from ReadLink.

If the link target is used to open a file within the FS, then this is no more or less safe than opening any other untrusted filename. FS.Open forbids absolute paths and paths containing .. or . links, so any interpretation of potentially-escaping link targets will need happen in user code, not the FS. But it also doesn't sound like anyone wants to open link targets within the FS that contains the link; the use cases described all involve doing some other operation, such as creating a new symlink with the given target. We don't consider it a vulnerability that tar can unpack an archive containing a link to /etc/passwd.

rsc commented

Okay, if we make ReadLink just have no constraints on the returned string, then we have:

type ReadLinkFS interface {
	FS

	// ReadLink returns the destination of the named symbolic link.
	ReadLink(name string) (string, error)

	// Lstat returns a FileInfo describing the file without following any symbolic links.
	// If there is an error, it should be of type *PathError.
	Lstat(name string) (FileInfo, error)
}

along with toplevel

func ReadLink(fsys FS, name string) (string, error)
func Lstat(fsys FS, name string) (FileInfo, error)

Again, there are no guarantees or requirements about what ReadLink returns.

Do I have that right? Does that sound reasonable?

@rsc Replace Lstat with StatLink as originally proposed by @dsnet and that matches my understanding. If we're all good, I'm happy to send out a new CL for that. (Should be substantially simpler than the previous iterations.)

tianon commented

IMO the name StatLink implies it only works on symlinks, when the intent is really "Stat but never following symlinks" which is an operation very commonly spelled elsewhere as "lstat"

Lstat is the usual name for this operation and, for example, we already have os.Lstat. What is the argument for naming it StatLink?

mvdan commented

In #49580 (comment) and #49580 (comment) myself and Joe lean towards avoiding C-inspired short names like Lstat, which os.Lstat did borrow, but we don't have to follow. ReadLink already splits away from os.Readlink for what it's worth, as well as ReadDir versus os.Readdir, so Joe and I assumed there was a precedent to better "idiomatic Go" names in io/fs than the older os.

I would still like a better name than Lstat, though I admit that StatLink sounds like it only works on links. Perhaps LinkStat is a bit closer to lstat and doesn't give the impression that it only works on link arguments.

I'm a bit skeptical of StatLink (or LinkStat). ReadLink/Readlink and ReadDir/Readdir are clearly the same operations, so I don't think that's a particularly sound precedent. Lstat, for better or worse, is the UNIX term for this operation and the name we use elsewhere (I'd honestly never thought about what "lstat" was short for before now).

dsnet commented

Lstat, for better or worse, is the UNIX term for this operation

Are we perhaps overly making our naming UNIX-centric? Symbolic links are not unique to UNIX. They're supported on Windows platforms as well (albeit not used very often). The progeny for my earlier comment was because I was implementing Microsoft's SMB2 protocol in Go and the issue of symbolic links came up. In my opinion, we should aim to be as platform agnostic in io/fs namings.

neild commented

This is the very definition of a bikeshed, but we do need to decide on a color so:

We've got precedent for Lstat. It's unambiguous and commonly used. Unlike ReadDir, we're not trying to change the signature of an existing method (Readdir returns []FileInfo, and ReadDir returns []DirEntry). We've got plenty of Unix-isms in the file APIs already (such as file permission bits), and Windows (AFAIK) has symbolic links primarily for POSIX compatibility anyway. I think we should just stick with Lstat.

Have all remaining concerns about this proposal been addressed?

The proposal is:

package fs

type ReadLinkFS interface {
        FS

        // ReadLink returns the destination of the named symbolic link.
        ReadLink(name string) (string, error)

        // Lstat returns a FileInfo describing the file without following any symbolic links.
        // If there is an error, it should be of type *PathError.
        Lstat(name string) (FileInfo, error)
}

along with toplevel

func ReadLink(fsys FS, name string) (string, error)
func Lstat(fsys FS, name string) (FileInfo, error)

There are no guarantees or requirements about what ReadLink returns.

(This time for sure!)

Sorry for being late to the party, I've just returned from a long paternity leave. My only concern is the interface methods docs, IMO they are too Unix specific. Windows supports other types of links that are not symbolic links. For example, on Windows, os.ReadLink supports symbolic links and mount points, and os.Lstat follows any kind of link (also known as reparse tag name surrogate).

My suggestion would be to remove the word symbolic from the docs.

mvdan commented

Swapping "symbolic link" for "link" could make the docs a bit ambiguous with hard links though: https://pkg.go.dev/os#Link

Does Windows use any term for ReadLink and Lstat other than "link"? The OS in general does seem to have the notion of "hard links" as well as "symbolic links", which this proposal is mainly aimed at: https://learn.microsoft.com/en-us/windows/win32/fileio/hard-links-and-junctions

Does Windows use any term for ReadLink and Lstat other than "link"?

Could we use "soft link"? It is not a well established term on Windows, but it appears in Microsoft's docs and it should work well on Unix too. We could mention symlinks, and then clarify that other types of soft links are also supported, such as Windows mount points.

@aclements SGTM. I can draft a new version of the CL within the next week or so.

I have a new patchset of https://go.dev/cl/385534 ready for review that incorporates the discussed changes.

I'm happy to use the term "soft link". It's a bit tricky to document in detail because this is an interface. It might not be backed by any sort of OS file system at all, so the exact meaning is kind of left up to each implementation of the interface. I'm open to suggestions, though.

Based on the discussion above, this proposal seems like a likely accept.

The proposal is:

package fs

type ReadLinkFS interface {
        FS

        // ReadLink returns the destination of the named soft link.
        ReadLink(name string) (string, error)

        // Lstat returns a FileInfo describing the file without following any soft links.
        // If there is an error, it should be of type *PathError.
        Lstat(name string) (FileInfo, error)
}

along with toplevel

func ReadLink(fsys FS, name string) (string, error)
func Lstat(fsys FS, name string) (FileInfo, error)

There are no guarantees or requirements about what ReadLink returns.

We can continue to hash out what the exact documentation should look like to cover both Unix and Windows.

(This time for sure!)

On Windows, can the current interface distinguish between Junction and Symlink, which are two different types?
See also https://www.2brightsparks.com/resources/articles/ntfs-hard-links-junctions-and-symbolic-links.html

The return value of ReadLink lacks a rigorous definition. According to this wiki page from ntfs-3g, Junctions may also contain volume id. If the target volume does not exist, what should ReadLink return? A failure or NT Object Manager path beginning with \\?\?

mvdan commented

@zhangyoufu It seems to me like the same questions apply to the documented behavior for https://pkg.go.dev/os#Readlink. Does os.Readlink behave OK for you in those scenarios?

@mvdan Yes. Current behavior of os.Readlink meet my expectation under Windows.

junction (mountpoint) => \\?\Volume{c54466ef-0000-0000-0000-100000000000}\
junction => C:\junction-target
symlinkd => symlinkd-target
symlink => symlink-target

Go standard library did a good job handling/testing all these cases.

mvdan commented

@zhangyoufu given that os.Readlink doesn't document any specific Windows behavior, and that it already behaves the way that works for you, I suppose we can do the same for fs.ReadLinkFS.ReadLink. I'm not sure that anything in particular needs to change in the proposed API.

I'm not sure that anything in particular needs to change in the proposed API.

Agree. io/fs interfaces and documentation should avoid mentioning concret implementations details and special cases. If the os implementation needs more documentation (which I think it does), then it should be added to os.DirFS.

During my test on Windows, FileInfo returned by Lstat looks strange:

  • we can Readlink() on junction, while FileInfo.Mode() returns ModeIrregular rather than ModeSymlink
  • junction (incl. volume mountpoint) and symlinkd has FILE_ATTRIBUTE_DIRECTORY from win32 API, but FileInfo.IsDir() simply returns false

Here is my test result:

mountpoint => \\?\Volume{c54466ef-0000-0000-0000-100000000000}\, modeSymlink=false, modeIrregular=true, isDir=false, fileAttributeDir=true
junction => C:\junction-target, modeSymlink=false, modeIrregular=true, isDir=false, fileAttributeDir=true
symlinkd => X:\symlinkd-target, modeSymlink=true, modeIrregular=false, isDir=false, fileAttributeDir=true
symlink => X:\symlink-target, modeSymlink=true, modeIrregular=false, isDir=false, fileAttributeDir=false

To make io/fs interface portable, robust and implementation-neutral, it may be necessary to clarify:

  • If Readlink() succeed on a file, do we require ModeSymlink on its FileInfo.Mode?
  • Is it allowed to have both ModeDirectory and ModeSymlink?
  • Should we expose a race-free way to perform both ReadLink and Lstat? (on Linux it's possible to open using O_PATH | O_NOFOLLOW, then calls fstat and readlinkat)

Test Code:

test.bat
@echo off
cd /D "%~dp0"

for /f "tokens=*" %%i in ('mountvol %SystemDrive% /L') do set VolumeName=%%i

REM prepare
md mountpoint
mountvol mountpoint %VolumeName%
mklink /J junction %SystemDrive%\junction-target
mklink /D symlinkd X:\symlinkd-target
mklink symlink X:\symlink-target

pause
go run main.go
pause

REM cleanup
mountvol mountpoint /D
rd mountpoint
rd junction
rd symlinkd
del symlink
main.go
package main

import (
    "fmt"
    "io/fs"
    "os"
    "reflect"
    "syscall"
)

func main() {
    for _, source := range []string{"mountpoint", "junction", "symlinkd", "symlink"} {
        target, err := os.Readlink(source)
        if err != nil {
            fmt.Println(err)
            continue
        }
        fi, err := os.Lstat(source)
        if err != nil {
            fmt.Println(err)
            continue
        }
        modeSymlink := fi.Mode() & fs.ModeSymlink != 0
        modeIrregular := fi.Mode() & fs.ModeIrregular != 0
        fileAttributeDir := reflect.ValueOf(fi).Elem().FieldByName("FileAttributes").Uint() & syscall.FILE_ATTRIBUTE_DIRECTORY != 0
        fmt.Printf("%s => %s, modeSymlink=%v, modeIrregular=%v, isDir=%v, fileAttributeDir=%v\n", source, target, modeSymlink, modeIrregular, fi.IsDir(), fileAttributeDir)
    }
}

we can Readlink() on junction, while FileInfo.Mode() returns ModeIrregular rather than ModeSymlink

This is the expected behavior since Go 1.23 (see #61893).

junction (incl. volume mountpoint) and symlinkd has FILE_ATTRIBUTE_DIRECTORY from win32 API, but FileInfo.IsDir() simply returns false

This is also the expected behavior. I don't know the exact reason to not set os.ModeDir for symbolic links and mount points, but it has the nice benefit of avoiding unintentionally following a mount point when traversing a directory, which can lead to infinite recursion.

If Readlink() succeed on a file, do we require ModeSymlink on its FileInfo.Mode?

I wouldn't limit ReadLinkFS.Readlink to just symlinks. Some implementations might support other types of "soft links" (e.g. Windows mount points).

Should we expose a race-free way to perform both ReadLink and Lstat? (on Linux it's possible to open using O_PATH | O_NOFOLLOW, then calls fstat and readlinkat)

This is very related to #67002. It probably deserves a separate proposal.

I wouldn't limit ReadLinkFS.Readlink to just symlinks. Some implementations might support other types of "soft links" (e.g. Windows mount points).

IEEE Std 1003.1 defined readlink(2), the API, as read the contents of a symbolic link, and readlink(1), the CLI utility, as display the contents of a symbolic link. It also defined the term link (in context of file hierarchy), is either a hard link or a symbolic link.

I understand that Go may not follow aforementioned spec/standard. But I would be surprised if Go define Readlink to also work on links that is neither hardlink, nor symlink.

IMHO, Windows junctions (incl. mount points) are also symlinks.

I think none of this affects the ReadLinkFS interface directly, though it certainly affects implementations. It seems like we should improve the documentation on os.Readlink, but that can be done separately.

No change in consensus, so accepted. ๐ŸŽ‰
This issue now tracks the work of implementing the proposal.

The proposal is:

package fs

type ReadLinkFS interface {
        FS

        // ReadLink returns the destination of the named soft link.
        ReadLink(name string) (string, error)

        // Lstat returns a FileInfo describing the file without following any soft links.
        // If there is an error, it should be of type *PathError.
        Lstat(name string) (FileInfo, error)
}

along with toplevel

func ReadLink(fsys FS, name string) (string, error)
func Lstat(fsys FS, name string) (FileInfo, error)

There are no guarantees or requirements about what ReadLink returns.

We can continue to hash out what the exact documentation should look like to cover both Unix and Windows.

Code freeze is today and CL 385534 hasn't been properly reviewed yet. Any chance to put this into the fast line so it can land in Go 1.24?

mvdan commented

One last minute comment came in before the submit happened, which only affects fstest and looks pretty easy to fix. I assume we're still OK for 1.24?

@golang/release I'd like to request a freeze exception. CL 385534 is ready to be merged once/if the exception is approved.

We considered this freeze exception in a release meeting today. This seems to be a new API that would be safe to land in the next upcoming major Go release, Go 1.25, so that if there are unexpected issues or findings, there's more time available to address them. I see the CL author commented being okay with that.

It's unfortunate that it was close to making it into into Go 1.24 but it didn't quite make it. We are trying to unblock RC 1 so that it can be released next week per https://go.dev/s/release timeline, and there being 14 issues blocking RC 1 today (see query; much of it is documentation, though) meant we had less room to make freeze exceptions.

Since this seems to be otherwise ready, I'll move this to Go1.25 milestone and add early-in-cycle label so this can be submitted soon after tree reopening (tracked in #70525).