Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

os: Lstat on darwin sometimes fails to expand symlinks in paths with trailing slashes #59586

Open
bcmills opened this issue Apr 12, 2023 · 10 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Darwin
Milestone

Comments

@bcmills
Copy link
Member

bcmills commented Apr 12, 2023

What version of Go are you using (go version)?

$ go version
go version devel go1.21-ebc13fb0b8 Wed Apr 12 16:09:24 2023 +0000 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/gopher/Library/Caches/go-build"
GOENV="/Users/gopher/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/tmp/buildlet/gopath/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/tmp/buildlet/gopath"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/tmp/buildlet/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/tmp/buildlet/go/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="devel gomote.XXXXX"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/tmp/buildlet/go/src/go.mod"
GOWORK=""
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/wh/9yc0j5w50z97w3528z_7qvr40000gn/T/go-build1774834589=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Extend path/filepath.TestWalkSymlinkRoot with the following test cases, inspired by cmd/go.TestScript/list_goroot_symlink:

diff --git i/src/path/filepath/path_test.go w/src/path/filepath/path_test.go
index cfc5cad863..4337bedda3 100644
--- i/src/path/filepath/path_test.go
+++ w/src/path/filepath/path_test.go
@@ -842,6 +842,16 @@ func TestWalkSymlinkRoot(t *testing.T) {
 		t.Fatal(err)
 	}

+	abslink := filepath.Join(td, "abslink")
+	if err := os.Symlink(dir, abslink); err != nil {
+		t.Fatal(err)
+	}
+
+	linklink := filepath.Join(td, "linklink")
+	if err := os.Symlink("link", linklink); err != nil {
+		t.Fatal(err)
+	}
+
 	// Per https://pubs.opengroup.org/onlinepubs/9699919799.2013edition/basedefs/V1_chap04.html#tag_04_12:
 	// “A pathname that contains at least one non- <slash> character and that ends
 	// with one or more trailing <slash> characters shall not be resolved
@@ -868,6 +878,26 @@ func TestWalkSymlinkRoot(t *testing.T) {
 			root: link + string(filepath.Separator),
 			want: []string{link, filepath.Join(link, "foo")},
 		},
+		{
+			desc: "abs no slash",
+			root: abslink,
+			want: []string{abslink},
+		},
+		{
+			desc: "abs with slash",
+			root: abslink + string(filepath.Separator),
+			want: []string{abslink, filepath.Join(abslink, "foo")},
+		},
+		{
+			desc: "double link no slash",
+			root: linklink,
+			want: []string{linklink},
+		},
+		{
+			desc: "double link with slash",
+			root: linklink + string(filepath.Separator),
+			want: []string{linklink, filepath.Join(linklink, "foo")},
+		},
 	} {
 		tt := tt
 		t.Run(tt.desc, func(t *testing.T) {

What did you expect to see?

The additional tests should pass.
Per POSIX pathname resolution, pathname resolution should proceed as:

  1. Any symlinks in the portion of the path in the td variable are resolved, giving a path of the form $td/linklink/ (where $td is fully-resolved).
    • The “remaining pathname” after the linklink component is /.
  2. $td/linklink resolves to $td/link, so $td/link should be prefixed to /, giving $td/link/ for the next step.
    • Since the pathname $td/link/ has a trailing slash and the path includes unresolved components, pathname resolution is not yet complete.
  3. $td/link resolves to $td/dir, so $td/dir should be prefixed to /, giving $td/dir/, which is fully resolved.

What did you see instead?

The additional tests pass on linux, consistent with POSIX pathname resolution.

However, the linklink test fails on a darwin-amd64-longtest gomote instance:

--- FAIL: TestWalkSymlinkRoot (0.00s)
    --- FAIL: TestWalkSymlinkRoot/double_link_with_slash (0.00s)
        path_test.go:909: `/var/folders/wh/9yc0j5w50z97w3528z_7qvr40000gn/T/TestWalkSymlinkRoot3176243506/001/linklink/`: Lrwxr-xr-x
        path_test.go:918: Walk(`/var/folders/wh/9yc0j5w50z97w3528z_7qvr40000gn/T/TestWalkSymlinkRoot3176243506/001/linklink/`) visited [`/var/folders/wh/9yc0j5w50z97w3528z_7qvr40000gn/T/TestWalkSymlinkRoot3176243506/001/linklink`]; want [`/var/folders/wh/9yc0j5w50z97w3528z_7qvr40000gn/T/TestWalkSymlinkRoot3176243506/001/linklink` `/var/folders/wh/9yc0j5w50z97w3528z_7qvr40000gn/T/TestWalkSymlinkRoot3176243506/001/linklink/foo`]
FAIL
FAIL    path/filepath   0.104s

(CC @golang/darwin)

@bcmills
Copy link
Member Author

bcmills commented Apr 12, 2023

On darwin, os.lstatNoLog is a thin wrapper around syscall.Lstat, which itself is a thin wrapper around libc_lstat64_trampoline.

I suspect that this is a bug in the macOS libc implementation of lstat64.

@bcmills bcmills added OS-Darwin NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Apr 12, 2023
@bcmills bcmills added this to the Backlog milestone Apr 12, 2023
@bcmills
Copy link
Member Author

bcmills commented Apr 12, 2023

darwin/arm64 uses lstat instead of lstat64 because it has a 64-bit ino_t (https://github.com/apple/darwin-xnu/blob/8f02f2a044b9bb1ad951987ef5bab20ec9486310/bsd/sys/cdefs.h#L604-L616). But it also exhibits the same bug.

@bcmills
Copy link
Member Author

bcmills commented Apr 12, 2023

Notably, POSIX requires:

A pathname that contains at least one non-<slash> character and that ends with one or more trailing <slash> characters shall not be resolved successfully unless the last pathname component before the trailing <slash> characters names an existing directory or a directory entry that is to be created for a directory immediately after the pathname is resolved.

@gopherbot
Copy link

Change https://go.dev/cl/484216 mentions this issue: cmd/go: skip TestScript/list_goroot_symlink on darwin

gopherbot pushed a commit that referenced this issue Apr 13, 2023
The list_goroot_symlink test relies on fsys.Walk (and ultimately
syscall.Lstat) conforming to POSIX pathname resolution semantics.
POSIX requires that symlinks ending in a slash be fully resolved,
but it appears that lstat in current darwin kernels does not fully
resolve the last pathname component when it is a symlink to a symlink.

For #59586.
For #35678.

Change-Id: I37526f012ba94fa1796b33109a41c3226c967d3e
Reviewed-on: https://go-review.googlesource.com/c/go/+/484216
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
TryBot-Bypass: Bryan Mills <bcmills@google.com>
@bcmills
Copy link
Member Author

bcmills commented Apr 13, 2023

A C reproducer:

issue59586.c
#define _XOPEN_SOURCE 700
#define _DARWIN_C_SOURCE 1
#include <unistd.h>
#include <sys/stat.h>
#include <stdio.h>
#include <stdlib.h>

int main() {
	char dir[] = "/tmp/issue59586-XXXXXX";
	if (mkdtemp(dir) == NULL) {
		perror("mkdtemp");
		exit(1);
	}
	if (chdir(dir) != 0) {
		perror("chdir");
		exit(1);
	}

	if (mkdir("dir", 0777) != 0) {
		perror("mkdir dir");
		exit(1);
	}

	if (symlink("dir", "link") != 0) {
		perror("symlink(dir, link)");
		exit(1);
	}

	if (symlink("link", "linklink") != 0) {
		perror("symlink(link, linklink)");
		exit(1);
	}

	struct stat st;
	if (lstat("linklink/", &st) != 0) {
		perror("lstat linklink/");
		exit(1);
	}

	if (S_ISLNK(st.st_mode)) {
		printf("symlink unresolved\n");
		exit(1);
	} else if (!S_ISDIR(st.st_mode)) {
		printf("resolved to non-directory\n");
		exit(1);
	} else {
		printf("resolved to directory\n");
	}
}

On Linux it prints resolved to directory, but on macOS 13 it prints symlink unresolved.

gopher@Gophers-iMac-Pro-2 src % clang ./issue59586.c && ./a.out
symlink unresolved
gopher@Gophers-iMac-Pro-2 src % clang --version
Apple clang version 14.0.0 (clang-1400.0.29.202)
Target: x86_64-apple-darwin22.1.0
Thread model: posix
InstalledDir: /Library/Developer/CommandLineTools/usr/bin

@gopherbot
Copy link

Change https://go.dev/cl/484395 mentions this issue: path/filepath: add test cases for walking a symlink-to-symlink-to-dir

@bcmills
Copy link
Member Author

bcmills commented Apr 13, 2023

Looks like only darwin is affected — the BSDs and SysV derivatives are all consistent with the behavior on Linux.

@bcmills bcmills modified the milestones: Backlog, Unplanned Apr 13, 2023
gopherbot pushed a commit that referenced this issue Apr 13, 2023
The "double link with slash" test is skipped on darwin due to an
apparent kernel / libc bug. If the bug is present on other platforms
too, I'd like to know about it.

For #59586.

Change-Id: I4bdd6a80a3ed7b0960ea6da30f91a655f317d512
Reviewed-on: https://go-review.googlesource.com/c/go/+/484395
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
@bcmills
Copy link
Member Author

bcmills commented Apr 13, 2023

Since this appears to be a kernel bug and we don't have a pressing need to work around it at the moment, moving to the Unplanned milestone.

If and when Apple fixes the macOS bug, we can remove the test-skips that refer to this issue and close it.

@rsc
Copy link
Contributor

rsc commented Apr 14, 2023

Here's a shell reproducer:

$ mkdir dir
$ ln -s dir link
$ ln -s link linklink
$ ls -l
total 0
drwxr-xr-x  2 rsc  primarygroup  64 Apr 13 20:12 dir
lrwxr-xr-x  1 rsc  primarygroup   3 Apr 13 20:12 link -> dir
lrwxr-xr-x  1 rsc  primarygroup   4 Apr 13 20:12 linklink -> link
$ stat dir
16777220 263323688 drwxr-xr-x 2 rsc primarygroup 0 64 "Apr 13 20:12:27 2023" "Apr 13 20:12:27 2023" "Apr 13 20:12:27 2023" "Apr 13 20:12:27 2023" 4096 0 0 dir
$ stat link
16777220 263323690 lrwxr-xr-x 1 rsc primarygroup 0 3 "Apr 13 20:12:31 2023" "Apr 13 20:12:31 2023" "Apr 13 20:12:31 2023" "Apr 13 20:12:31 2023" 4096 0 0 link
$ stat link/
16777220 263323688 drwxr-xr-x 2 rsc primarygroup 0 64 "Apr 13 20:12:27 2023" "Apr 13 20:12:27 2023" "Apr 13 20:12:27 2023" "Apr 13 20:12:27 2023" 4096 0 0 link/
$ stat linklink
16777220 263323700 lrwxr-xr-x 1 rsc primarygroup 0 4 "Apr 13 20:12:36 2023" "Apr 13 20:12:36 2023" "Apr 13 20:12:36 2023" "Apr 13 20:12:36 2023" 4096 0 0 linklink
$ stat linklink/
16777220 263323690 lrwxr-xr-x 1 rsc primarygroup 0 3 "Apr 13 20:12:31 2023" "Apr 13 20:12:31 2023" "Apr 13 20:12:31 2023" "Apr 13 20:12:31 2023" 4096 0 0 linklink/
$

The stat linklink/ at the end is the bug. It should resolve to the directory due to the trailing slash, same as stat link/.

As @bcmills mentioned above, resolving the symlink is required by POSIX, and I have confirmed that the bug is not present on FreeBSD or Linux, so this is not a case of "everyone ignores this POSIX detail".

@gopherbot
Copy link

Change https://go.dev/cl/484755 mentions this issue: path/filepath,cmd/go: skip tests involving double-symlinks on ios

gopherbot pushed a commit that referenced this issue Apr 14, 2023
For #59586.

Change-Id: I092f7a4abce1074b8eef64a3ecf9fc187914709b
Reviewed-on: https://go-review.googlesource.com/c/go/+/484755
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: David Chase <drchase@google.com>
TryBot-Bypass: Bryan Mills <bcmills@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Darwin
Projects
None yet
Development

No branches or pull requests

3 participants