-
Notifications
You must be signed in to change notification settings - Fork 24
Read the destination of named symbolic link #157
Conversation
FileInfo.IsDir() returns false if it is symlink. But package is sometime defined under symlink. So should follow the symlink.
Codecov Report
@@ Coverage Diff @@
## master #157 +/- ##
==========================================
- Coverage 78.88% 78.84% -0.05%
==========================================
Files 24 23 -1
Lines 3709 3668 -41
==========================================
- Hits 2926 2892 -34
+ Misses 582 573 -9
- Partials 201 203 +2
Continue to review full report at Codecov.
|
This is a tricky issue. Go's toolchain in general is touchy about symlinks; I want to avoid introducing new, complex behavior without a clear understanding of the implications. This change would permit symlinks in contexts that the go toolchain might (?) not. So, it's not a firm "no," but rather a "I need more information about exactly how the go toolchain currently works with symlinks before I can decide." |
Thank you for your comment.
Sorry, I'm not sure that go's toolchain permit symlinks or not. I used https://github.com/douban/libmc. The repository includes symlink that is named When I execute $ cat main.go
package main
import (
"fmt"
"github.com/douban/libmc/golibmc"
)
func main() {
fmt.Println(golibmc.HashMD5)
}
$ dep init
ouchie, solve error: No versions of github.com/douban/libmc met constraints:
master: Could not introduce github.com/douban/libmc@master, as its subpackage github.com/douban/libmc/golibmc is missing. (Package is required by (root).)
v1.1.0: Could not introduce github.com/douban/libmc@v1.1.0, as its subpackage github.com/douban/libmc/golibmc is missing. (Package is required by (root).)
v1.0.1: Could not introduce github.com/douban/libmc@v1.0.1, as it is not allowed by constraint master from project github.com/ReSTARTR/go-dep.
v1.0.0: Could not introduce github.com/douban/libmc@v1.0.0, as it is not allowed by constraint master from project github.com/ReSTARTR/go-dep.
v0.5.6: Could not introduce github.com/douban/libmc@v0.5.6, as it is not allowed by constraint master from project github.com/ReSTARTR/go-dep.
v0.5.5: Could not introduce github.com/douban/libmc@v0.5.5, as it is not allowed by constraint master from project github.com/ReSTARTR/go-dep.
v0.5.4: Could not introduce github.com/douban/libmc@v0.5.4, as it is not allowed by constraint master from project github.com/ReSTARTR/go-dep.
v0.5.3: Could not introduce github.com/douban/libmc@v0.5.3, as it is not allowed by constraint master from project github.com/ReSTARTR/go-dep.
v0.5.2: Could not introduce github.com/douban/libmc@v0.5.2, as it is not allowed by constraint master from project github.com/ReSTARTR/go-dep.
v0.5.1: Could not introduce github.com/douban/libmc@v0.5.1, as it is not allowed by constraint master from project github.com/ReSTARTR/go-dep.
v0.5.0: Could not introduce github.com/douban/libmc@v0.5.0, as it is not allowed by constraint master from project github.com/ReSTARTR/go-dep.
v0.5.0: Could not introduce github.com/douban/libmc@v0.5.0, as it is not allowed by constraint master from project github.com/ReSTARTR/go-dep.
master: Could not introduce github.com/douban/libmc@master, as its subpackage github.com/douban/libmc/golibmc is missing. (Package is required by (root).)
stable: Could not introduce github.com/douban/libmc@stable, as it is not allowed by constraint master from project github.com/ReSTARTR/go-dep.No versions of github.com/douban/libmc met constraints:
master: Could not introduce github.com/douban/libmc@master, as its subpackage github.com/douban/libmc/golibmc is missing. (Package is required by (root).)
v1.1.0: Could not introduce github.com/douban/libmc@v1.1.0, as its subpackage github.com/douban/libmc/golibmc is missing. (Package is required by (root).)
v1.0.1: Could not introduce github.com/douban/libmc@v1.0.1, as it is not allowed by constraint master from project github.com/ReSTARTR/go-dep.
v1.0.0: Could not introduce github.com/douban/libmc@v1.0.0, as it is not allowed by constraint master from project github.com/ReSTARTR/go-dep.
v0.5.6: Could not introduce github.com/douban/libmc@v0.5.6, as it is not allowed by constraint master from project github.com/ReSTARTR/go-dep.
v0.5.5: Could not introduce github.com/douban/libmc@v0.5.5, as it is not allowed by constraint master from project github.com/ReSTARTR/go-dep.
v0.5.4: Could not introduce github.com/douban/libmc@v0.5.4, as it is not allowed by constraint master from project github.com/ReSTARTR/go-dep.
v0.5.3: Could not introduce github.com/douban/libmc@v0.5.3, as it is not allowed by constraint master from project github.com/ReSTARTR/go-dep.
v0.5.2: Could not introduce github.com/douban/libmc@v0.5.2, as it is not allowed by constraint master from project github.com/ReSTARTR/go-dep.
v0.5.1: Could not introduce github.com/douban/libmc@v0.5.1, as it is not allowed by constraint master from project github.com/ReSTARTR/go-dep.
v0.5.0: Could not introduce github.com/douban/libmc@v0.5.0, as it is not allowed by constraint master from project github.com/ReSTARTR/go-dep.
v0.5.0: Could not introduce github.com/douban/libmc@v0.5.0, as it is not allowed by constraint master from project github.com/ReSTARTR/go-dep.
master: Could not introduce github.com/douban/libmc@master, as its subpackage github.com/douban/libmc/golibmc is missing. (Package is required by (root).)
stable: Could not introduce github.com/douban/libmc@stable, as it is not allowed by constraint master from project github.com/ReSTARTR/go-dep. So, if gps can follow the symlink, this problem will be fixed. |
OK, a compromise - if we can verify that the symlink doesn't escape the root of the tree, then we can follow it. If not, it should be skipped. Let's do this in two phases. Initially, in this PR, let's have it be:
In the longer term, though, we can try to accept some absolute symlinks, assuming we can demonstrate them to be children of
I'd rather not introduce the possibility of failures, errors, or possibly even security issues until I better understand the implications. (There's a reason Rob Pike hates symlinks.) Does this seem sane? |
Oh, also - if we add this, then it definitely needs full test coverage. There's a set of fixtures in Thanks! |
Okay, I got it. I'll try to fix this changes and add some test cases 👍 |
_testdata/src/symlinks/symlinks.go
Outdated
package symlinks | ||
|
||
import ( | ||
_ "github.com/sdboyer/gps" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a blank import should be only in a main or test package, or have a comment justifying it
_testdata/src/symlinks/pkg/gopkg.go
Outdated
package gopkg | ||
|
||
const ( | ||
Foo = "foo" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exported const Foo should have comment (or a comment on this block) or be unexported
_testdata/src/symlinks/pkg/gopkg.go
Outdated
package gopkg | ||
|
||
const ( | ||
Foo = "foo" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exported const Foo should have comment (or a comment on this block) or be unexported
I have fixed some code, and added test cases. I have an another issue:cry: How about this idea? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still checking a couple of the behaviors, because things don't seem quite right - but in the meantime, a couple of notes.
analysis_test.go
Outdated
@@ -898,6 +899,58 @@ func TestListPackages(t *testing.T) { | |||
}, | |||
}, | |||
} | |||
if runtime.GOOS != "windows" { | |||
type t struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of redeclaring the anonymous struct here as a typed one, just move this declaration up to the top of the func and change it to table := map[string]t{
analysis.go
Outdated
// 2. Relative symlinks pointing to somewhere outside of the root (via ..) should also be skipped. | ||
if !fi.IsDir() && fi.Mode()&os.ModeSymlink != 0 { | ||
n := fi.Name() | ||
if strings.HasPrefix(n, string(filepath.Separator)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use filepath.IsAbs()
here; this won't work as expected on windows. (Compare to the windows impl of IsAbs()
])
analysis.go
Outdated
// 1. All absolute symlinks are disqualified; if one is encountered, it should be skipped. | ||
// 2. Relative symlinks pointing to somewhere outside of the root (via ..) should also be skipped. | ||
if !fi.IsDir() && fi.Mode()&os.ModeSymlink != 0 { | ||
n := fi.Name() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is right - this just returns the basename of the path (the final element).
I have changed some lines on your comments. |
Great, that's looking a lot better. Let's add two more test cases, then I'll merge:
|
That failure is erroneous; for some reason it's tied to CircleCI deciding to build under your account, rather than mine. I don't know why it does that, but it's not the first time I've seen it. I need to switch to Travis. Just as I was about to hit the merge buttton, I realized there's another, much more annoying case: if the symlink points to a valid directory, that directory might also have children. It would be very odd to include the contents of the referenced directory, but not the directory itself. Unfortunately, this is a trickier fix. I think our only option is just to abandon the use of Does that make sense? |
$ tree _testdata/src/symlinks
_testdata/src/symlinks
├── broken -> nodest
├── foo
│ ├── bar -> ../../pkg
│ └── foo.go
├── foobar -> foo/bar
├── gopkg -> pkg
├── pkg
│ ├── bar -> bar # <- Is it about this?
│ └── gopkg.go
└── symlinks.go It's my mistake ;-( I'll fix it.
I agree with it. So I'm going to change like this. What about do you think of this? // walkDir wraps filepath.Walk. Analyze file type before calling walkFn.
func walkDir(fileRoot string, walkFn filepath.WalkFunc) error {
return filepath.Walk(fileRoot, func(wp string, fi os.FileInfo, err error) error {
// analyze the file type
return walkFn(wp, fi, err)
}
} |
No problem - I missed it too, until I actually thought it through just before hitting the merge button!
Yes, though it would be a better example if
I don't think that's going to be quite enough. We need a full replacement for |
Mmm...it is a big change, isn't it? First step is like this. package gps
func ListPackages(fileRoot, importRoot string) (PackageTree, error) {
// ...
err = filepath.Walk(fileRoot, func(wp string, fi os.FileInfo, err error) error {
fi, err = stat(wp)
}
// ...
}
func stat(path string) (os.FileInfo, error) {
// promote the lines I've created until now
} And, we try to replace |
I tried to promote the lines that reads symlink into I think that we should not replace |
Ugh, sorry, I totally missed responding to your comment from three weeks ago. I read it, forgot to respond, but then it was out of my notifications so I didn't check back. But, the new changes you pushed look good. I'm hesitant about merging this without addressing the bigger problem. gps is still in prerelease, but I'd be uncomfortable making a release with this problem only half-solved. And I have a bunch of other fixes I'm trying to fire out right now, then rapidly roll releases - having this only partially done would make that difficult. I don't actually think the change is that bad, though. We just need to implement our own version of That said, I realize that is a larger change. If that's not something you're comfortable doing, that's no problem - we can merge as-is, and I'll make the necessary changes in a followup. |
Thank you for checking! I'm afraid that I don't fix that problem. But I thought that it might cause some conflicts with another changes ;-( So I'd be grad you to merge and make another changes. |
Yep, no problem - I'll merge, then take care of the rest. |
Read the destination of named symbolic link
FileInfo.IsDir()
returns false if it is symlink.But package is sometime defined under symlink.
So should follow the symlink.