Set ownership on symlinks created by build actions during their execution

cmd/bb_worker/main.go

@@ -182,7 +182,6 @@ func main() {
 		for _, buildDirectoryConfiguration := range configuration.BuildDirectories {
 			var virtualBuildDirectory virtual.PrepopulatedDirectory
 			var handleAllocator virtual.StatefulHandleAllocator
-			var symlinkFactory virtual.SymlinkFactory
 			var characterDeviceFactory virtual.CharacterDeviceFactory
 			var naiveBuildDirectory filesystem.DirectoryCloser
 			var fileFetcher cas.FileFetcher
@@ -223,11 +222,6 @@ func main() {
 					normalizer = virtual.CaseInsensitiveComponentNormalizer
 				}
 
-				symlinkFactory = virtual.NewHandleAllocatingSymlinkFactory(
-					virtual.BaseSymlinkFactory,
-					handleAllocator.New(),
-					path.LocalFormat,
-				)
 				characterDeviceFactory = virtual.NewHandleAllocatingCharacterDeviceFactory(
 					virtual.BaseCharacterDeviceFactory,
 					handleAllocator.New())
@@ -245,7 +239,7 @@ func main() {
 						),
 						handleAllocator,
 					),
-					symlinkFactory,
+					virtual.NewErrorSymlinkFactory(status.Error(codes.PermissionDenied, "Symlink outside build directory")),
 					util.DefaultErrorLogger,
 					handleAllocator,
 					initialContentsSorter,
@@ -337,6 +331,18 @@ func main() {
 					return err
 				}
 
+				defaultAttributesSetter := func(requested virtual.AttributesMask, attributes *virtual.Attributes) {
+					attributes.SetOwnerUserID(runnerConfiguration.BuildDirectoryOwnerUserId)
+					attributes.SetOwnerGroupID(runnerConfiguration.BuildDirectoryOwnerGroupId)
+				}
+				symlinkFactory := virtual.NewHandleAllocatingSymlinkFactory(
+					// Symlinks are not modified but rather replaced when changed.
+					// Therefore, it is safe to set the user as owner.
+					virtual.NewBaseSymlinkFactory(defaultAttributesSetter),
+					handleAllocator.New(),
+					path.LocalFormat,
+				)
+
 				// Execute commands using a separate runner process. Due to the
 				// interaction between threads, forking and execve() returning
 				// ETXTBSY, concurrent execution of build actions can only be
@@ -397,10 +403,7 @@ func main() {
 							symlinkFactory,
 							characterDeviceFactory,
 							handleAllocator,
-							/* defaultAttributesSetter = */ func(requested virtual.AttributesMask, attributes *virtual.Attributes) {
-								attributes.SetOwnerUserID(runnerConfiguration.BuildDirectoryOwnerUserId)
-								attributes.SetOwnerGroupID(runnerConfiguration.BuildDirectoryOwnerGroupId)
-							},
+							defaultAttributesSetter,
 							clock.SystemClock,
 						)
 					} else {

pkg/builder/virtual_build_directory.go

@@ -110,6 +110,7 @@ func (d *virtualBuildDirectory) InstallHooks(filePool pool.FilePool, errorLogger
 			),
 			do.handleAllocator,
 		),
+		do.symlinkFactory,
 		errorLogger,
 		do.defaultAttributesSetter,
 		namedAttributesFactory,

pkg/filesystem/virtual/BUILD.bazel

@@ -15,6 +15,7 @@ go_library(
         "component_normalizer.go",
         "directory.go",
         "empty_initial_contents_fetcher.go",
+        "error_symlink_factory.go",
         "file_allocator.go",
         "fuse_handle_allocator.go",
         "handle_allocating_file_allocator.go",

pkg/filesystem/virtual/base_symlink_factory.go

@@ -9,19 +9,30 @@ import (
 	"github.com/buildbarn/bb-storage/pkg/filesystem/path"
 )
 
-type symlinkFactory struct{}
+type baseSymlinkFactory struct {
+	defaultAttributesSetter DefaultAttributesSetter
+}
 
-func (symlinkFactory) LookupSymlink(target path.Parser) (LinkableLeaf, error) {
-	return symlink{target: target}, nil
+func (f *baseSymlinkFactory) LookupSymlink(target path.Parser) (LinkableLeaf, error) {
+	return symlink{
+		factory: f,
+		target:  target,
+	}, nil
 }
 
-// BaseSymlinkFactory can be used to create simple immutable symlink nodes.
-var BaseSymlinkFactory SymlinkFactory = symlinkFactory{}
+// NewBaseSymlinkFactory creates a SymlinkFactory that can be used to create
+// simple immutable symlink nodes.
+func NewBaseSymlinkFactory(defaultAttributesSetter DefaultAttributesSetter) SymlinkFactory {
+	return &baseSymlinkFactory{
+		defaultAttributesSetter: defaultAttributesSetter,
+	}
+}
 
 type symlink struct {
 	placeholderFile
 
-	target path.Parser
+	factory *baseSymlinkFactory
+	target  path.Parser
 }
 
 func (f symlink) readlinkString() (string, error) {
@@ -33,6 +44,7 @@ func (f symlink) readlinkString() (string, error) {
 }
 
 func (f symlink) VirtualGetAttributes(ctx context.Context, requested AttributesMask, attributes *Attributes) {
+	f.factory.defaultAttributesSetter(requested, attributes)
 	attributes.SetChangeID(0)
 	attributes.SetFileType(filesystem.FileTypeSymlink)
 	attributes.SetHasNamedAttributes(false)

pkg/filesystem/virtual/error_symlink_factory.go

@@ -0,0 +1,25 @@
+package virtual
+
+import (
+	"github.com/buildbarn/bb-storage/pkg/filesystem/path"
+)
+
+type errorSymlinkFactory struct {
+	err error
+}
+
+// NewErrorSymlinkFactory creates a SymlinkFactory that returns a fixed error
+// response. Such an implementation is useful for explicitly disabling symlink
+// creation.
+func NewErrorSymlinkFactory(err error) SymlinkFactory {
+	if err == nil {
+		panic("Attempted to create error symlink factory with nil error")
+	}
+	return &errorSymlinkFactory{
+		err: err,
+	}
+}
+
+func (sf *errorSymlinkFactory) LookupSymlink(target path.Parser) (LinkableLeaf, error) {
+	return nil, sf.err
+}

pkg/filesystem/virtual/in_memory_prepopulated_directory.go

@@ -25,7 +25,6 @@ type StringMatcher func(s string) bool
 // inMemoryFilesystem contains state that is shared across all
 // inMemoryPrepopulatedDirectory objects that form a single hierarchy.
 type inMemoryFilesystem struct {
-	symlinkFactory          SymlinkFactory
 	statefulHandleAllocator StatefulHandleAllocator
 	initialContentsSorter   Sorter
 	hiddenFilesMatcher      StringMatcher
@@ -44,6 +43,7 @@ type inMemoryFilesystem struct {
 type inMemorySubtree struct {
 	filesystem              *inMemoryFilesystem
 	fileAllocator           FileAllocator
+	symlinkFactory          SymlinkFactory
 	errorLogger             util.ErrorLogger
 	defaultAttributesSetter DefaultAttributesSetter
 	namedAttributesFactory  NamedAttributesFactory
@@ -52,14 +52,14 @@ type inMemorySubtree struct {
 func newInMemorySubtree(fileAllocator FileAllocator, symlinkFactory SymlinkFactory, errorLogger util.ErrorLogger, handleAllocator StatefulHandleAllocator, initialContentsSorter Sorter, hiddenFilesMatcher StringMatcher, clock clock.Clock, normalizer ComponentNormalizer, defaultAttributesSetter DefaultAttributesSetter, namedAttributesFactory NamedAttributesFactory) *inMemorySubtree {
 	return &inMemorySubtree{
 		filesystem: &inMemoryFilesystem{
-			symlinkFactory:          symlinkFactory,
 			statefulHandleAllocator: handleAllocator,
 			initialContentsSorter:   initialContentsSorter,
 			hiddenFilesMatcher:      hiddenFilesMatcher,
 			normalizer:              normalizer,
 			clock:                   clock,
 		},
 		fileAllocator:           fileAllocator,
+		symlinkFactory:          symlinkFactory,
 		errorLogger:             errorLogger,
 		defaultAttributesSetter: defaultAttributesSetter,
 		namedAttributesFactory:  namedAttributesFactory,
@@ -529,13 +529,14 @@ func (i *inMemoryPrepopulatedDirectory) postRemoveChildren(entries *inMemoryDire
 	}
 }
 
-func (i *inMemoryPrepopulatedDirectory) InstallHooks(fileAllocator FileAllocator, errorLogger util.ErrorLogger, defaultAttributesSetter DefaultAttributesSetter, namedAttributesFactory NamedAttributesFactory) {
+func (i *inMemoryPrepopulatedDirectory) InstallHooks(fileAllocator FileAllocator, symlinkFactory SymlinkFactory, errorLogger util.ErrorLogger, defaultAttributesSetter DefaultAttributesSetter, namedAttributesFactory NamedAttributesFactory) {
 	i.lock.Lock()
 	defer i.lock.Unlock()
 
 	i.subtree = &inMemorySubtree{
 		filesystem:              i.subtree.filesystem,
 		fileAllocator:           fileAllocator,
+		symlinkFactory:          symlinkFactory,
 		errorLogger:             errorLogger,
 		defaultAttributesSetter: defaultAttributesSetter,
 		namedAttributesFactory:  namedAttributesFactory,
@@ -1121,7 +1122,7 @@ func (i *inMemoryPrepopulatedDirectory) VirtualSymlink(ctx context.Context, poin
 	if s := contents.virtualMayAttach(normalizedLinkName); s != StatusOK {
 		return nil, ChangeInfo{}, s
 	}
-	child, err := i.subtree.filesystem.symlinkFactory.LookupSymlink(pointedTo)
+	child, err := i.subtree.symlinkFactory.LookupSymlink(pointedTo)
 	if err != nil {
 		i.subtree.errorLogger.Log(util.StatusWrapf(err, "Failed to create new symlink"))
 		return nil, ChangeInfo{}, StatusErrIO

pkg/filesystem/virtual/in_memory_prepopulated_directory_test.go

@@ -404,9 +404,10 @@ func TestInMemoryPrepopulatedDirectoryInstallHooks(t *testing.T) {
 	defaultAttributesSetter1 := mock.NewMockDefaultAttributesSetter(ctrl)
 	d := virtual.NewInMemoryPrepopulatedDirectory(fileAllocator1, symlinkFactory1, errorLogger1, handleAllocator, sort.Sort, hiddenFilesPatternForTesting.MatchString, clock.SystemClock, virtual.CaseSensitiveComponentNormalizer, defaultAttributesSetter1.Call, virtual.NoNamedAttributesFactory)
 	fileAllocator2 := mock.NewMockFileAllocator(ctrl)
+	symlinkFactory2 := mock.NewMockSymlinkFactory(ctrl)
 	errorLogger2 := mock.NewMockErrorLogger(ctrl)
 	defaultAttributesSetter2 := mock.NewMockDefaultAttributesSetter(ctrl)
-	d.InstallHooks(fileAllocator2, errorLogger2, defaultAttributesSetter2.Call, virtual.NoNamedAttributesFactory)
+	d.InstallHooks(fileAllocator2, symlinkFactory2, errorLogger2, defaultAttributesSetter2.Call, virtual.NoNamedAttributesFactory)
 
 	// Validate that the top-level directory uses both the new file
 	// allocator and error logger.
@@ -424,6 +425,27 @@ func TestInMemoryPrepopulatedDirectoryInstallHooks(t *testing.T) {
 		&attr)
 	require.Equal(t, virtual.StatusErrIO, s)
 
+	// Validate that symlinks uses the new symlink allocator
+	// and error logger as well.
+	symlinkLeaf := mock.NewMockLinkableLeaf(ctrl)
+	symlinkFactory2.EXPECT().LookupSymlink(path.UNIXFormat.NewParser("target")).Return(symlinkLeaf, nil)
+	symlinkLeaf.EXPECT().VirtualGetAttributes(
+		ctx,
+		virtual.AttributesMaskInodeNumber,
+		gomock.Any(),
+	).Do(func(ctx context.Context, requested virtual.AttributesMask, attributes *virtual.Attributes) {
+		attributes.SetInodeNumber(3)
+	})
+	var out virtual.Attributes
+	actualLeaf, changeInfo, s := d.VirtualSymlink(ctx, path.UNIXFormat.NewParser("target"), path.MustNewComponent("symlink"), virtual.AttributesMaskInodeNumber, &out)
+	require.Equal(t, virtual.StatusOK, s)
+	require.NotNil(t, actualLeaf)
+	require.Equal(t, virtual.ChangeInfo{
+		Before: 0,
+		After:  1,
+	}, changeInfo)
+	require.Equal(t, (&virtual.Attributes{}).SetInodeNumber(3), &out)
+
 	// Validate that a subdirectory uses the new file allocator
 	// and error logger as well.
 	inMemoryPrepopulatedDirectoryExpectMkdir(ctrl, handleAllocator)
@@ -1705,6 +1727,14 @@ func TestInMemoryPrepopulatedDirectoryVirtualSymlink(t *testing.T) {
 		require.Equal(t, virtual.StatusErrExist, s)
 	})
 
+	t.Run("FailingSymlinkFactory", func(t *testing.T) {
+		targetPathParser := path.UNIXFormat.NewParser("target")
+		symlinkFactory.EXPECT().LookupSymlink(targetPathParser).Return(nil, status.Error(codes.Internal, "Not allowed"))
+		errorLogger.EXPECT().Log(testutil.EqStatus(t, status.Error(codes.Internal, "Failed to create new symlink: Not allowed")))
+		_, _, s := d.VirtualSymlink(ctx, targetPathParser, path.MustNewComponent("symlink"), 0, &virtual.Attributes{})
+		require.Equal(t, virtual.StatusErrIO, s)
+	})
+
 	t.Run("Success", func(t *testing.T) {
 		leaf := mock.NewMockLinkableLeaf(ctrl)
 		targetPathParser := path.UNIXFormat.NewParser("target")

pkg/filesystem/virtual/prepopulated_directory.go

@@ -99,7 +99,7 @@ type PrepopulatedDirectory interface {
 	// This function is identical to BuildDirectory.InstallHooks(),
 	// except that it uses the FUSE specific FileAllocator instead
 	// of FilePool.
-	InstallHooks(fileAllocator FileAllocator, errorLogger util.ErrorLogger, defaultAttributesSetter DefaultAttributesSetter, namedAttributesFactory NamedAttributesFactory)
+	InstallHooks(fileAllocator FileAllocator, symlinkFactory SymlinkFactory, errorLogger util.ErrorLogger, defaultAttributesSetter DefaultAttributesSetter, namedAttributesFactory NamedAttributesFactory)
 	// FilterChildren() can be used to traverse over all of the
 	// InitialContentsFetcher and LinkableLeaf objects stored in this
 	// directory hierarchy. For each of the objects, a callback is

pkg/filesystem/virtual/winfsp/file_system_integration_test.go

@@ -81,7 +81,7 @@ func createWinFSPForTest(t *testing.T, terminationGroup program.Group, caseSensi
 	// Create a virtual directory to hold new files.
 	defaultAttributesSetter := func(requested virtual.AttributesMask, attributes *virtual.Attributes) {}
 	symlinkFactory := virtual.NewHandleAllocatingSymlinkFactory(
-		virtual.BaseSymlinkFactory,
+		virtual.NewBaseSymlinkFactory(defaultAttributesSetter),
 		handleAllocator.New(),
 		bb_path.LocalFormat,
 	)