diff --git a/.github/workflows/main.yaml b/.github/workflows/main.yaml index cbb4a7c8..17040f19 100644 --- a/.github/workflows/main.yaml +++ b/.github/workflows/main.yaml @@ -953,7 +953,7 @@ { "if": "matrix.host.platform_name == 'windows_amd64'", "name": "Execute WinFSP Integration Tests", - "run": "bazel test //pkg/filesystem/virtual/winfsp:file_system_integration_test" + "run": "bazel test --test_output=errors //pkg/filesystem/virtual/winfsp:file_system_integration_test" }, { "env": { diff --git a/.github/workflows/pull-requests.yaml b/.github/workflows/pull-requests.yaml index 2af01c90..88c0356d 100644 --- a/.github/workflows/pull-requests.yaml +++ b/.github/workflows/pull-requests.yaml @@ -71,7 +71,7 @@ { "if": "matrix.host.platform_name == 'windows_amd64'", "name": "Execute WinFSP Integration Tests", - "run": "bazel test //pkg/filesystem/virtual/winfsp:file_system_integration_test" + "run": "bazel test --test_output=errors //pkg/filesystem/virtual/winfsp:file_system_integration_test" } ], "strategy": { diff --git a/cmd/bb_virtual_tmp/main.go b/cmd/bb_virtual_tmp/main.go index 9053e9e3..97707795 100644 --- a/cmd/bb_virtual_tmp/main.go +++ b/cmd/bb_virtual_tmp/main.go @@ -51,7 +51,7 @@ func main() { if err := path.Resolve(path.UNIXFormat.NewParser(configuration.BuildDirectoryPath), scopeWalker); err != nil { return util.StatusWrap(err, "Failed to resolve build directory path") } - userSettableSymlink := virtual.NewUserSettableSymlink(buildDirectory) + userSettableSymlink := virtual.NewUserSettableSymlink(buildDirectory, path.UNIXFormat.NewParser("/invalid")) // Expose the symbolic link through a virtual file system. mount, handleAllocator, err := virtual_configuration.NewMountFromConfiguration( diff --git a/cmd/bb_worker/main.go b/cmd/bb_worker/main.go index 79b9f8ca..0548681f 100644 --- a/cmd/bb_worker/main.go +++ b/cmd/bb_worker/main.go @@ -225,7 +225,9 @@ func main() { symlinkFactory = virtual.NewHandleAllocatingSymlinkFactory( virtual.BaseSymlinkFactory, - handleAllocator.New()) + handleAllocator.New(), + path.LocalFormat, + ) characterDeviceFactory = virtual.NewHandleAllocatingCharacterDeviceFactory( virtual.BaseCharacterDeviceFactory, handleAllocator.New()) diff --git a/pkg/builder/virtual_build_directory.go b/pkg/builder/virtual_build_directory.go index 5ad71c9b..5ddb8a14 100644 --- a/pkg/builder/virtual_build_directory.go +++ b/pkg/builder/virtual_build_directory.go @@ -194,12 +194,17 @@ func (d *virtualBuildDirectory) Readlink(name path.Component) (path.Parser, erro if err != nil { return nil, err } - if _, leaf := child.GetPair(); leaf != nil { - p := virtual.ApplyReadlink{} - if !child.GetNode().VirtualApply(&p) { - panic("build directory contains leaves that don't handle ApplyReadlink") - } - return p.Target, p.Err + + _, leaf := child.GetPair() + if leaf == nil { + return nil, syscall.EISDIR + } + + var attributes virtual.Attributes + leaf.VirtualGetAttributes(context.TODO(), virtual.AttributesMaskSymlinkTarget, &attributes) + symlinkTarget, ok := attributes.GetSymlinkTarget() + if !ok { + return nil, syscall.EINVAL } - return nil, syscall.EISDIR + return symlinkTarget, nil } diff --git a/pkg/filesystem/virtual/attributes.go b/pkg/filesystem/virtual/attributes.go index 534e9e6c..06c01a76 100644 --- a/pkg/filesystem/virtual/attributes.go +++ b/pkg/filesystem/virtual/attributes.go @@ -4,6 +4,7 @@ import ( "time" "github.com/buildbarn/bb-storage/pkg/filesystem" + "github.com/buildbarn/bb-storage/pkg/filesystem/path" ) // AttributesMask is a bitmask of status attributes that need to be @@ -51,7 +52,12 @@ const ( // bits of set_mode). AttributesMaskPermissions // AttributesMaskSizeBytes requests the file size (st_size). + // When requested, this field should be set for all types of + // files, except for symbolic links. AttributesMaskSizeBytes + // AttributesMaskSymlinkTarget requests the target of a symbolic + // link. + AttributesMaskSymlinkTarget ) // Attributes of a file, normally requested through stat() or readdir(). @@ -72,6 +78,7 @@ type Attributes struct { ownerUserID uint32 permissions Permissions sizeBytes uint64 + symlinkTarget path.Parser } // GetChangeID returns the change ID, which clients can use to determine @@ -264,3 +271,15 @@ func (a *Attributes) SetSizeBytes(sizeBytes uint64) *Attributes { a.fieldsPresent |= AttributesMaskSizeBytes return a } + +// GetSymlinkTarget returns the target of a symbolic link. +func (a *Attributes) GetSymlinkTarget() (path.Parser, bool) { + return a.symlinkTarget, a.fieldsPresent&AttributesMaskSymlinkTarget != 0 +} + +// SetSymlinkTarget sets the target of a symbolic link. +func (a *Attributes) SetSymlinkTarget(symlinkTarget path.Parser) *Attributes { + a.symlinkTarget = symlinkTarget + a.fieldsPresent |= AttributesMaskSymlinkTarget + return a +} diff --git a/pkg/filesystem/virtual/base_symlink_factory.go b/pkg/filesystem/virtual/base_symlink_factory.go index fd030110..57f1f47a 100644 --- a/pkg/filesystem/virtual/base_symlink_factory.go +++ b/pkg/filesystem/virtual/base_symlink_factory.go @@ -2,21 +2,17 @@ package virtual import ( "context" - "unicode/utf8" remoteexecution "github.com/bazelbuild/remote-apis/build/bazel/remote/execution/v2" "github.com/buildbarn/bb-remote-execution/pkg/proto/bazeloutputservice" "github.com/buildbarn/bb-storage/pkg/filesystem" "github.com/buildbarn/bb-storage/pkg/filesystem/path" - - "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" ) type symlinkFactory struct{} -func (symlinkFactory) LookupSymlink(target []byte) LinkableLeaf { - return symlink{target: target} +func (symlinkFactory) LookupSymlink(target path.Parser) (LinkableLeaf, error) { + return symlink{target: target}, nil } // BaseSymlinkFactory can be used to create simple immutable symlink nodes. @@ -25,23 +21,12 @@ var BaseSymlinkFactory SymlinkFactory = symlinkFactory{} type symlink struct { placeholderFile - target []byte -} - -func (f symlink) readlinkParser() (path.Parser, error) { - if !utf8.Valid(f.target) { - return nil, status.Error(codes.InvalidArgument, "Symbolic link contents are not valid UTF-8") - } - return path.UNIXFormat.NewParser(string(f.target)), nil + target path.Parser } func (f symlink) readlinkString() (string, error) { - targetParser, err := f.readlinkParser() - if err != nil { - return "", err - } targetPath, scopeWalker := path.EmptyBuilder.Join(path.VoidScopeWalker) - if err := path.Resolve(targetParser, scopeWalker); err != nil { + if err := path.Resolve(f.target, scopeWalker); err != nil { return "", err } return targetPath.GetUNIXString(), nil @@ -52,11 +37,7 @@ func (f symlink) VirtualGetAttributes(ctx context.Context, requested AttributesM attributes.SetFileType(filesystem.FileTypeSymlink) attributes.SetHasNamedAttributes(false) attributes.SetPermissions(PermissionsRead | PermissionsWrite | PermissionsExecute) - attributes.SetSizeBytes(uint64(len(f.target))) -} - -func (f symlink) VirtualReadlink(ctx context.Context) ([]byte, Status) { - return f.target, StatusOK + attributes.SetSymlinkTarget(f.target) } func (f symlink) VirtualSetAttributes(ctx context.Context, in *Attributes, requested AttributesMask, out *Attributes) Status { @@ -69,8 +50,6 @@ func (f symlink) VirtualSetAttributes(ctx context.Context, in *Attributes, reque func (f symlink) VirtualApply(data any) bool { switch p := data.(type) { - case *ApplyReadlink: - p.Target, p.Err = f.readlinkParser() case *ApplyGetBazelOutputServiceStat: if target, err := f.readlinkString(); err == nil { p.Stat = &bazeloutputservice.BatchStatResponse_Stat{ diff --git a/pkg/filesystem/virtual/blob_access_cas_file_factory.go b/pkg/filesystem/virtual/blob_access_cas_file_factory.go index 49291019..20e8ecd2 100644 --- a/pkg/filesystem/virtual/blob_access_cas_file_factory.go +++ b/pkg/filesystem/virtual/blob_access_cas_file_factory.go @@ -2,7 +2,6 @@ package virtual import ( "context" - "syscall" remoteexecution "github.com/bazelbuild/remote-apis/build/bazel/remote/execution/v2" "github.com/buildbarn/bb-remote-execution/pkg/proto/bazeloutputservice" @@ -73,8 +72,6 @@ func (blobAccessCASFile) VirtualAllocate(off, size uint64) Status { func (f *blobAccessCASFile) virtualApplyCommon(data any) bool { switch p := data.(type) { - case *ApplyReadlink: - p.Err = syscall.EINVAL case *ApplyUploadFile: // This file is already backed by the Content Addressable // Storage. There is thus no need to upload it once again. @@ -147,10 +144,6 @@ func (f *blobAccessCASFile) VirtualRead(buf []byte, off uint64) (int, bool, Stat return len(buf), eof, StatusOK } -func (blobAccessCASFile) VirtualReadlink(ctx context.Context) ([]byte, Status) { - return nil, StatusErrInval -} - func (blobAccessCASFile) VirtualClose(shareAccess ShareMask) {} func (blobAccessCASFile) virtualSetAttributesCommon(in *Attributes) Status { diff --git a/pkg/filesystem/virtual/cas_initial_contents_fetcher.go b/pkg/filesystem/virtual/cas_initial_contents_fetcher.go index 1767722e..7368bf6a 100644 --- a/pkg/filesystem/virtual/cas_initial_contents_fetcher.go +++ b/pkg/filesystem/virtual/cas_initial_contents_fetcher.go @@ -110,7 +110,10 @@ func (icf *casInitialContentsFetcher) fetchContentsUnwrapped(fileReadMonitorFact return nil, status.Errorf(codes.InvalidArgument, "Directory contains multiple children named %#v", entry.Name) } - leaf := icf.options.symlinkFactory.LookupSymlink([]byte(entry.Target)) + leaf, err := icf.options.symlinkFactory.LookupSymlink(path.UNIXFormat.NewParser(entry.Target)) + if err != nil { + return nil, status.Errorf(codes.InvalidArgument, "Failed to look up symlink named %#v", entry.Name) + } children[component] = InitialChild{}.FromLeaf(leaf) leavesToUnlink = append(leavesToUnlink, leaf) } diff --git a/pkg/filesystem/virtual/cas_initial_contents_fetcher_test.go b/pkg/filesystem/virtual/cas_initial_contents_fetcher_test.go index 84853fb5..6e106d09 100644 --- a/pkg/filesystem/virtual/cas_initial_contents_fetcher_test.go +++ b/pkg/filesystem/virtual/cas_initial_contents_fetcher_test.go @@ -212,7 +212,13 @@ func TestCASInitialContentsFetcherFetchContents(t *testing.T) { gomock.Any(), ).Return(fileLeaf) symlinkLeaf := mock.NewMockLinkableLeaf(ctrl) - symlinkFactory.EXPECT().LookupSymlink([]byte("target")).Return(symlinkLeaf) + symlinkFactory.EXPECT().LookupSymlink(gomock.Any()). + DoAndReturn(func(targetParser path.Parser) (virtual.LinkableLeaf, error) { + targetBuilder, scopeWalker := path.EmptyBuilder.Join(path.VoidScopeWalker) + require.NoError(t, path.Resolve(targetParser, scopeWalker)) + require.Equal(t, "target", targetBuilder.GetUNIXString()) + return symlinkLeaf, nil + }) children, err := initialContentsFetcher.FetchContents(fileReadMonitorFactory.Call) require.NoError(t, err) diff --git a/pkg/filesystem/virtual/configuration/BUILD.bazel b/pkg/filesystem/virtual/configuration/BUILD.bazel index d62c85dc..8e91f44b 100644 --- a/pkg/filesystem/virtual/configuration/BUILD.bazel +++ b/pkg/filesystem/virtual/configuration/BUILD.bazel @@ -22,6 +22,7 @@ go_library( "//pkg/proto/configuration/filesystem/virtual", "@com_github_buildbarn_bb_storage//pkg/clock", "@com_github_buildbarn_bb_storage//pkg/eviction", + "@com_github_buildbarn_bb_storage//pkg/filesystem/path", "@com_github_buildbarn_bb_storage//pkg/jmespath", "@com_github_buildbarn_bb_storage//pkg/program", "@com_github_buildbarn_bb_storage//pkg/random", diff --git a/pkg/filesystem/virtual/configuration/configuration.go b/pkg/filesystem/virtual/configuration/configuration.go index f363d9d9..f192001b 100644 --- a/pkg/filesystem/virtual/configuration/configuration.go +++ b/pkg/filesystem/virtual/configuration/configuration.go @@ -6,6 +6,7 @@ import ( pb "github.com/buildbarn/bb-remote-execution/pkg/proto/configuration/filesystem/virtual" "github.com/buildbarn/bb-storage/pkg/clock" "github.com/buildbarn/bb-storage/pkg/eviction" + "github.com/buildbarn/bb-storage/pkg/filesystem/path" "github.com/buildbarn/bb-storage/pkg/jmespath" "github.com/buildbarn/bb-storage/pkg/program" "github.com/buildbarn/bb-storage/pkg/random" @@ -111,6 +112,7 @@ func (m *nfsv4Mount) Expose(terminationGroup program.Group, rootDirectory virtua clock.SystemClock, enforcedLeaseTime.AsDuration(), announcedLeaseTime.AsDuration(), + path.LocalFormat, ), nfsv4.NewNFS40Program( rootDirectory, @@ -121,6 +123,7 @@ func (m *nfsv4Mount) Expose(terminationGroup program.Group, rootDirectory virtua clock.SystemClock, enforcedLeaseTime.AsDuration(), announcedLeaseTime.AsDuration(), + path.LocalFormat, ), }) diff --git a/pkg/filesystem/virtual/directory.go b/pkg/filesystem/virtual/directory.go index e2a69d98..c82cf811 100644 --- a/pkg/filesystem/virtual/directory.go +++ b/pkg/filesystem/virtual/directory.go @@ -72,7 +72,7 @@ type Directory interface { VirtualRemove(name path.Component, removeDirectory, removeLeaf bool) (ChangeInfo, Status) // VirtualSymlink creates a symbolic link within the current // directory. - VirtualSymlink(ctx context.Context, pointedTo []byte, linkName path.Component, requested AttributesMask, attributes *Attributes) (Leaf, ChangeInfo, Status) + VirtualSymlink(ctx context.Context, pointedTo path.Parser, linkName path.Component, requested AttributesMask, attributes *Attributes) (Leaf, ChangeInfo, Status) } const ( diff --git a/pkg/filesystem/virtual/fuse/simple_raw_file_system.go b/pkg/filesystem/virtual/fuse/simple_raw_file_system.go index 83a2389d..9c8831de 100644 --- a/pkg/filesystem/virtual/fuse/simple_raw_file_system.go +++ b/pkg/filesystem/virtual/fuse/simple_raw_file_system.go @@ -31,7 +31,8 @@ const ( virtual.AttributesMaskOwnerGroupID | virtual.AttributesMaskOwnerUserID | virtual.AttributesMaskPermissions | - virtual.AttributesMaskSizeBytes + virtual.AttributesMaskSizeBytes | + virtual.AttributesMaskSymlinkTarget // AttributesMaskForFUSEDirEntry is the attributes mask to use // for VirtualReadDir() to populate all relevant fields of // fuse.DirEntry. @@ -183,11 +184,18 @@ func populateAttr(attributes *virtual.Attributes, out *fuse.Attr) { } out.Mode |= uint32(permissions.ToMode()) - sizeBytes, ok := attributes.GetSizeBytes() - if !ok { + if sizeBytes, ok := attributes.GetSizeBytes(); ok { + out.Size = sizeBytes + } else if symlinkTarget, ok := attributes.GetSymlinkTarget(); ok { + pathBuilder, scopeWalker := path.EmptyBuilder.Join(path.VoidScopeWalker) + if err := path.Resolve(symlinkTarget, scopeWalker); err == nil { + out.Size = uint64(len(pathBuilder.GetUNIXString())) + } else { + out.Size = 1 + } + } else { panic("Attributes do not contain mandatory size attribute") } - out.Size = sizeBytes } func populateEntryOut(attributes *virtual.Attributes, out *fuse.EntryOut) { @@ -499,7 +507,7 @@ func (rfs *simpleRawFileSystem) Symlink(cancel <-chan struct{}, header *fuse.InH rfs.nodeLock.RUnlock() var attributes virtual.Attributes - child, _, vs := i.VirtualSymlink(ctx, []byte(pointedTo), path.MustNewComponent(linkName), AttributesMaskForFUSEAttr, &attributes) + child, _, vs := i.VirtualSymlink(ctx, path.UNIXFormat.NewParser(pointedTo), path.MustNewComponent(linkName), AttributesMaskForFUSEAttr, &attributes) if vs != virtual.StatusOK { return toFUSEStatus(vs) } @@ -517,8 +525,17 @@ func (rfs *simpleRawFileSystem) Readlink(cancel <-chan struct{}, header *fuse.In i := rfs.getLeafLocked(header.NodeId) rfs.nodeLock.RUnlock() - target, vs := i.VirtualReadlink(ctx) - return target, toFUSEStatus(vs) + var attributes virtual.Attributes + i.VirtualGetAttributes(ctx, virtual.AttributesMaskSymlinkTarget, &attributes) + symlinkTarget, ok := attributes.GetSymlinkTarget() + if !ok { + return nil, fuse.EINVAL + } + pathBuilder, scopeWalker := path.EmptyBuilder.Join(path.VoidScopeWalker) + if err := path.Resolve(symlinkTarget, scopeWalker); err != nil { + return nil, fuse.EIO + } + return []byte(pathBuilder.GetUNIXString()), fuse.OK } func (rfs *simpleRawFileSystem) Access(cancel <-chan struct{}, input *fuse.AccessIn) fuse.Status { diff --git a/pkg/filesystem/virtual/fuse/simple_raw_file_system_test.go b/pkg/filesystem/virtual/fuse/simple_raw_file_system_test.go index 98e4396f..b7939150 100644 --- a/pkg/filesystem/virtual/fuse/simple_raw_file_system_test.go +++ b/pkg/filesystem/virtual/fuse/simple_raw_file_system_test.go @@ -592,7 +592,7 @@ func TestSimpleRawFileSystemSymlink(t *testing.T) { t.Run("Failure", func(t *testing.T) { rootDirectory.EXPECT().VirtualSymlink( gomock.Any(), - []byte("target"), + gomock.Any(), path.MustNewComponent("symlink"), fuse.AttributesMaskForFUSEAttr, gomock.Any(), @@ -609,11 +609,15 @@ func TestSimpleRawFileSystemSymlink(t *testing.T) { symlink := mock.NewMockVirtualLeaf(ctrl) rootDirectory.EXPECT().VirtualSymlink( gomock.Any(), - []byte("target"), + gomock.Any(), path.MustNewComponent("symlink"), fuse.AttributesMaskForFUSEAttr, gomock.Any(), - ).DoAndReturn(func(ctx context.Context, pointedTo []byte, linkName path.Component, requested virtual.AttributesMask, out *virtual.Attributes) (virtual.Leaf, virtual.ChangeInfo, virtual.Status) { + ).DoAndReturn(func(ctx context.Context, pointedTo path.Parser, linkName path.Component, requested virtual.AttributesMask, out *virtual.Attributes) (virtual.Leaf, virtual.ChangeInfo, virtual.Status) { + pointedToBuilder, scopeWalker := path.EmptyBuilder.Join(path.VoidScopeWalker) + require.NoError(t, path.Resolve(pointedTo, scopeWalker)) + require.Equal(t, "target", pointedToBuilder.GetUNIXString()) + out.SetFileType(filesystem.FileTypeSymlink) out.SetInodeNumber(123) out.SetLinkCount(1) @@ -641,7 +645,10 @@ func TestSimpleRawFileSystemSymlink(t *testing.T) { // Future calls of the node should be forwarded to the // right symlink instance. - symlink.EXPECT().VirtualReadlink(gomock.Any()).Return([]byte("target"), virtual.StatusOK) + symlink.EXPECT().VirtualGetAttributes(gomock.Any(), virtual.AttributesMaskSymlinkTarget, gomock.Any()).DoAndReturn( + func(ctx context.Context, requested virtual.AttributesMask, out *virtual.Attributes) { + out.SetSymlinkTarget(path.UNIXFormat.NewParser("target")) + }) target, s := rfs.Readlink(nil, &go_fuse.InHeader{NodeId: 123}) require.Equal(t, go_fuse.OK, s) @@ -1190,22 +1197,18 @@ func TestSimpleRawFileSystemReadlink(t *testing.T) { }, }, entryOut) - t.Run("IOError", func(t *testing.T) { - symlink.EXPECT().VirtualReadlink(gomock.Any()).Return(nil, virtual.StatusErrIO) - - _, s := rfs.Readlink(nil, &go_fuse.InHeader{NodeId: 2}) - require.Equal(t, go_fuse.EIO, s) - }) - t.Run("WrongFileType", func(t *testing.T) { - symlink.EXPECT().VirtualReadlink(gomock.Any()).Return(nil, virtual.StatusErrInval) + symlink.EXPECT().VirtualGetAttributes(gomock.Any(), virtual.AttributesMaskSymlinkTarget, gomock.Any()) _, s := rfs.Readlink(nil, &go_fuse.InHeader{NodeId: 2}) require.Equal(t, go_fuse.EINVAL, s) }) t.Run("Success", func(t *testing.T) { - symlink.EXPECT().VirtualReadlink(gomock.Any()).Return([]byte("target"), virtual.StatusOK) + symlink.EXPECT().VirtualGetAttributes(gomock.Any(), virtual.AttributesMaskSymlinkTarget, gomock.Any()).DoAndReturn( + func(ctx context.Context, requested virtual.AttributesMask, out *virtual.Attributes) { + out.SetSymlinkTarget(path.UNIXFormat.NewParser("target")) + }) target, s := rfs.Readlink(nil, &go_fuse.InHeader{NodeId: 2}) require.Equal(t, go_fuse.OK, s) diff --git a/pkg/filesystem/virtual/handle_allocating_symlink_factory.go b/pkg/filesystem/virtual/handle_allocating_symlink_factory.go index ab56eb20..a81746db 100644 --- a/pkg/filesystem/virtual/handle_allocating_symlink_factory.go +++ b/pkg/filesystem/virtual/handle_allocating_symlink_factory.go @@ -1,5 +1,9 @@ package virtual +import ( + "github.com/buildbarn/bb-storage/pkg/filesystem/path" +) + type handleAllocatingSymlinkFactory struct { base SymlinkFactory allocator StatelessHandleAllocator @@ -14,15 +18,23 @@ type handleAllocatingSymlinkFactory struct { // the symbolic link to become part of the file handle, which is // undesirable. In the case of NFS we want these nodes to be explicitly // tracked, using an invisible link count. -func NewHandleAllocatingSymlinkFactory(base SymlinkFactory, allocation StatelessHandleAllocation) SymlinkFactory { +func NewHandleAllocatingSymlinkFactory(base SymlinkFactory, allocation StatelessHandleAllocation, pathFormat path.Format) SymlinkFactory { return &handleAllocatingSymlinkFactory{ base: base, allocator: allocation.AsStatelessAllocator(), } } -func (sf *handleAllocatingSymlinkFactory) LookupSymlink(target []byte) LinkableLeaf { +func (sf *handleAllocatingSymlinkFactory) LookupSymlink(target path.Parser) (LinkableLeaf, error) { + builder, scopeWalker := path.EmptyBuilder.Join(path.VoidScopeWalker) + if err := path.Resolve(target, scopeWalker); err != nil { + return nil, err + } + symlink, err := sf.base.LookupSymlink(target) + if err != nil { + return nil, err + } return sf.allocator. - New(ByteSliceID(target)). - AsLinkableLeaf(sf.base.LookupSymlink(target)) + New(ByteSliceID([]byte(builder.GetUNIXString()))). + AsLinkableLeaf(symlink), nil } diff --git a/pkg/filesystem/virtual/in_memory_prepopulated_directory.go b/pkg/filesystem/virtual/in_memory_prepopulated_directory.go index 2c683fd9..a3ffb44d 100644 --- a/pkg/filesystem/virtual/in_memory_prepopulated_directory.go +++ b/pkg/filesystem/virtual/in_memory_prepopulated_directory.go @@ -1108,7 +1108,7 @@ func (i *inMemoryPrepopulatedDirectory) VirtualSetAttributes(ctx context.Context return StatusOK } -func (i *inMemoryPrepopulatedDirectory) VirtualSymlink(ctx context.Context, pointedTo []byte, linkName path.Component, requested AttributesMask, out *Attributes) (Leaf, ChangeInfo, Status) { +func (i *inMemoryPrepopulatedDirectory) VirtualSymlink(ctx context.Context, pointedTo path.Parser, linkName path.Component, requested AttributesMask, out *Attributes) (Leaf, ChangeInfo, Status) { i.lock.Lock() defer i.lock.Unlock() @@ -1121,7 +1121,11 @@ func (i *inMemoryPrepopulatedDirectory) VirtualSymlink(ctx context.Context, poin if s := contents.virtualMayAttach(normalizedLinkName); s != StatusOK { return nil, ChangeInfo{}, s } - child := i.subtree.filesystem.symlinkFactory.LookupSymlink(pointedTo) + child, err := i.subtree.filesystem.symlinkFactory.LookupSymlink(pointedTo) + if err != nil { + i.subtree.errorLogger.Log(util.StatusWrapf(err, "Failed to create new symlink")) + return nil, ChangeInfo{}, StatusErrIO + } changeIDBefore := contents.changeID contents.attach(i.subtree, linkName, normalizedLinkName, inMemoryDirectoryChild{}.FromLeaf(child)) diff --git a/pkg/filesystem/virtual/in_memory_prepopulated_directory_test.go b/pkg/filesystem/virtual/in_memory_prepopulated_directory_test.go index 4bb04226..ed40ccc6 100644 --- a/pkg/filesystem/virtual/in_memory_prepopulated_directory_test.go +++ b/pkg/filesystem/virtual/in_memory_prepopulated_directory_test.go @@ -1689,7 +1689,7 @@ func TestInMemoryPrepopulatedDirectoryVirtualSymlink(t *testing.T) { Return(nil, status.Error(codes.Internal, "Network error")) errorLogger.EXPECT().Log(testutil.EqStatus(t, status.Error(codes.Internal, "Failed to initialize directory: Network error"))) - _, _, s := childDirectory.VirtualSymlink(ctx, []byte("target"), path.MustNewComponent("symlink"), 0, &virtual.Attributes{}) + _, _, s := childDirectory.VirtualSymlink(ctx, path.UNIXFormat.NewParser("target"), path.MustNewComponent("symlink"), 0, &virtual.Attributes{}) require.Equal(t, virtual.StatusErrIO, s) }) @@ -1701,13 +1701,14 @@ func TestInMemoryPrepopulatedDirectoryVirtualSymlink(t *testing.T) { path.MustNewComponent("existing_file"): virtual.InitialChild{}.FromLeaf(existingFile), }, false)) - _, _, s := d.VirtualSymlink(ctx, []byte("target"), path.MustNewComponent("existing_file"), 0, &virtual.Attributes{}) + _, _, s := d.VirtualSymlink(ctx, path.UNIXFormat.NewParser("target"), path.MustNewComponent("existing_file"), 0, &virtual.Attributes{}) require.Equal(t, virtual.StatusErrExist, s) }) t.Run("Success", func(t *testing.T) { leaf := mock.NewMockLinkableLeaf(ctrl) - symlinkFactory.EXPECT().LookupSymlink([]byte("target")).Return(leaf) + targetPathParser := path.UNIXFormat.NewParser("target") + symlinkFactory.EXPECT().LookupSymlink(targetPathParser).Return(leaf, nil) leaf.EXPECT().VirtualGetAttributes( ctx, virtual.AttributesMaskInodeNumber, @@ -1717,7 +1718,7 @@ func TestInMemoryPrepopulatedDirectoryVirtualSymlink(t *testing.T) { }) var out virtual.Attributes - actualLeaf, changeInfo, s := d.VirtualSymlink(ctx, []byte("target"), path.MustNewComponent("symlink"), virtual.AttributesMaskInodeNumber, &out) + actualLeaf, changeInfo, s := d.VirtualSymlink(ctx, targetPathParser, path.MustNewComponent("symlink"), virtual.AttributesMaskInodeNumber, &out) require.Equal(t, virtual.StatusOK, s) require.NotNil(t, actualLeaf) require.Equal(t, virtual.ChangeInfo{ diff --git a/pkg/filesystem/virtual/leaf.go b/pkg/filesystem/virtual/leaf.go index 3fcfe033..3e7b9c6f 100644 --- a/pkg/filesystem/virtual/leaf.go +++ b/pkg/filesystem/virtual/leaf.go @@ -52,7 +52,6 @@ type Leaf interface { VirtualSeek(offset uint64, regionType filesystem.RegionType) (*uint64, Status) VirtualOpenSelf(ctx context.Context, shareAccess ShareMask, options *OpenExistingOptions, requested AttributesMask, attributes *Attributes) Status VirtualRead(buf []byte, offset uint64) (n int, eof bool, s Status) - VirtualReadlink(ctx context.Context) ([]byte, Status) VirtualClose(shareAccess ShareMask) VirtualWrite(buf []byte, offset uint64) (int, Status) } diff --git a/pkg/filesystem/virtual/nfsv4/nfs40_program.go b/pkg/filesystem/virtual/nfsv4/nfs40_program.go index f82dde32..79433602 100644 --- a/pkg/filesystem/virtual/nfsv4/nfs40_program.go +++ b/pkg/filesystem/virtual/nfsv4/nfs40_program.go @@ -8,6 +8,7 @@ import ( "strconv" "sync" "time" + "unicode/utf8" "github.com/buildbarn/bb-remote-execution/pkg/filesystem/virtual" "github.com/buildbarn/bb-storage/pkg/clock" @@ -68,6 +69,7 @@ type nfs40Program struct { clock clock.Clock enforcedLeaseTime time.Duration announcedLeaseTime nfsv4.NfsLease4 + pathFormat path.Format lock sync.Mutex now time.Time @@ -84,7 +86,16 @@ type nfs40Program struct { // NewNFS40Program creates an nfsv4.Nfs4Program that forwards all // operations to a virtual file system. It implements most of the // features of NFSv4.0. -func NewNFS40Program(rootDirectory virtual.Directory, openedFilesPool *OpenedFilesPool, randomNumberGenerator random.SingleThreadedGenerator, rebootVerifier nfsv4.Verifier4, stateIDOtherPrefix [stateIDOtherPrefixLength]byte, clock clock.Clock, enforcedLeaseTime, announcedLeaseTime time.Duration) nfsv4.Nfs4Program { +func NewNFS40Program( + rootDirectory virtual.Directory, + openedFilesPool *OpenedFilesPool, + randomNumberGenerator random.SingleThreadedGenerator, + rebootVerifier nfsv4.Verifier4, + stateIDOtherPrefix [stateIDOtherPrefixLength]byte, + clock clock.Clock, + enforcedLeaseTime, announcedLeaseTime time.Duration, + pathFormat path.Format, +) nfsv4.Nfs4Program { nfs40ProgramPrometheusMetrics.Do(func() { prometheus.MustRegister(nfs40ProgramOpenOwnersCreated) prometheus.MustRegister(nfs40ProgramOpenOwnersRemoved) @@ -106,6 +117,7 @@ func NewNFS40Program(rootDirectory virtual.Directory, openedFilesPool *OpenedFil clock: clock, enforcedLeaseTime: enforcedLeaseTime, announcedLeaseTime: nfsv4.NfsLease4(announcedLeaseTime.Seconds()), + pathFormat: pathFormat, randomNumberGenerator: randomNumberGenerator, clientsByLongID: map[string]*nfs40ClientState{}, @@ -584,12 +596,18 @@ func (p *nfs40Program) writeAttributes(attributes *virtual.Attributes, attrReque nfsv4.WriteChangeid4(w, attributes.GetChangeID()) } if b := uint32(1 << nfsv4.FATTR4_SIZE); f&b != 0 { - sizeBytes, ok := attributes.GetSizeBytes() - if !ok { + s |= b + if sizeBytes, ok := attributes.GetSizeBytes(); ok { + nfsv4.WriteUint64T(w, sizeBytes) + } else if symlinkTarget, ok := attributes.GetSymlinkTarget(); ok { + if link, ok := pathParserToLinktext4(symlinkTarget, p.pathFormat); ok { + nfsv4.WriteUint64T(w, uint64(len(link))) + } else { + nfsv4.WriteUint64T(w, 1) + } + } else { panic("FATTR4_SIZE is a required attribute") } - s |= b - nfsv4.WriteUint64T(w, sizeBytes) } if b := uint32(1 << nfsv4.FATTR4_LINK_SUPPORT); f&b != 0 { s |= b @@ -1043,8 +1061,17 @@ func (s *compoundState) opCreate(ctx context.Context, args *nfsv4.Create4args) n leaf, changeInfo, vs = currentDirectory.VirtualMknod(ctx, name, filesystem.FileTypeFIFO, virtual.AttributesMaskFileHandle, &attributes) fileHandle.node = virtual.DirectoryChild{}.FromLeaf(leaf) case *nfsv4.Createtype4_NF4LNK: + if !utf8.Valid(objectType.Linkdata) { + return &nfsv4.Create4res_default{Status: nfsv4.NFS4ERR_BADCHAR} + } var leaf virtual.Leaf - leaf, changeInfo, vs = currentDirectory.VirtualSymlink(ctx, objectType.Linkdata, name, virtual.AttributesMaskFileHandle, &attributes) + leaf, changeInfo, vs = currentDirectory.VirtualSymlink( + ctx, + path.UNIXFormat.NewParser(string(objectType.Linkdata)), + name, + virtual.AttributesMaskFileHandle, + &attributes, + ) fileHandle.node = virtual.DirectoryChild{}.FromLeaf(leaf) case *nfsv4.Createtype4_NF4SOCK: var leaf virtual.Leaf @@ -1919,13 +1946,19 @@ func (s *compoundState) opReadlink(ctx context.Context) nfsv4.Readlink4res { } return &nfsv4.Readlink4res_default{Status: st} } - target, vs := currentLeaf.VirtualReadlink(ctx) - if vs != virtual.StatusOK { - return &nfsv4.Readlink4res_default{Status: toNFSv4Status(vs)} + var attributes virtual.Attributes + currentLeaf.VirtualGetAttributes(ctx, virtual.AttributesMaskSymlinkTarget, &attributes) + targetParser, ok := attributes.GetSymlinkTarget() + if !ok { + return &nfsv4.Readlink4res_default{Status: nfsv4.NFS4ERR_INVAL} + } + link, ok := pathParserToLinktext4(targetParser, s.program.pathFormat) + if !ok { + return &nfsv4.Readlink4res_default{Status: nfsv4.NFS4ERR_IO} } return &nfsv4.Readlink4res_NFS4_OK{ Resok4: nfsv4.Readlink4resok{ - Link: target, + Link: link, }, } } @@ -3074,7 +3107,7 @@ func attrRequestToAttributesMask(attrRequest nfsv4.Bitmap4) virtual.AttributesMa attributesMask |= virtual.AttributesMaskChangeID } if f&uint32(1</17/check_readiness + slotDir := filepath.Join(vfsPath, "17") + require.NoError(t, os.Mkdir(slotDir, 0o777)) + require.NoError(t, os.Mkdir(filepath.Join(slotDir, "check_readiness"), 0o777)) + + dir, err := bb_filesystem.NewLocalDirectory(bb_path.LocalFormat.NewParser(vfsPath + `\`)) + require.NoError(t, err, "Failed to open WinFSP mount root via NewLocalDirectory") + defer dir.Close() + + child, err := dir.EnterDirectory(bb_path.MustNewComponent("17")) + require.NoError(t, err, "Failed to enter subdirectory '17'") + defer child.Close() + + info, err := child.Lstat(bb_path.MustNewComponent("check_readiness")) + require.NoError(t, err, "Failed to stat 'check_readiness'") + require.True(t, info.Type() == bb_filesystem.FileTypeDirectory) + + return nil + }) +} diff --git a/pkg/filesystem/virtual/winfsp/file_system_test.go b/pkg/filesystem/virtual/winfsp/file_system_test.go index 8798b9c7..fcc79709 100644 --- a/pkg/filesystem/virtual/winfsp/file_system_test.go +++ b/pkg/filesystem/virtual/winfsp/file_system_test.go @@ -1314,8 +1314,13 @@ func TestWinFSPFileSystemGetReparsePointByName(t *testing.T) { }) // Mock readlink - target := []byte("target.txt") - symlink.EXPECT().VirtualReadlink(gomock.Any()).Return(target, virtual.StatusOK) + symlink.EXPECT().VirtualGetAttributes( + gomock.Any(), + virtual.AttributesMaskSymlinkTarget, + gomock.Any(), + ).Do(func(ctx context.Context, requested virtual.AttributesMask, attributes *virtual.Attributes) { + attributes.SetSymlinkTarget(path.WindowsFormat.NewParser("target.txt")) + }) buffer := make([]byte, 1024) bytesWritten, err := fs.GetReparsePointByName(ref, "\\symlink.txt", false, buffer) @@ -1374,8 +1379,13 @@ func TestWinFSPFileSystemGetReparsePoint(t *testing.T) { handle, err := fs.Open(ref, "\\symlink.txt", dispositionToOptions(windows.FILE_OPEN_IF), windows.FILE_READ_DATA, &info) require.NoError(t, err) - target := []byte("relative_target.txt") - symlink.EXPECT().VirtualReadlink(gomock.Any()).Return(target, virtual.StatusOK) + symlink.EXPECT().VirtualGetAttributes( + gomock.Any(), + virtual.AttributesMaskSymlinkTarget, + gomock.Any(), + ).Do(func(ctx context.Context, requested virtual.AttributesMask, attributes *virtual.Attributes) { + attributes.SetSymlinkTarget(path.WindowsFormat.NewParser("relative_target.txt")) + }) buffer := make([]byte, 1024) bytesWritten, err := fs.GetReparsePoint(ref, handle, "\\symlink.txt", buffer) @@ -1409,8 +1419,13 @@ func TestWinFSPFileSystemGetReparsePoint(t *testing.T) { handle, err := fs.Open(ref, "\\abs_symlink.txt", dispositionToOptions(windows.FILE_OPEN_IF), windows.FILE_READ_DATA, &info) require.NoError(t, err) - target := []byte("C:\\absolute\\target.txt") - symlink.EXPECT().VirtualReadlink(gomock.Any()).Return(target, virtual.StatusOK) + symlink.EXPECT().VirtualGetAttributes( + gomock.Any(), + virtual.AttributesMaskSymlinkTarget, + gomock.Any(), + ).Do(func(ctx context.Context, requested virtual.AttributesMask, attributes *virtual.Attributes) { + attributes.SetSymlinkTarget(path.WindowsFormat.NewParser("C:\\absolute\\target.txt")) + }) buffer := make([]byte, 1024) bytesWritten, err := fs.GetReparsePoint(ref, handle, "\\abs_symlink.txt", buffer) @@ -1419,7 +1434,7 @@ func TestWinFSPFileSystemGetReparsePoint(t *testing.T) { actualTarget, flags, err := extractSymlinkReparseTarget(buffer[:bytesWritten]) require.NoError(t, err) - require.Equal(t, "C:\\absolute\\target.txt", string(actualTarget)) + require.Equal(t, `\??\C:\absolute\target.txt`, string(actualTarget)) require.Equal(t, uint32(0), flags) }) @@ -1444,12 +1459,16 @@ func TestWinFSPFileSystemGetReparsePoint(t *testing.T) { handle, err := fs.Open(ref, "\\regular.txt", dispositionToOptions(windows.FILE_OPEN_IF), windows.FILE_READ_DATA, &info) require.NoError(t, err) - regularFile.EXPECT().VirtualReadlink(gomock.Any()).Return(nil, virtual.StatusErrSymlink) + regularFile.EXPECT().VirtualGetAttributes( + gomock.Any(), + virtual.AttributesMaskSymlinkTarget, + gomock.Any(), + ) buffer := make([]byte, 1024) _, err = fs.GetReparsePoint(ref, handle, "\\regular.txt", buffer) require.Error(t, err) - require.Equal(t, windows.STATUS_REPARSE, err) + require.Equal(t, windows.STATUS_INVALID_PARAMETER, err) }) } @@ -1956,11 +1975,17 @@ func TestWinFSPFileSystemSymlinkCreation(t *testing.T) { rootDirectory.EXPECT().VirtualSymlink( gomock.Any(), - []byte(targetPath), + gomock.Any(), path.MustNewComponent("symlink.txt"), virtual.AttributesMaskFileType, gomock.Any(), - ).DoAndReturn(func(ctx context.Context, target []byte, name path.Component, requested virtual.AttributesMask, out *virtual.Attributes) (virtual.Leaf, virtual.ChangeInfo, virtual.Status) { + ).DoAndReturn(func(ctx context.Context, target path.Parser, name path.Component, requested virtual.AttributesMask, out *virtual.Attributes) (virtual.Leaf, virtual.ChangeInfo, virtual.Status) { + targetBuilder, scopeWalker := path.EmptyBuilder.Join(path.VoidScopeWalker) + require.NoError(t, path.Resolve(target, scopeWalker)) + targetStr, err := path.LocalFormat.GetString(targetBuilder) + require.NoError(t, err) + require.Equal(t, targetPath, targetStr) + out.SetFileType(filesystem.FileTypeSymlink) out.SetInodeNumber(1101) out.SetPermissions(virtual.PermissionsRead) @@ -2014,11 +2039,17 @@ func TestWinFSPFileSystemSymlinkCreation(t *testing.T) { rootDirectory.EXPECT().VirtualSymlink( gomock.Any(), - []byte(targetPath), + gomock.Any(), path.MustNewComponent("abs_symlink.txt"), virtual.AttributesMaskFileType, gomock.Any(), - ).DoAndReturn(func(ctx context.Context, target []byte, name path.Component, requested virtual.AttributesMask, out *virtual.Attributes) (virtual.Leaf, virtual.ChangeInfo, virtual.Status) { + ).DoAndReturn(func(ctx context.Context, target path.Parser, name path.Component, requested virtual.AttributesMask, out *virtual.Attributes) (virtual.Leaf, virtual.ChangeInfo, virtual.Status) { + targetBuilder, scopeWalker := path.EmptyBuilder.Join(path.VoidScopeWalker) + require.NoError(t, path.Resolve(target, scopeWalker)) + targetStr, err := path.LocalFormat.GetString(targetBuilder) + require.NoError(t, err) + require.Equal(t, targetPath, targetStr) + out.SetFileType(filesystem.FileTypeSymlink) out.SetInodeNumber(1103) out.SetPermissions(virtual.PermissionsRead) diff --git a/tools/github_workflows/github_workflows.jsonnet b/tools/github_workflows/github_workflows.jsonnet index a47a2c64..6f6e9ecf 100644 --- a/tools/github_workflows/github_workflows.jsonnet +++ b/tools/github_workflows/github_workflows.jsonnet @@ -26,7 +26,7 @@ workflows_template.getWorkflows( }, { name: 'Execute WinFSP Integration Tests', - run: 'bazel test //pkg/filesystem/virtual/winfsp:file_system_integration_test', + run: 'bazel test --test_output=errors //pkg/filesystem/virtual/winfsp:file_system_integration_test', 'if': "matrix.host.platform_name == 'windows_amd64'", }, ]