Skip to content

Let symlinks be backed by path.Parser#216

Open
EdSchouten wants to merge 1 commit intomainfrom
eschouten/20260317-symlink
Open

Let symlinks be backed by path.Parser#216
EdSchouten wants to merge 1 commit intomainfrom
eschouten/20260317-symlink

Conversation

@EdSchouten
Copy link
Member

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

@aspect-workflows
Copy link

aspect-workflows bot commented Mar 17, 2026

Test

All tests were cache hits

19 tests (100.0%) were fully cached saving 3s.

@EdSchouten EdSchouten force-pushed the eschouten/20260317-symlink branch 9 times, most recently from e00c152 to f5b08b4 Compare March 18, 2026 10:48
EdSchouten added a commit to buildbarn/bb-clientd that referenced this pull request Mar 18, 2026
This change is necessary to catch up with the API changes introduced in
buildbarn/bb-remote-execution#216. It removes
any implementations of, and calls to VirtualReadLink(), and uses
VirtualGetAttributes(AttributesMaskSymlinkTarget) instead.

With this change applied, symlinks created in the virtual file system
are always normalized.

    $ ln -s //////../foo//bar/./baz
    $ ls -l baz
    lrwxrwxrwx  9999 nobody  nobody  12 Jan  1  2000 baz -> /foo/bar/baz
@EdSchouten EdSchouten force-pushed the eschouten/20260317-symlink branch from f5b08b4 to 6b5c48b Compare March 18, 2026 13:46
@EdSchouten
Copy link
Member Author

@tomgr Hmm... I'm seeing the file system integration test fail now. I tried debugging this via CI, but that's a bit annoying to do. Would you have an idea where to look to get this fixed?

@tomgr
Copy link
Contributor

tomgr commented Mar 18, 2026

I've got a fix for this branch in f6265f6 -- feel free to take that commit or bits of it and incorporate it into your commit if it's easier.

There are two separate fixes:

  1. Update bb-storage to get the new \. fix - without that some of the WinFSP relative symlink tests fail.
  2. Use WindowsPathFormatDevicePath when printing absolute symlinks so that \??\ is used in the SubstituteName we write into the reparse buffer.

The second one is a bit surprising - it's not documented anywhere I can find but it does seem that on Windows an absolute symlink target has to start with \??\ - that matches exactly what CreateSymbolicLink would do for example. It does seem windows relies on the SubstituteName being a NT path.

@tomgr
Copy link
Contributor

tomgr commented Mar 18, 2026

If like me you're wondering why this didn't get noticed before I think it's because absolute symlinks were never created by the CAS layer, only relative ones. Absolute symlinks were only created via OS calls and the old WinFSP VFS code would have just taken the symlink target from the WinFSP call, saved it, and then returned the same data when the symlink target was read. So if the symlink target started off with a \??\ prefix, it would have finished with one too.

@EdSchouten EdSchouten force-pushed the eschouten/20260317-symlink branch from 6b5c48b to 3f1081e Compare March 18, 2026 20:13
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.

Co-authored-by: Thomas Gibson-Robinson <tom@cocotec.io>
Fixes: #213
@EdSchouten EdSchouten force-pushed the eschouten/20260317-symlink branch from 3f1081e to 6efe325 Compare March 18, 2026 20:28
@EdSchouten
Copy link
Member Author

EdSchouten commented Mar 18, 2026

Thanks for your changes, @tomgr. I've just included them into this PR. Can you give this another spin and let me know whether this works for you?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants