Resolve context root on context.Walk when root is symlink #113
Conversation
Signed-off-by: Darren Stahl <darst@microsoft.com>
Signed-off-by: Darren Stahl <darst@microsoft.com>
| } | ||
| } | ||
| return c.pathDriver.Walk(root, func(p string, fi os.FileInfo, err error) error { | ||
| contained, err := c.containWithRoot(p, root) |
There was a problem hiding this comment.
Is the above logic duplicated in container with root?
There was a problem hiding this comment.
It was, but shouldn't have been. Removed the duplicated logic. containWithRoot now uses the root as given with no modifications.
|
Are you opening up an upstream change to fix the |
Signed-off-by: Darren Stahl <darst@microsoft.com>
|
Yes, the changes will be submitted to Go's Syscall package. The current state of https://github.com/golang/sys syscall package is not up to date with Go master (which contains the patch that broke this), so I'm not sure if this change will land in sys or syscall or both. It is intended to be upstreamed though, and this is just a temporary fork. |
|
LGTM |
1 similar comment
|
LGTM |
@darstahl can you please point out to those? I did a quick look and can't seem to find any. |
|
Found myself re-reading this today :) If anyone knows if the Windows' readlink bug is fixed in golang, please share the news |
|
Guess what, I found the issue (golang/go#30463) and the fix (https://go-review.googlesource.com/c/go/+/164201, very similar to what we have in here). Will follow up with the clean patch |
This reverts the part of commit 8100e75 (PR containerd#113) that added the fork of Readlink() for Windows. At that time the fork was done to work around the bug in golang's implementation of os.Readlink() for Windows. The above bug was never reported upstream, but fortunately it was independently found, reported [1], and fixed [2]. The fix made its way into go-1.13 (there is no mention of that in release notes, so I checked it in git history). [1] golang/go#30463 [2] https://go-review.googlesource.com/c/go/+/164201 Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This reverts the part of commit 8100e75 (PR containerd#113) that added the fork of Readlink() for Windows. At that time the fork was done to work around the bug in golang's implementation of os.Readlink() for Windows. The above bug was never reported upstream, but fortunately it was independently found, reported [1], and fixed [2]. The fix made its way into go-1.13 (there is no mention of that in release notes, so I checked it in git history). [1] golang/go#30463 [2] https://go-review.googlesource.com/c/go/+/164201 Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
When the root of a context is a symlink, Walk does not walk the contents of the directory as expected due to the implementation in Golang. This expands the root of a context to the target of the link prior to the walk in order to correctly walk the contents of the context.
Readlink was forked on Windows as there is a bug in Golang that prevents Volume GUID paths of the form
\??\Volumefrom being a valid target of a link. This type of path is necessary for mounted VHDs of Windows container filesystems.Signed-off-by: Darren Stahl darst@microsoft.com