Skip to content

Commit

Permalink
Fix the way to compute the target path relative to the symlink (#245)
Browse files Browse the repository at this point in the history 
* Fix how target path relative to the symlink is computed

* more tests
  • Loading branch information
@k-ye
k-ye authored Nov 20, 2020
1 parent b732553 commit 8f976b9
Show file tree
Hide file tree
Showing 3 changed files with 95 additions and 61 deletions.
26 changes: 16 additions & 10 deletions go/pkg/client/tree.go
View file
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
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"},
},
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
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

0 comments on commit 8f976b9

Please sign in to comment.