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.

Sign up for GitHub

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

Fix the way to compute the target path relative to the symlink #245

Merged
merged 2 commits into from
Nov 20, 2020
Merged

Fix the way to compute the target path relative to the symlink #245

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
826d42d
Fix how target path relative to the symlink is computed
k-ye Nov 20, 2020
e46aa81
more tests
k-ye Nov 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 16 additions & 10 deletions go/pkg/client/tree.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,18 +87,22 @@ func shouldIgnore(inp string, t command.InputType, excl []*command.InputExclusio
return false
}

// getTargetRelPath returns the part of target that is relative to execRoot,
// iff target is under execRoot. Otherwise it returns an error.
func getTargetRelPath(execRoot, target string) (string, error) {
// getTargetRelPath returns the part of the target relative to the symlink's
// directory, iff the target is under execRoot. Otherwise it returns an error.
func getTargetRelPath(execRoot, symlinkNormDir, target string) (string, error) {
symlinkAbsDir := filepath.Clean(filepath.Join(execRoot, symlinkNormDir))
if !filepath.IsAbs(target) {
target = filepath.Clean(filepath.Join(execRoot, target))
target = filepath.Clean(filepath.Join(symlinkAbsDir, target))
}
return getRelPath(execRoot, target)
if _, err := getRelPath(execRoot, target); err != nil {
return "", err
}
return filepath.Rel(symlinkAbsDir, target)
}

// preprocessSymlink returns two things: if the routine should continue, and if
// there is an error to be reported back.
func preprocessSymlink(execRoot, symlink string, meta *filemetadata.SymlinkMetadata, opts *TreeSymlinkOpts) (bool, error) {
func preprocessSymlink(execRoot, symlinkNormDir string, meta *filemetadata.SymlinkMetadata, opts *TreeSymlinkOpts) (bool, error) {
if meta.IsDangling {
// For now, we do not treat a dangling symlink as an error. In the case
// where the symlink is not preserved (i.e. needs to be converted to a
Expand All @@ -111,7 +115,7 @@ func preprocessSymlink(execRoot, symlink string, meta *filemetadata.SymlinkMetad
return true, nil
}

if _, err := getTargetRelPath(execRoot, meta.Target); err != nil {
if _, err := getTargetRelPath(execRoot, symlinkNormDir, meta.Target); err != nil {
return false, err
}
return true, nil
Expand All @@ -130,8 +134,10 @@ func loadFiles(execRoot string, excl []*command.InputExclusion, path string, fs
}
meta := cache.Get(absPath)
isSymlink := meta.Symlink != nil
symlinkNormDir := ""
if isSymlink {
cont, err := preprocessSymlink(execRoot, path, meta.Symlink, opts)
symlinkNormDir = filepath.Dir(normPath)
cont, err := preprocessSymlink(execRoot, symlinkNormDir, meta.Symlink, opts)
if err != nil {
return err
}
Expand Down Expand Up @@ -161,7 +167,7 @@ func loadFiles(execRoot string, excl []*command.InputExclusion, path string, fs
}
return nil
} else if t == command.SymlinkInputType {
relTarget, err := getTargetRelPath(execRoot, meta.Symlink.Target)
relTarget, err := getTargetRelPath(execRoot, symlinkNormDir, meta.Symlink.Target)
if err != nil {
return err
}
Expand All @@ -175,7 +181,7 @@ func loadFiles(execRoot string, excl []*command.InputExclusion, path string, fs
if meta.Symlink.IsDangling || !opts.FollowsTarget {
return nil
}
return loadFiles(execRoot, excl, relTarget, fs, chunkSize, cache, opts)
return loadFiles(execRoot, excl, filepath.Clean(filepath.Join(symlinkNormDir, relTarget)), fs, chunkSize, cache, opts)
}
// Directory
files, err := ioutil.ReadDir(absPath)
Expand Down
65 changes: 34 additions & 31 deletions go/pkg/client/tree_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,15 @@ type inputPath struct {
func construct(dir string, ips []*inputPath) error {
for _, ip := range ips {
path := filepath.Join(dir, ip.path)
if ip.emptyDir {
if err := os.MkdirAll(path, 0777); err != nil {
return err
}
continue
}
if err := os.MkdirAll(filepath.Dir(path), 0777); err != nil {
return err
}
if ip.isSymlink {
target := ip.symlinkTarget
if ip.isAbsolute {
Expand All @@ -67,16 +76,7 @@ func construct(dir string, ips []*inputPath) error {
}
continue
}
if ip.emptyDir {
if err := os.MkdirAll(path, 0777); err != nil {
return err
}
continue
}
// Regular file.
if err := os.MkdirAll(filepath.Dir(path), 0777); err != nil {
return err
}
perm := os.FileMode(0666)
if ip.isExecutable {
perm = os.FileMode(0777)
Expand Down Expand Up @@ -345,6 +345,11 @@ func TestComputeMerkleTreeEmptyStructureVirtualInputs(t *testing.T) {
}

func TestComputeMerkleTree(t *testing.T) {
foobarSymDir := &repb.Directory{Symlinks: []*repb.SymlinkNode{{Name: "foobarSymDir", Target: "../foobarDir"}}}
foobarSymDirBlob := mustMarshal(foobarSymDir)
foobarSymDirDg := digest.NewFromBlob(foobarSymDirBlob)
foobarSymDirDgPb := foobarSymDirDg.ToProto()

tests := []struct {
desc string
input []*inputPath
Expand Down Expand Up @@ -692,28 +697,27 @@ func TestComputeMerkleTree(t *testing.T) {
input: []*inputPath{
{path: "foobarDir/foo", fileContents: fooBlob, isExecutable: true},
{path: "foobarDir/bar", fileContents: barBlob},
{path: "foobarSymDir", isSymlink: true, isAbsolute: true, symlinkTarget: "foobarDir"},
{path: "base/foobarSymDir", isSymlink: true, isAbsolute: true, symlinkTarget: "foobarDir"},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might seem like it should be ../foobarDir, but because isAbsolute is true, the actual target it generates is "${execRoot}/foobarDir".

},
spec: &command.InputSpec{
// The symlink target will be traversed recursively.
Inputs: []string{"foobarSymDir"},
Inputs: []string{"base/foobarSymDir"},
},
rootDir: &repb.Directory{
Directories: []*repb.DirectoryNode{{Name: "foobarDir", Digest: foobarDirDgPb}},
Symlinks: []*repb.SymlinkNode{{Name: "foobarSymDir", Target: "foobarDir"}},
Directories: []*repb.DirectoryNode{{Name: "base", Digest: foobarSymDirDgPb}, {Name: "foobarDir", Digest: foobarDirDgPb}},
},
additionalBlobs: [][]byte{fooBlob, barBlob, foobarDirBlob},
additionalBlobs: [][]byte{fooBlob, barBlob, foobarDirBlob, foobarSymDirBlob},
wantCacheCalls: map[string]int{
"foobarDir": 1,
"foobarDir/foo": 1,
"foobarDir/bar": 1,
"foobarSymDir": 1,
"foobarDir": 1,
"foobarDir/foo": 1,
"foobarDir/bar": 1,
"base/foobarSymDir": 1,
},
wantStats: &client.TreeStats{
InputDirectories: 2,
InputDirectories: 3,
InputFiles: 2,
InputSymlinks: 1,
TotalInputBytes: fooDg.Size + barDg.Size + foobarDirDg.Size,
TotalInputBytes: fooDg.Size + barDg.Size + foobarDirDg.Size + foobarSymDirDg.Size,
},
treeOpts: &client.TreeSymlinkOpts{
Preserved: true,
Expand All @@ -725,28 +729,27 @@ func TestComputeMerkleTree(t *testing.T) {
input: []*inputPath{
{path: "foobarDir/foo", fileContents: fooBlob, isExecutable: true},
{path: "foobarDir/bar", fileContents: barBlob},
{path: "foobarSymDir", isSymlink: true, symlinkTarget: "foobarDir"},
{path: "base/foobarSymDir", isSymlink: true, symlinkTarget: "../foobarDir"},
},
spec: &command.InputSpec{
// The symlink target will be traversed recursively.
Inputs: []string{"foobarSymDir"},
Inputs: []string{"base/foobarSymDir"},
},
rootDir: &repb.Directory{
Directories: []*repb.DirectoryNode{{Name: "foobarDir", Digest: foobarDirDgPb}},
Symlinks: []*repb.SymlinkNode{{Name: "foobarSymDir", Target: "foobarDir"}},
Directories: []*repb.DirectoryNode{{Name: "base", Digest: foobarSymDirDgPb}, {Name: "foobarDir", Digest: foobarDirDgPb}},
},
additionalBlobs: [][]byte{fooBlob, barBlob, foobarDirBlob},
additionalBlobs: [][]byte{fooBlob, barBlob, foobarDirBlob, foobarSymDirBlob},
wantCacheCalls: map[string]int{
"foobarDir": 1,
"foobarDir/foo": 1,
"foobarDir/bar": 1,
"foobarSymDir": 1,
"foobarDir": 1,
"foobarDir/foo": 1,
"foobarDir/bar": 1,
"base/foobarSymDir": 1,
},
wantStats: &client.TreeStats{
InputDirectories: 2,
InputDirectories: 3,
InputFiles: 2,
InputSymlinks: 1,
TotalInputBytes: fooDg.Size + barDg.Size + foobarDirDg.Size,
TotalInputBytes: fooDg.Size + barDg.Size + foobarDirDg.Size + foobarSymDirDg.Size,
},
treeOpts: &client.TreeSymlinkOpts{
Preserved: true,
Expand Down
65 changes: 45 additions & 20 deletions go/pkg/client/tree_whitebox_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,43 +3,68 @@ package client
import "testing"

func TestGetTargetRelPath(t *testing.T) {
execRoot := "/execRoot/dir"
execRoot := "/execRoot"
defaultSymlinkNormDir := "symDir"
tests := []struct {
desc string
target string
wantErr bool
relTarget string
desc string
symlinkNormDir string
target string
wantErr bool
relTarget string
}{
{
desc: "basic",
target: "foo",
relTarget: "foo",
desc: "basic",
symlinkNormDir: defaultSymlinkNormDir,
target: "foo",
relTarget: "foo",
},
{
desc: "there and back again",
target: "../dir/sub/foo",
relTarget: "sub/foo",
desc: "relative target path under exec root",
symlinkNormDir: defaultSymlinkNormDir,
// /execRoot/symDir/../dir/foo ==> /execRoot/dir/foo
target: "../dir/foo",
relTarget: "../dir/foo",
},
{
desc: "relative target path escaping exec root",
target: "../foo",
desc: "relative target path escaping exec root",
symlinkNormDir: defaultSymlinkNormDir,
// /execRoot/symDir/../../foo ==> /foo
target: "../../foo",
wantErr: true,
},
{
desc: "absolute target path under exec root",
target: execRoot + "/sub/foo",
relTarget: "sub/foo",
desc: "deeper relative target path",
symlinkNormDir: "base/sub",
// /execRoot/base/sub/../../foo ==> /execRoot/foo
target: "../../foo",
relTarget: "../../foo",
},
{
desc: "absolute target path escaping exec root",
target: "/another/dir/foo",
wantErr: true,
desc: "absolute target path under exec root",
symlinkNormDir: "base",
target: execRoot + "/base/foo",
relTarget: "foo",
},
{
desc: "abs target to rel target",
symlinkNormDir: "base/sub1/sub2",
target: execRoot + "/dir/foo",
// symlinkAbsDir: /execRoot/base/sub1/sub2
// targetAbs: /execRoot/dir/foo
// target rel to symlink: ../../../dir/foo
relTarget: "../../../dir/foo",
},
{
desc: "absolute target path escaping exec root",
symlinkNormDir: defaultSymlinkNormDir,
target: "/another/dir/foo",
wantErr: true,
},
}

for _, tc := range tests {
t.Run(tc.desc, func(t *testing.T) {
res, err := getTargetRelPath(execRoot, tc.target)
res, err := getTargetRelPath(execRoot, tc.symlinkNormDir, tc.target)
if (err != nil) != tc.wantErr {
t.Errorf("getTargetRelPath(target=%q) error: expected=%v got=%v", tc.target, tc.wantErr, err)
}
Expand Down