From ea7c0715b2d5f543081d740c27c4d5c700982793 Mon Sep 17 00:00:00 2001 From: Fredrik Medley Date: Thu, 5 Feb 2026 20:17:45 +0100 Subject: [PATCH 1/4] Set ownership on symlinks created by build actions during their execution In commit 410ea5cf12ac778e3774f2c84af938bf98f47c57 "Set ownership on files created by build actions during their execution" files got correct ownership associated with them. This commit adds the same feature to symlinks. All directories, files and symlinks created by an action are owned by the user. Files provided as input are readonly and are therefore owned by root so that actions don't try to make them writable to edit them. Because directories are always mutable and symlink targets can only be modified by replacing them, directories and symlinks provided as inputs can safely be owned by the user. This commit changes the owner of user provided symlinks from root to the user. This solves the use case of hardlinking symlinks, which requires the user to be the owner of the symlink. The following test was performed with Bazel: ```starlark --- rules.bzl --- def _impl(ctx): file = ctx.actions.declare_file("file") symlink = ctx.actions.declare_symlink("symlink") args = ctx.actions.args() args.add_all([file, symlink]) ctx.actions.run_shell( outputs = [file, symlink], command = """ exec 2>&1 set +ex touch $1 ln -s file $2 ln $2 $2.hardlink ls -la $(dirname $1) """, arguments = [args], ) return [ DefaultInfo(files = depset([file, symlink])), OutputGroupInfo( file = depset([file]), ), ] hardlink_a_symlink_rule = rule( implementation = _impl, ) --- BUILD.bazel --- load(":rules.bzl", "hardlink_a_symlink_rule") hardlink_a_symlink_rule(name = "hardlink_a_symlink") filegroup( name = "hardlink_a_symlink_file", srcs = [":hardlink_a_symlink"], output_group = "file", ) genrule( name = "hardlink_input_symlink", srcs = [":hardlink_a_symlink", ":hardlink_a_symlink_file"], outs = ["symlink.another_hardlink"], cmd = """ exec 2>&1 set +ex cd $$(dirname $(location :hardlink_a_symlink_file)) ln symlink symlink.another_hardlink ls -la """, ) --- Output --- $ bazel build :hardlink_a_symlink INFO: From Action file: total 0 drwxrwxrwx 1 fredrik fredrik 0 Feb 5 21:12 . drwxrwxrwx 1 fredrik fredrik 0 Feb 5 21:12 .. -rw-rw-rw- 1 fredrik fredrik 0 Feb 5 21:12 file lrwxrwxrwx 9999 fredrik fredrik 4 Jan 1 2000 symlink -> file lrwxrwxrwx 9999 fredrik fredrik 4 Jan 1 2000 symlink.hardlink -> file INFO: From Executing genrule //:hardlink_input_symlink: total 0 drwxrwxrwx 1 fredrik fredrik 0 Feb 5 21:12 . drwxrwxrwx 1 fredrik fredrik 0 Feb 5 21:12 .. -r-xr-xr-x 9999 root root 0 Jan 1 2000 file lrwxrwxrwx 9999 fredrik fredrik 4 Jan 1 2000 symlink -> file lrwxrwxrwx 9999 fredrik fredrik 4 Jan 1 2000 symlink.another_hardlink -> file ``` References: https://sourceforge.net/p/fuse/mailman/message/35004606/ https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=800179c9b8a1e796e441674776d11cd4c05d61d7 https://github.com/torvalds/linux/commit/800179c9b8a1e796e441674776d11cd4c05d61d7 --- cmd/bb_worker/main.go | 10 ++---- pkg/builder/virtual_build_directory.go | 20 +++++++++--- pkg/filesystem/virtual/BUILD.bazel | 1 + .../virtual/base_symlink_factory.go | 24 ++++++++++---- .../virtual/error_symlink_factory.go | 27 ++++++++++++++++ .../handle_allocating_symlink_factory.go | 2 +- .../in_memory_prepopulated_directory.go | 9 +++--- .../in_memory_prepopulated_directory_test.go | 32 ++++++++++++++++++- .../virtual/prepopulated_directory.go | 2 +- .../winfsp/file_system_integration_test.go | 3 +- 10 files changed, 102 insertions(+), 28 deletions(-) create mode 100644 pkg/filesystem/virtual/error_symlink_factory.go diff --git a/cmd/bb_worker/main.go b/cmd/bb_worker/main.go index b3fa5bb3..f98d21b7 100644 --- a/cmd/bb_worker/main.go +++ b/cmd/bb_worker/main.go @@ -3,6 +3,7 @@ package main import ( "context" "encoding/json" + "errors" "fmt" "log" "net/http" @@ -182,7 +183,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 +223,6 @@ func main() { normalizer = virtual.CaseInsensitiveComponentNormalizer } - symlinkFactory = virtual.NewHandleAllocatingSymlinkFactory( - virtual.BaseSymlinkFactory, - handleAllocator.New(), - path.LocalFormat, - ) characterDeviceFactory = virtual.NewHandleAllocatingCharacterDeviceFactory( virtual.BaseCharacterDeviceFactory, handleAllocator.New()) @@ -245,7 +240,7 @@ func main() { ), handleAllocator, ), - symlinkFactory, + virtual.NewErrorSymlinkFactory(errors.New("Symlinks outside build directory not allowed")), util.DefaultErrorLogger, handleAllocator, initialContentsSorter, @@ -394,7 +389,6 @@ func main() { re_blobstore.NewSuspendingBlobAccess( contentAddressableStorageWriter, suspendableClock), - symlinkFactory, characterDeviceFactory, handleAllocator, /* defaultAttributesSetter = */ func(requested virtual.AttributesMask, attributes *virtual.Attributes) { diff --git a/pkg/builder/virtual_build_directory.go b/pkg/builder/virtual_build_directory.go index 5ddb8a14..9e0b4206 100644 --- a/pkg/builder/virtual_build_directory.go +++ b/pkg/builder/virtual_build_directory.go @@ -23,7 +23,6 @@ import ( type virtualBuildDirectoryOptions struct { directoryFetcher cas.DirectoryFetcher contentAddressableStorage blobstore.BlobAccess - symlinkFactory virtual.SymlinkFactory characterDeviceFactory virtual.CharacterDeviceFactory handleAllocator virtual.StatefulHandleAllocator defaultAttributesSetter virtual.DefaultAttributesSetter @@ -40,13 +39,12 @@ type virtualBuildDirectory struct { // input root explicitly, it calls PrepopulatedDirectory.CreateChildren // to add special file and directory nodes whose contents are read on // demand. -func NewVirtualBuildDirectory(directory virtual.PrepopulatedDirectory, directoryFetcher cas.DirectoryFetcher, contentAddressableStorage blobstore.BlobAccess, symlinkFactory virtual.SymlinkFactory, characterDeviceFactory virtual.CharacterDeviceFactory, handleAllocator virtual.StatefulHandleAllocator, defaultAttributesSetter virtual.DefaultAttributesSetter, clock clock.Clock) BuildDirectory { +func NewVirtualBuildDirectory(directory virtual.PrepopulatedDirectory, directoryFetcher cas.DirectoryFetcher, contentAddressableStorage blobstore.BlobAccess, characterDeviceFactory virtual.CharacterDeviceFactory, handleAllocator virtual.StatefulHandleAllocator, defaultAttributesSetter virtual.DefaultAttributesSetter, clock clock.Clock) BuildDirectory { return &virtualBuildDirectory{ PrepopulatedDirectory: directory, options: &virtualBuildDirectoryOptions{ directoryFetcher: directoryFetcher, contentAddressableStorage: contentAddressableStorage, - symlinkFactory: symlinkFactory, characterDeviceFactory: characterDeviceFactory, handleAllocator: handleAllocator, defaultAttributesSetter: defaultAttributesSetter, @@ -95,7 +93,10 @@ func (d *virtualBuildDirectory) InstallHooks(filePool pool.FilePool, errorLogger ), do.handleAllocator, ), - do.symlinkFactory, + virtual.NewHandleAllocatingSymlinkFactory( + virtual.NewBaseSymlinkFactory(do.defaultAttributesSetter), + do.handleAllocator.New(), + ), errorLogger, do.handleAllocator, do.clock, @@ -110,6 +111,10 @@ func (d *virtualBuildDirectory) InstallHooks(filePool pool.FilePool, errorLogger ), do.handleAllocator, ), + virtual.NewHandleAllocatingSymlinkFactory( + virtual.NewBaseSymlinkFactory(do.defaultAttributesSetter), + do.handleAllocator.New(), + ), errorLogger, do.defaultAttributesSetter, namedAttributesFactory, @@ -126,7 +131,12 @@ func (d *virtualBuildDirectory) MergeDirectoryContents(ctx context.Context, erro d.options.contentAddressableStorage, errorLogger), d.options.handleAllocator.New()), - d.options.symlinkFactory, + virtual.NewHandleAllocatingSymlinkFactory( + // Symlinks are not modified but rather replaced when changed. + // Therefore, it is safe to set the user as owner. + virtual.NewBaseSymlinkFactory(d.options.defaultAttributesSetter), + d.options.handleAllocator.New(), + ), digest.GetDigestFunction()) if monitor != nil { initialContentsFetcher = virtual.NewAccessMonitoringInitialContentsFetcher(initialContentsFetcher, monitor) diff --git a/pkg/filesystem/virtual/BUILD.bazel b/pkg/filesystem/virtual/BUILD.bazel index f03a16ae..a446fa5d 100644 --- a/pkg/filesystem/virtual/BUILD.bazel +++ b/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", diff --git a/pkg/filesystem/virtual/base_symlink_factory.go b/pkg/filesystem/virtual/base_symlink_factory.go index 57f1f47a..6158a15d 100644 --- a/pkg/filesystem/virtual/base_symlink_factory.go +++ b/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{ + defaultAttributesSetter: f.defaultAttributesSetter, + 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 + defaultAttributesSetter DefaultAttributesSetter + 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.defaultAttributesSetter(requested, attributes) attributes.SetChangeID(0) attributes.SetFileType(filesystem.FileTypeSymlink) attributes.SetHasNamedAttributes(false) diff --git a/pkg/filesystem/virtual/error_symlink_factory.go b/pkg/filesystem/virtual/error_symlink_factory.go new file mode 100644 index 00000000..a64acb73 --- /dev/null +++ b/pkg/filesystem/virtual/error_symlink_factory.go @@ -0,0 +1,27 @@ +package virtual + +import ( + "log" + + "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 { + log.Fatal("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 +} diff --git a/pkg/filesystem/virtual/handle_allocating_symlink_factory.go b/pkg/filesystem/virtual/handle_allocating_symlink_factory.go index 7fbfa5b7..6bc9d887 100644 --- a/pkg/filesystem/virtual/handle_allocating_symlink_factory.go +++ b/pkg/filesystem/virtual/handle_allocating_symlink_factory.go @@ -19,7 +19,7 @@ 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, pathFormat path.Format) SymlinkFactory { +func NewHandleAllocatingSymlinkFactory(base SymlinkFactory, allocation StatelessHandleAllocation) SymlinkFactory { return &handleAllocatingSymlinkFactory{ base: base, allocator: allocation.AsStatelessAllocator(), diff --git a/pkg/filesystem/virtual/in_memory_prepopulated_directory.go b/pkg/filesystem/virtual/in_memory_prepopulated_directory.go index a3ffb44d..687fdb07 100644 --- a/pkg/filesystem/virtual/in_memory_prepopulated_directory.go +++ b/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,7 +52,6 @@ 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, @@ -60,6 +59,7 @@ func newInMemorySubtree(fileAllocator FileAllocator, symlinkFactory SymlinkFacto 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 diff --git a/pkg/filesystem/virtual/in_memory_prepopulated_directory_test.go b/pkg/filesystem/virtual/in_memory_prepopulated_directory_test.go index ed40ccc6..bf596e77 100644 --- a/pkg/filesystem/virtual/in_memory_prepopulated_directory_test.go +++ b/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") diff --git a/pkg/filesystem/virtual/prepopulated_directory.go b/pkg/filesystem/virtual/prepopulated_directory.go index 0274346e..8686c53b 100644 --- a/pkg/filesystem/virtual/prepopulated_directory.go +++ b/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 diff --git a/pkg/filesystem/virtual/winfsp/file_system_integration_test.go b/pkg/filesystem/virtual/winfsp/file_system_integration_test.go index fcc5282a..82268149 100644 --- a/pkg/filesystem/virtual/winfsp/file_system_integration_test.go +++ b/pkg/filesystem/virtual/winfsp/file_system_integration_test.go @@ -81,9 +81,8 @@ 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, ) rootDir := virtual.NewInMemoryPrepopulatedDirectory( virtual.NewHandleAllocatingFileAllocator( From ca080dca985d65f0c038356c02628dbe1a4e6e62 Mon Sep 17 00:00:00 2001 From: Fredrik Medley Date: Mon, 30 Mar 2026 20:32:14 +0200 Subject: [PATCH 2/4] Fix review comments --- cmd/bb_worker/main.go | 4 +-- pkg/builder/virtual_build_directory.go | 28 +++++++++---------- .../virtual/base_symlink_factory.go | 10 +++---- .../virtual/error_symlink_factory.go | 4 +-- .../handle_allocating_symlink_factory.go | 2 +- 5 files changed, 22 insertions(+), 26 deletions(-) diff --git a/cmd/bb_worker/main.go b/cmd/bb_worker/main.go index f98d21b7..ed12bace 100644 --- a/cmd/bb_worker/main.go +++ b/cmd/bb_worker/main.go @@ -3,7 +3,6 @@ package main import ( "context" "encoding/json" - "errors" "fmt" "log" "net/http" @@ -240,7 +239,7 @@ func main() { ), handleAllocator, ), - virtual.NewErrorSymlinkFactory(errors.New("Symlinks outside build directory not allowed")), + virtual.NewErrorSymlinkFactory(status.Error(codes.PermissionDenied, "Symlink outside build directory")), util.DefaultErrorLogger, handleAllocator, initialContentsSorter, @@ -395,6 +394,7 @@ func main() { attributes.SetOwnerUserID(runnerConfiguration.BuildDirectoryOwnerUserId) attributes.SetOwnerGroupID(runnerConfiguration.BuildDirectoryOwnerGroupId) }, + path.LocalFormat, clock.SystemClock, ) } else { diff --git a/pkg/builder/virtual_build_directory.go b/pkg/builder/virtual_build_directory.go index 9e0b4206..29a16eef 100644 --- a/pkg/builder/virtual_build_directory.go +++ b/pkg/builder/virtual_build_directory.go @@ -23,6 +23,7 @@ import ( type virtualBuildDirectoryOptions struct { directoryFetcher cas.DirectoryFetcher contentAddressableStorage blobstore.BlobAccess + symlinkFactory virtual.SymlinkFactory characterDeviceFactory virtual.CharacterDeviceFactory handleAllocator virtual.StatefulHandleAllocator defaultAttributesSetter virtual.DefaultAttributesSetter @@ -39,12 +40,20 @@ type virtualBuildDirectory struct { // input root explicitly, it calls PrepopulatedDirectory.CreateChildren // to add special file and directory nodes whose contents are read on // demand. -func NewVirtualBuildDirectory(directory virtual.PrepopulatedDirectory, directoryFetcher cas.DirectoryFetcher, contentAddressableStorage blobstore.BlobAccess, characterDeviceFactory virtual.CharacterDeviceFactory, handleAllocator virtual.StatefulHandleAllocator, defaultAttributesSetter virtual.DefaultAttributesSetter, clock clock.Clock) BuildDirectory { +func NewVirtualBuildDirectory(directory virtual.PrepopulatedDirectory, directoryFetcher cas.DirectoryFetcher, contentAddressableStorage blobstore.BlobAccess, characterDeviceFactory virtual.CharacterDeviceFactory, handleAllocator virtual.StatefulHandleAllocator, defaultAttributesSetter virtual.DefaultAttributesSetter, pathFormat path.Format, clock clock.Clock) BuildDirectory { + 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(), + pathFormat, + ) return &virtualBuildDirectory{ PrepopulatedDirectory: directory, options: &virtualBuildDirectoryOptions{ directoryFetcher: directoryFetcher, contentAddressableStorage: contentAddressableStorage, + symlinkFactory: symlinkFactory, characterDeviceFactory: characterDeviceFactory, handleAllocator: handleAllocator, defaultAttributesSetter: defaultAttributesSetter, @@ -93,10 +102,7 @@ func (d *virtualBuildDirectory) InstallHooks(filePool pool.FilePool, errorLogger ), do.handleAllocator, ), - virtual.NewHandleAllocatingSymlinkFactory( - virtual.NewBaseSymlinkFactory(do.defaultAttributesSetter), - do.handleAllocator.New(), - ), + do.symlinkFactory, errorLogger, do.handleAllocator, do.clock, @@ -111,10 +117,7 @@ func (d *virtualBuildDirectory) InstallHooks(filePool pool.FilePool, errorLogger ), do.handleAllocator, ), - virtual.NewHandleAllocatingSymlinkFactory( - virtual.NewBaseSymlinkFactory(do.defaultAttributesSetter), - do.handleAllocator.New(), - ), + do.symlinkFactory, errorLogger, do.defaultAttributesSetter, namedAttributesFactory, @@ -131,12 +134,7 @@ func (d *virtualBuildDirectory) MergeDirectoryContents(ctx context.Context, erro d.options.contentAddressableStorage, errorLogger), d.options.handleAllocator.New()), - virtual.NewHandleAllocatingSymlinkFactory( - // Symlinks are not modified but rather replaced when changed. - // Therefore, it is safe to set the user as owner. - virtual.NewBaseSymlinkFactory(d.options.defaultAttributesSetter), - d.options.handleAllocator.New(), - ), + d.options.symlinkFactory, digest.GetDigestFunction()) if monitor != nil { initialContentsFetcher = virtual.NewAccessMonitoringInitialContentsFetcher(initialContentsFetcher, monitor) diff --git a/pkg/filesystem/virtual/base_symlink_factory.go b/pkg/filesystem/virtual/base_symlink_factory.go index 6158a15d..32cf6fe7 100644 --- a/pkg/filesystem/virtual/base_symlink_factory.go +++ b/pkg/filesystem/virtual/base_symlink_factory.go @@ -15,8 +15,8 @@ type baseSymlinkFactory struct { func (f *baseSymlinkFactory) LookupSymlink(target path.Parser) (LinkableLeaf, error) { return symlink{ - defaultAttributesSetter: f.defaultAttributesSetter, - target: target, + factory: f, + target: target, }, nil } @@ -31,8 +31,8 @@ func NewBaseSymlinkFactory(defaultAttributesSetter DefaultAttributesSetter) Syml type symlink struct { placeholderFile - defaultAttributesSetter DefaultAttributesSetter - target path.Parser + factory *baseSymlinkFactory + target path.Parser } func (f symlink) readlinkString() (string, error) { @@ -44,7 +44,7 @@ func (f symlink) readlinkString() (string, error) { } func (f symlink) VirtualGetAttributes(ctx context.Context, requested AttributesMask, attributes *Attributes) { - f.defaultAttributesSetter(requested, attributes) + f.factory.defaultAttributesSetter(requested, attributes) attributes.SetChangeID(0) attributes.SetFileType(filesystem.FileTypeSymlink) attributes.SetHasNamedAttributes(false) diff --git a/pkg/filesystem/virtual/error_symlink_factory.go b/pkg/filesystem/virtual/error_symlink_factory.go index a64acb73..f67df5fa 100644 --- a/pkg/filesystem/virtual/error_symlink_factory.go +++ b/pkg/filesystem/virtual/error_symlink_factory.go @@ -1,8 +1,6 @@ package virtual import ( - "log" - "github.com/buildbarn/bb-storage/pkg/filesystem/path" ) @@ -15,7 +13,7 @@ type errorSymlinkFactory struct { // creation. func NewErrorSymlinkFactory(err error) SymlinkFactory { if err == nil { - log.Fatal("Attempted to create error symlink factory with nil error") + panic("Attempted to create error symlink factory with nil error") } return &errorSymlinkFactory{ err: err, diff --git a/pkg/filesystem/virtual/handle_allocating_symlink_factory.go b/pkg/filesystem/virtual/handle_allocating_symlink_factory.go index 6bc9d887..7fbfa5b7 100644 --- a/pkg/filesystem/virtual/handle_allocating_symlink_factory.go +++ b/pkg/filesystem/virtual/handle_allocating_symlink_factory.go @@ -19,7 +19,7 @@ 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(), From 6916c600879dc727af743b0d02010163c33fd9c0 Mon Sep 17 00:00:00 2001 From: Fredrik Medley Date: Mon, 30 Mar 2026 21:37:30 +0200 Subject: [PATCH 3/4] Move symlinkFactory to worker main.go --- cmd/bb_worker/main.go | 19 ++++++++++++++----- pkg/builder/virtual_build_directory.go | 9 +-------- 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/cmd/bb_worker/main.go b/cmd/bb_worker/main.go index ed12bace..ed297c6a 100644 --- a/cmd/bb_worker/main.go +++ b/cmd/bb_worker/main.go @@ -331,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 @@ -388,13 +400,10 @@ func main() { re_blobstore.NewSuspendingBlobAccess( contentAddressableStorageWriter, suspendableClock), + symlinkFactory, characterDeviceFactory, handleAllocator, - /* defaultAttributesSetter = */ func(requested virtual.AttributesMask, attributes *virtual.Attributes) { - attributes.SetOwnerUserID(runnerConfiguration.BuildDirectoryOwnerUserId) - attributes.SetOwnerGroupID(runnerConfiguration.BuildDirectoryOwnerGroupId) - }, - path.LocalFormat, + defaultAttributesSetter, clock.SystemClock, ) } else { diff --git a/pkg/builder/virtual_build_directory.go b/pkg/builder/virtual_build_directory.go index 29a16eef..fbf5fa96 100644 --- a/pkg/builder/virtual_build_directory.go +++ b/pkg/builder/virtual_build_directory.go @@ -40,14 +40,7 @@ type virtualBuildDirectory struct { // input root explicitly, it calls PrepopulatedDirectory.CreateChildren // to add special file and directory nodes whose contents are read on // demand. -func NewVirtualBuildDirectory(directory virtual.PrepopulatedDirectory, directoryFetcher cas.DirectoryFetcher, contentAddressableStorage blobstore.BlobAccess, characterDeviceFactory virtual.CharacterDeviceFactory, handleAllocator virtual.StatefulHandleAllocator, defaultAttributesSetter virtual.DefaultAttributesSetter, pathFormat path.Format, clock clock.Clock) BuildDirectory { - 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(), - pathFormat, - ) +func NewVirtualBuildDirectory(directory virtual.PrepopulatedDirectory, directoryFetcher cas.DirectoryFetcher, contentAddressableStorage blobstore.BlobAccess, symlinkFactory virtual.SymlinkFactory, characterDeviceFactory virtual.CharacterDeviceFactory, handleAllocator virtual.StatefulHandleAllocator, defaultAttributesSetter virtual.DefaultAttributesSetter, clock clock.Clock) BuildDirectory { return &virtualBuildDirectory{ PrepopulatedDirectory: directory, options: &virtualBuildDirectoryOptions{ From 67f8d1fbf39fc48b674e41f555f6f8c4150efaf9 Mon Sep 17 00:00:00 2001 From: Fredrik Medley Date: Mon, 30 Mar 2026 22:08:09 +0200 Subject: [PATCH 4/4] Fix windows tests --- pkg/filesystem/virtual/winfsp/file_system_integration_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/filesystem/virtual/winfsp/file_system_integration_test.go b/pkg/filesystem/virtual/winfsp/file_system_integration_test.go index 82268149..4e4ce198 100644 --- a/pkg/filesystem/virtual/winfsp/file_system_integration_test.go +++ b/pkg/filesystem/virtual/winfsp/file_system_integration_test.go @@ -83,6 +83,7 @@ func createWinFSPForTest(t *testing.T, terminationGroup program.Group, caseSensi symlinkFactory := virtual.NewHandleAllocatingSymlinkFactory( virtual.NewBaseSymlinkFactory(defaultAttributesSetter), handleAllocator.New(), + bb_path.LocalFormat, ) rootDir := virtual.NewInMemoryPrepopulatedDirectory( virtual.NewHandleAllocatingFileAllocator(