Normalize REv2 symlink targets to native format

REv2 SymlinkNode targets use UNIX path separators `/`, and although
Windows file paths should handle `/` or `\` equivalently, this is not
the case when resolving symlink targets. In particular, symlink reparse
buffers are documented as being pathnames, which means they must:

1) use the `\` separator;
2) if targetting a directory, it must not have a trailing `\` separator.

We now normalise symlink targets on Windows when building the VFS
entries to ensure they obey these rules.

Author: tomgr

MODULE.bazel

@@ -23,8 +23,8 @@ git_override(
 
 git_override(
     module_name = "com_github_buildbarn_bb_storage",
-    commit = "6002cad335378bbdcdda4225d47c7dc2c08a6d94",
-    remote = "https://github.com/buildbarn/bb-storage.git",
+    commit = "5d535411b13e77d910bba6c7aab95de88252cd83",
+    remote = "https://github.com/tomgr/bb-storage.git",
 )
 
 git_override(

pkg/filesystem/virtual/BUILD.bazel

@@ -26,6 +26,8 @@ go_library(
         "leaf.go",
         "linkable_leaf.go",
         "named_attributes_factory.go",
+        "native_symlink_target_unix.go",
+        "native_symlink_target_windows.go",
         "nfs_handle_allocator.go",
         "node.go",
         "permissions.go",

pkg/filesystem/virtual/base_symlink_factory.go

@@ -32,7 +32,7 @@ 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
+	return path.LocalFormat.NewParser(string(f.target)), nil
 }
 
 func (f symlink) readlinkString() (string, error) {

pkg/filesystem/virtual/cas_initial_contents_fetcher.go

@@ -110,7 +110,14 @@ 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))
+		// REv2 symlink targets use UNIX path separators. Convert
+		// to the native platform format so that the stored bytes
+		// can be returned verbatim by VirtualReadlink.
+		target, err := nativeSymlinkTarget(entry.Target)
+		if err != nil {
+			return nil, util.StatusWrapf(err, "Failed to normalize target of symlink %#v", entry.Name)
+		}
+		leaf := icf.options.symlinkFactory.LookupSymlink(target)
 		children[component] = InitialChild{}.FromLeaf(leaf)
 		leavesToUnlink = append(leavesToUnlink, leaf)
 	}

pkg/filesystem/virtual/native_symlink_target_unix.go

@@ -0,0 +1,10 @@
+//go:build !windows
+
+package virtual
+
+// nativeSymlinkTarget converts a REv2 symlink target (UNIX format)
+// to the native platform format. On UNIX this is a no-op, as REv2
+// already uses UNIX path separators.
+func nativeSymlinkTarget(target string) ([]byte, error) {
+	return []byte(target), nil
+}

pkg/filesystem/virtual/native_symlink_target_windows.go

@@ -0,0 +1,22 @@
+//go:build windows
+
+package virtual
+
+import "github.com/buildbarn/bb-storage/pkg/filesystem/path"
+
+// nativeSymlinkTarget converts a REv2 symlink target (UNIX format)
+// to the native platform format. On Windows this converts forward
+// slashes to backslashes and strips trailing directory separators
+// that would cause issues in reparse point substitute names.
+func nativeSymlinkTarget(target string) ([]byte, error) {
+	targetParser := path.UNIXFormat.NewParser(target)
+	targetPath, scopeWalker := path.EmptyBuilder.Join(path.VoidScopeWalker)
+	if err := path.Resolve(targetParser, scopeWalker); err != nil {
+		return nil, err
+	}
+	s, err := targetPath.GetWindowsString(path.WindowsPathFormatStandard)
+	if err != nil {
+		return nil, err
+	}
+	return []byte(s), nil
+}

pkg/filesystem/virtual/winfsp/BUILD.bazel

@@ -68,12 +68,17 @@ go_test(
     tags = ["manual"],
     deps = select({
         "@rules_go//go/platform:windows": [
+            "//pkg/cas",
             "//pkg/filesystem/pool",
             "//pkg/filesystem/virtual",
             "//pkg/filesystem/virtual/configuration",
             "//pkg/proto/configuration/filesystem/virtual",
+            "@bazel_remote_apis//build/bazel/remote/execution/v2:remote_execution_go_proto",
             "@com_github_buildbarn_bb_storage//pkg/blockdevice",
             "@com_github_buildbarn_bb_storage//pkg/clock",
+            "@com_github_buildbarn_bb_storage//pkg/digest",
+            "@com_github_buildbarn_bb_storage//pkg/filesystem",
+            "@com_github_buildbarn_bb_storage//pkg/filesystem/path",
             "@com_github_buildbarn_bb_storage//pkg/program",
             "@com_github_buildbarn_bb_storage//pkg/util",
             "@com_github_stretchr_testify//require",

pkg/filesystem/virtual/winfsp/file_system.go

@@ -1352,17 +1352,17 @@ func getReparsePointForLeaf(ctx context.Context, leaf virtual.Leaf, buffer []byt
 		return 0, nil
 	}
 
-	// Parse the path to determine if it's absolute
+	// Symlink targets are stored in native (Windows) format, so
+	// we can pass them directly to FillSymlinkReparseBuffer.
 	w := relativePathChecker{}
 	if err := path.Resolve(path.LocalFormat.NewParser(string(target)), &w); err != nil {
 		return 0, err
 	}
-	var flags int
+	var flags uint32
 	if w.isRelative {
-		flags = windowsext.SYMLINK_FLAG_RELATIVE
+		flags = uint32(windowsext.SYMLINK_FLAG_RELATIVE)
 	}
-
-	return FillSymlinkReparseBuffer(string(target), uint32(flags), buffer)
+	return FillSymlinkReparseBuffer(string(target), flags, buffer)
 }
 
 func (fs *FileSystem) GetReparsePoint(ref *ffi.FileSystemRef, file uintptr, name string, buffer []byte) (int, error) {

pkg/filesystem/virtual/winfsp/file_system_integration_test.go

@@ -14,13 +14,17 @@ import (
 	"strings"
 	"testing"
 
+	remoteexecution "github.com/bazelbuild/remote-apis/build/bazel/remote/execution/v2"
 	"github.com/bazelbuild/rules_go/go/runfiles"
+	"github.com/buildbarn/bb-remote-execution/pkg/cas"
 	"github.com/buildbarn/bb-remote-execution/pkg/filesystem/pool"
 	"github.com/buildbarn/bb-remote-execution/pkg/filesystem/virtual"
 	virtual_configuration "github.com/buildbarn/bb-remote-execution/pkg/filesystem/virtual/configuration"
 	virtual_pb "github.com/buildbarn/bb-remote-execution/pkg/proto/configuration/filesystem/virtual"
 	"github.com/buildbarn/bb-storage/pkg/blockdevice"
 	"github.com/buildbarn/bb-storage/pkg/clock"
+	"github.com/buildbarn/bb-storage/pkg/digest"
+	bb_path "github.com/buildbarn/bb-storage/pkg/filesystem/path"
 	"github.com/buildbarn/bb-storage/pkg/program"
 	"github.com/buildbarn/bb-storage/pkg/util"
 	"github.com/stretchr/testify/require"
@@ -39,7 +43,7 @@ func findFreeDriveLetter() (string, error) {
 	return "", fmt.Errorf("no free drive letters available")
 }
 
-func createWinFSPMountForTest(t *testing.T, terminationGroup program.Group, caseSensitive bool) (string, blockdevice.BlockDevice) {
+func createWinFSPForTest(t *testing.T, terminationGroup program.Group, caseSensitive bool) (string, blockdevice.BlockDevice, virtual_configuration.Mount, virtual.PrepopulatedDirectory, virtual.SymlinkFactory) {
 	// We can't run winfsp-tests at a directory path due to
 	// https://github.com/winfsp/winfsp/issues/279. Instead find a free drive
 	// letter and run it there instead.
@@ -75,38 +79,42 @@ func createWinFSPMountForTest(t *testing.T, terminationGroup program.Group, case
 
 	// Create a virtual directory to hold new files.
 	defaultAttributesSetter := func(requested virtual.AttributesMask, attributes *virtual.Attributes) {}
-	err = mount.Expose(
-		terminationGroup,
-		virtual.NewInMemoryPrepopulatedDirectory(
-			virtual.NewHandleAllocatingFileAllocator(
-				virtual.NewPoolBackedFileAllocator(
-					pool.NewBlockDeviceBackedFilePool(
-						bd,
-						pool.NewBitmapSectorAllocator(uint32(sectorCount)),
-						sectorSizeBytes,
-					),
-					util.DefaultErrorLogger,
-					defaultAttributesSetter,
-					virtual.NoNamedAttributesFactory,
+	symlinkFactory := virtual.NewHandleAllocatingSymlinkFactory(
+		virtual.BaseSymlinkFactory,
+		handleAllocator.New(),
+	)
+	rootDir := virtual.NewInMemoryPrepopulatedDirectory(
+		virtual.NewHandleAllocatingFileAllocator(
+			virtual.NewPoolBackedFileAllocator(
+				pool.NewBlockDeviceBackedFilePool(
+					bd,
+					pool.NewBitmapSectorAllocator(uint32(sectorCount)),
+					sectorSizeBytes,
 				),
-				handleAllocator,
+				util.DefaultErrorLogger,
+				defaultAttributesSetter,
+				virtual.NoNamedAttributesFactory,
 			),
-			virtual.NewHandleAllocatingSymlinkFactory(
-				virtual.BaseSymlinkFactory,
-				handleAllocator.New(),
-			),
-			util.DefaultErrorLogger,
 			handleAllocator,
-			sort.Sort,
-			func(s string) bool { return false },
-			clock.SystemClock,
-			normalizer,
-			defaultAttributesSetter,
-			virtual.NoNamedAttributesFactory,
 		),
+		symlinkFactory,
+		util.DefaultErrorLogger,
+		handleAllocator,
+		sort.Sort,
+		func(s string) bool { return false },
+		clock.SystemClock,
+		normalizer,
+		defaultAttributesSetter,
+		virtual.NoNamedAttributesFactory,
 	)
-	require.NoError(t, err, "Failed to expose mount point")
 
+	return vfsPath, bd, mount, rootDir, symlinkFactory
+}
+
+func createWinFSPMountForTest(t *testing.T, terminationGroup program.Group, caseSensitive bool) (string, blockdevice.BlockDevice) {
+	vfsPath, bd, mount, rootDir, _ := createWinFSPForTest(t, terminationGroup, caseSensitive)
+	err := mount.Expose(terminationGroup, rootDir)
+	require.NoError(t, err, "Failed to expose mount point")
 	return vfsPath, bd
 }
 
@@ -417,6 +425,140 @@ func TestWinFSPFileSystemGetSecurityByNameIntegration(t *testing.T) {
 	})
 }
 
+// staticDirectoryWalker is a simple DirectoryWalker that returns a
+// fixed Directory proto. Used to test the CAS ingestion path with
+// symlinks whose targets use UNIX path separators (per the REv2 spec).
+type staticDirectoryWalker struct {
+	directory *remoteexecution.Directory
+}
+
+func (w *staticDirectoryWalker) GetDirectory(ctx context.Context) (*remoteexecution.Directory, error) {
+	return w.directory, nil
+}
+
+func (w *staticDirectoryWalker) GetChild(d digest.Digest) cas.DirectoryWalker {
+	return &staticDirectoryWalker{}
+}
+
+func (w *staticDirectoryWalker) GetDescription() string {
+	return "static test directory"
+}
+
+func (w *staticDirectoryWalker) GetContainingDigest() digest.Digest {
+	return digest.MustNewDigest("test", remoteexecution.DigestFunction_SHA256, "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", 0)
+}
+
+func TestWinFSPFileSystemStatFollowsSymlink(t *testing.T) {
+	program.RunLocal(context.Background(), func(ctx context.Context, siblingsGroup, dependenciesGroup program.Group) error {
+		vfsPath, bd, mount, rootDir, symlinkFactory := createWinFSPForTest(t, dependenciesGroup, false)
+		defer bd.Close()
+
+		// Build a pnpm-style node_modules layout with chained
+		// symlinks. Symlinks are created through the CAS
+		// ingestion path (NewCASInitialContentsFetcher) so that
+		// their UNIX-formatted targets from the REv2 proto are
+		// normalized to native Windows format, exercising the
+		// real production code path.
+		digestFunction := digest.MustNewFunction("test", remoteexecution.DigestFunction_SHA256)
+		noopFileReadMonitorFactory := virtual.FileReadMonitorFactory(
+			func(name bb_path.Component) virtual.FileReadMonitor {
+				return func() {}
+			})
+
+		// Helper to create a chain of nested directories.
+		mkdirs := func(parent virtual.PrepopulatedDirectory, names ...string) virtual.PrepopulatedDirectory {
+			for _, name := range names {
+				child, err := parent.CreateAndEnterPrepopulatedDirectory(bb_path.MustNewComponent(name))
+				require.NoError(t, err)
+				parent = child
+			}
+			return parent
+		}
+
+		// Helper to create symlink children via the CAS
+		// ingestion path, so that UNIX-formatted targets from
+		// the REv2 proto are normalized to native Windows format.
+		addCASSymlinks := func(parent virtual.PrepopulatedDirectory, symlinks []*remoteexecution.SymlinkNode) {
+			children, err := virtual.NewCASInitialContentsFetcher(
+				ctx,
+				&staticDirectoryWalker{directory: &remoteexecution.Directory{
+					Symlinks: symlinks,
+				}},
+				nil, symlinkFactory, digestFunction,
+			).FetchContents(noopFileReadMonitorFactory)
+			require.NoError(t, err)
+			require.NoError(t, parent.CreateChildren(children, false))
+		}
+
+		// store/pkg — the real directory.
+		mkdirs(rootDir, "store", "pkg")
+
+		// store-link -> store/pkg (single symlink).
+		addCASSymlinks(rootDir, []*remoteexecution.SymlinkNode{
+			{Name: "store-link", Target: "store/pkg"},
+		})
+
+		// node_modules/.pnpm/pkg@1.0.0/node_modules/pkg -> ../../../../store/pkg
+		nmDir := mkdirs(rootDir, "node_modules")
+		innerNmDir := mkdirs(nmDir, ".pnpm", "pkg@1.0.0", "node_modules")
+		addCASSymlinks(innerNmDir, []*remoteexecution.SymlinkNode{
+			{Name: "pkg", Target: "../../../../store/pkg"},
+		})
+
+		// node_modules/pkg -> .pnpm/pkg@1.0.0/node_modules/pkg (chained symlink).
+		addCASSymlinks(nmDir, []*remoteexecution.SymlinkNode{
+			{Name: "pkg", Target: ".pnpm/pkg@1.0.0/node_modules/pkg"},
+		})
+
+		require.NoError(t, mount.Expose(dependenciesGroup, rootDir))
+
+		// Write a file into the real directory after mounting.
+		testContent := []byte(`{"name":"pkg"}`)
+		require.NoError(t, os.WriteFile(
+			filepath.Join(vfsPath, "store", "pkg", "package.json"),
+			testContent, 0o644,
+		))
+
+		t.Run("SingleSymlink", func(t *testing.T) {
+			singleSymlinkPath := filepath.Join(vfsPath, "store-link")
+			info, err := os.Stat(singleSymlinkPath)
+			require.NoError(t, err)
+			require.True(t, info.IsDir())
+
+			content, err := os.ReadFile(filepath.Join(singleSymlinkPath, "package.json"))
+			require.NoError(t, err)
+			require.Equal(t, testContent, content)
+		})
+
+		t.Run("ChainedSymlinks", func(t *testing.T) {
+			symlinkPath := filepath.Join(vfsPath, "node_modules", "pkg")
+
+			info, err := os.Lstat(symlinkPath)
+			require.NoError(t, err)
+			require.NotZero(t, info.Mode()&os.ModeSymlink)
+
+			target, err := os.Readlink(symlinkPath)
+			require.NoError(t, err)
+			require.Equal(t, `.pnpm\pkg@1.0.0\node_modules\pkg`, target)
+
+			info, err = os.Stat(symlinkPath)
+			require.NoError(t, err)
+			require.True(t, info.IsDir())
+
+			content, err := os.ReadFile(filepath.Join(symlinkPath, "package.json"))
+			require.NoError(t, err)
+			require.Equal(t, testContent, content)
+
+			entries, err := os.ReadDir(symlinkPath)
+			require.NoError(t, err)
+			require.Len(t, entries, 1)
+			require.Equal(t, "package.json", entries[0].Name())
+		})
+
+		return nil
+	})
+}
+
 func TestWinFSPFileSystemCasePreserving(t *testing.T) {
 	program.RunLocal(context.Background(), func(ctx context.Context, siblingsGroup, dependenciesGroup program.Group) error {
 		vfsPath, bd := createWinFSPMountForTest(t, dependenciesGroup, false)