Let symlinks be backed by path.Parser

Right now they are backed by byte slices. This leads to some incorrect
behavior on Windows. CASInitialContentsFetcher will create symlinks
containing forward slashes, which we then hand over to WinFSP without
any translation in between.

This change modifies symlinks to be backed by a path.Parser. This means
that at any point where we need to convert a symlink to a string
representation, we need to perform an explicit conversion to the
expected path format.

What is a bit annoying is that the st_size of symlinks on UNIX needs to
match the amount of data returned by readlink(). This means that symlink
objects can no longer be aware of their own size. We therefore remove
the SizeBytes attribute from these files. The FUSE and NFSv4 backends
will now consider both AttributesMaskSizeBytes and
AttributesMaskSymlinkTarget to determine the file's real size.

Fixes: #213

Author: EdSchouten

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(

cmd/bb_worker/main.go

@@ -221,7 +221,9 @@ func main() {
 
 				symlinkFactory = virtual.NewHandleAllocatingSymlinkFactory(
 					virtual.BaseSymlinkFactory,
-					handleAllocator.New())
+					handleAllocator.New(),
+					path.LocalFormat,
+				)
 				characterDeviceFactory = virtual.NewHandleAllocatingCharacterDeviceFactory(
 					virtual.BaseCharacterDeviceFactory,
 					handleAllocator.New())

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
 }

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
+}

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{

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 {

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)
 	}

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)

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",

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,
 		),
 	})