Skip to content

Conversation

@rminnich
Copy link

@rminnich rminnich commented Jun 3, 2025

Provide package level ErrLoop instead.

@cyphar
Copy link
Owner

cyphar commented Jun 3, 2025

Plan9 doesn't have symlinks so I'm not sure what the benefit is of using this on Plan9 -- filepath.Join should theoretically be correct on Plan9 (it isn't race-safe but neither is SecureJoin).

error.Is(err, unix.ELOOP) continuing to work would be nice as well.

@rminnich
Copy link
Author

rminnich commented Jun 3, 2025

what's a better way to do this? Maybe just rewrite the whole thing for plan 9 without symlinks? I can't remove it, somebody else is using it ...

go-nfs

@rminnich
Copy link
Author

rminnich commented Jun 3, 2025

This will be zero behavioral change on unix, and only change what happens on plan 9

Copy link
Owner

@cyphar cyphar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please squash your commits.

I guess that making SecureJoin a wrapper for filepath.Join on Plan9 would feel a little weird. But so is having special symlink-related logic for a system that doesn't have symlinks. I'm fine with either I guess, it's just a bit odd.

@cyphar
Copy link
Owner

cyphar commented Jun 3, 2025

Basically my suggestion would be to have something like

//go:build plan9

// SecureJoin is equivalent to filepath.Join, as plan9 doesn't have symlinks.
func SecureJoin(root, unsafePath string) (string, error) {
    return filepath.Join(root, unsafePath), nil
}

// Ditto for SecureJoinVFS.

But I'm not sure if that's really preferable...

@rminnich
Copy link
Author

Your suggestion is much preferable to what I've done.

@rminnich
Copy link
Author

All non-symlink tests pass on Plan 9, and in fact found an error, thanks for the push.

Since Plan 9 does not have symlinks, these problems do not occur.
Therefore, SecureJoinVFS and SecureJoin can map to filepath.Join,
along with the test for rootpath containing ..

Split tests so some can run on Plan 9

Move common variables and functions into common.go

Signed-off-by: Ronald G Minnich <[email protected]>
@cyphar cyphar force-pushed the main branch 4 times, most recently from 325f6cb to 70d0ea2 Compare July 16, 2025 10:14
@cyphar cyphar force-pushed the main branch 3 times, most recently from bc37ffe to bc371b5 Compare August 8, 2025 05:20
@cyphar
Copy link
Owner

cyphar commented Sep 10, 2025

@rminnich Do you know of any way to get GHA to run a Plan9 VM (and a recommendation of which VM systems might work well with it) by any chance?

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