darwin: syscallcompat: Openat: use O_SYMLINK

Also add tests that opening a symlink (using unix.O_PATH | unix.O_NOFOLLOW)
works.

https://github.com/rfjakob/gocryptfs/issues/993

6 files changed, 102 insertions, 8 deletions

diff --git a/internal/fusefrontend/root_node.go b/internal/fusefrontend/root_node.go
index 489e3c6..362290d 100644
--- a/internal/fusefrontend/root_node.go
+++ b/internal/fusefrontend/root_node.go
@@ -124,11 +124,11 @@ func (rn *RootNode) mangleOpenFlags(flags uint32) (newFlags int) {
 	// accesses. Running xfstests generic/013 on ext4 used to trigger lots of
 	// EINVAL errors due to missing alignment. Just fall back to buffered IO.
 	newFlags = newFlags &^ syscallcompat.O_DIRECT
-	// Create and Open are two separate FUSE operations, so O_CREAT should not
-	// be part of the open flags.
+	// Create and Open are two separate FUSE operations, so O_CREAT should usually not
+	// be part of the Open() flags. Create() will add O_CREAT back itself.
 	newFlags = newFlags &^ syscall.O_CREAT
-	// We always want O_NOFOLLOW to be safe against symlink races
-	newFlags |= syscall.O_NOFOLLOW
+	// We always want O_NOFOLLOW/O_SYMLINK to be safe against symlink races
+	newFlags |= syscallcompat.OpenatFlagNofollowSymlink
 	return newFlags
 }
 
diff --git a/internal/syscallcompat/sys_common.go b/internal/syscallcompat/sys_common.go
index 70ee633..3cb9ffa 100644
--- a/internal/syscallcompat/sys_common.go
+++ b/internal/syscallcompat/sys_common.go
@@ -54,10 +54,10 @@ func Openat(dirfd int, path string, flags int, mode uint32) (fd int, err error)
 			flags |= syscall.O_EXCL
 		}
 	} else {
-		// If O_CREAT is not used, we should use O_NOFOLLOW
-		if flags&syscall.O_NOFOLLOW == 0 {
-			tlog.Warn.Printf("Openat: O_NOFOLLOW missing: flags = %#x", flags)
-			flags |= syscall.O_NOFOLLOW
+		// If O_CREAT is not used, we should use O_NOFOLLOW or O_SYMLINK
+		if flags&(unix.O_NOFOLLOW|OpenatFlagNofollowSymlink) == 0 {
+			tlog.Warn.Printf("Openat: O_NOFOLLOW/O_SYMLINK missing: flags = %#x", flags)
+			flags |= unix.O_NOFOLLOW
 		}
 	}
 
diff --git a/internal/syscallcompat/sys_darwin.go b/internal/syscallcompat/sys_darwin.go
index 0ebdd3b..ef19f24 100644
--- a/internal/syscallcompat/sys_darwin.go
+++ b/internal/syscallcompat/sys_darwin.go
@@ -27,6 +27,10 @@ const (
 	// Only exists on Linux. Define here to fix build failure, even though
 	// we will never see this flag.
 	RENAME_WHITEOUT = 1 << 30
+
+	// On Darwin we use O_SYMLINK which allows opening a symlink itself.
+	// On Linux, we only have O_NOFOLLOW.
+	OpenatFlagNofollowSymlink = unix.O_SYMLINK
 )
 
 // Unfortunately fsetattrlist does not have a syscall wrapper yet.
diff --git a/internal/syscallcompat/sys_linux.go b/internal/syscallcompat/sys_linux.go
index 19d2c56..71478af 100644
--- a/internal/syscallcompat/sys_linux.go
+++ b/internal/syscallcompat/sys_linux.go
@@ -28,6 +28,10 @@ const (
 	RENAME_NOREPLACE = unix.RENAME_NOREPLACE
 	RENAME_WHITEOUT  = unix.RENAME_WHITEOUT
 	RENAME_EXCHANGE  = unix.RENAME_EXCHANGE
+
+	// On Darwin we use O_SYMLINK which allows opening a symlink itself.
+	// On Linux, we only have O_NOFOLLOW.
+	OpenatFlagNofollowSymlink = unix.O_NOFOLLOW
 )
 
 var preallocWarn sync.Once
diff --git a/tests/matrix/symlink_darwin_test.go b/tests/matrix/symlink_darwin_test.go
new file mode 100644
index 0000000..be28d9d
--- /dev/null
+++ b/tests/matrix/symlink_darwin_test.go
@@ -0,0 +1,39 @@
+package matrix
+
+import (
+	"os"
+	"testing"
+
+	"golang.org/x/sys/unix"
+
+	"github.com/rfjakob/gocryptfs/v2/tests/test_helpers"
+)
+
+// TestOpenSymlinkDarwin checks that a symlink can be opened
+// using O_SYMLINK.
+func TestOpenSymlinkDarwin(t *testing.T) {
+	path := test_helpers.DefaultPlainDir + "/TestOpenSymlink"
+	target := "/target/does/not/exist"
+	err := os.Symlink(target, path)
+	if err != nil {
+		t.Fatal(err)
+	}
+	fd, err := unix.Open(path, unix.O_RDONLY|unix.O_SYMLINK, 0)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer unix.Close(fd)
+	var st unix.Stat_t
+	if err := unix.Fstat(fd, &st); err != nil {
+		t.Fatal(err)
+	}
+	if st.Size != int64(len(target)) {
+		t.Errorf("wrong size: have=%d want=%d", st.Size, len(target))
+	}
+	if err := unix.Unlink(path); err != nil {
+		t.Fatal(err)
+	}
+	if err := unix.Fstat(fd, &st); err != nil {
+		t.Error(err)
+	}
+}
diff --git a/tests/matrix/symlink_linux_test.go b/tests/matrix/symlink_linux_test.go
new file mode 100644
index 0000000..fdb7051
--- /dev/null
+++ b/tests/matrix/symlink_linux_test.go
@@ -0,0 +1,47 @@
+package matrix
+
+import (
+	"os"
+	"testing"
+
+	"golang.org/x/sys/unix"
+
+	"github.com/rfjakob/gocryptfs/v2/tests/test_helpers"
+)
+
+// TestOpenSymlinkLinux checks that a symlink can be opened
+// using O_PATH.
+// Only works on Linux because is uses O_PATH and AT_EMPTY_PATH.
+// MacOS has O_SYMLINK instead (see TestOpenSymlinkDarwin).
+func TestOpenSymlinkLinux(t *testing.T) {
+	path := test_helpers.DefaultPlainDir + "/TestOpenSymlink"
+	target := "/target/does/not/exist"
+	err := os.Symlink(target, path)
+	if err != nil {
+		t.Fatal(err)
+	}
+	how := unix.OpenHow{
+		Flags: unix.O_PATH | unix.O_NOFOLLOW,
+	}
+	fd, err := unix.Openat2(unix.AT_FDCWD, path, &how)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer unix.Close(fd)
+	var st unix.Stat_t
+	if err := unix.Fstatat(fd, "", &st, unix.AT_EMPTY_PATH); err != nil {
+		t.Fatal(err)
+	}
+	if st.Size != int64(len(target)) {
+		t.Errorf("wrong size: have=%d want=%d", st.Size, len(target))
+	}
+	if err := unix.Unlink(path); err != nil {
+		t.Fatal(err)
+	}
+	if err = unix.Fstatat(fd, "", &st, unix.AT_EMPTY_PATH); err != nil {
+		// That's a bug, but I have never heard of a use case that would break because of this.
+		// Also I don't see how to fix it, as gocryptfs does not get informed about the earlier
+		// Openat2().
+		t.Logf("posix compliance issue: deleted symlink cannot be accessed: Fstatat: %v", err)
+	}
+}