Repository: tetratelabs/wazero
Pull request #1172 (2023-02-27 02:45:09)

windows: fixes unlinking symlink to dir, rmdir on opened empty dir.

mathetake: not found a fix yet.
mathetake: interesting as I expected this only fails with 1.20. 
mathetake: looks like this has existed for a while but somehow hasn't been caught until Zig test has started failing with 1.20 upgrade
mathetake: ok original issue has been fixed, but another comes up:

```

test "makePath in a directory that no longer exists" {
    if (builtin.os.tag == .windows) return error.SkipZigTest; // Windows returns FileBusy if attempting to remove an open dir

    var tmp = tmpDir(.{});
    defer tmp.cleanup();
    try tmp.parent_dir.deleteTree(&tmp.sub_path);

    try testing.expectError(error.FileNotFound, tmp.dir.makePath("sub-path"));
}

```
codefromthecrypt: @mathetake the other one looks like they are skipping on windows, which I guess is annoying because we can't do that without excluding "makePath in a directory that no longer exists" from all operating systems (due to the filter being a compile option). one way could be to exclude it from zig, but re-add the test in our repo so that it runs when != windows. wdyt?
mathetake: `makePath in a directory that no longer exists"` this is not the only test in zig skipped on window, and there's tens and we previously passing. Plus wasmtime passes so there must be a way to pass on windows. Also, we should be able to provide consistent behavior regardless of the host. So I will figure out this rather than adding skip on windows.
mathetake: minimize the case

```zig
test "minimized" {
    var tmp = tmpDir(.{});
    tmp.parent_dir.deleteTree(&tmp.sub_path) catch @panic("aaaaaaaa");
}
```
mathetake: found the repro which only fails on windows https://github.com/tetratelabs/wazero/pull/1172/commits/1e35027971bc7452863da464cb31f05e9a7557e2
mathetake: maybe time to resolve https://github.com/tetratelabs/wazero/blame/75aa6b2a6efa86b20e9b4eecd2889c83b714b1ca/internal/platform/open_file_windows.go#L40
mathetake: LOL only windows 1.18 is failing LOL
mathetake: 🤞 
mathetake: all went green

Patch:
diff --git a/internal/platform/open_file_windows.go b/internal/platform/open_file_windows.go
index 66414a47..c344806d 100644
--- a/internal/platform/open_file_windows.go
+++ b/internal/platform/open_file_windows.go
@@ -3,6 +3,8 @@ package platform
 import (
 	"io/fs"
 	"os"
+	"runtime"
+	"strings"
 	"syscall"
 	"unsafe"
 )
@@ -140,6 +142,18 @@ func open(path string, mode int, perm uint32) (fd syscall.Handle, err error) {
 			}
 		}
 	}
+
+	if isGo120 {
+		// This shouldn't be included before 1.20 to have consistent behavior.
+		// https://github.com/golang/go/commit/0f0aa5d8a6a0253627d58b3aa083b24a1091933f
+		if createmode == syscall.OPEN_EXISTING && access == syscall.GENERIC_READ {
+			// Necessary for opening directory handles.
+			attrs |= syscall.FILE_FLAG_BACKUP_SEMANTICS
+		}
+	}
+
 	h, e := syscall.CreateFile(pathp, access, sharemode, sa, createmode, attrs, 0)
 	return h, e
 }
+
+var isGo120 = strings.Contains(runtime.Version(), "go1.20")
diff --git a/internal/platform/unlink.go b/internal/platform/unlink.go
new file mode 100644
index 00000000..898d4b4c
--- /dev/null
+++ b/internal/platform/unlink.go
@@ -0,0 +1,13 @@
+//go:build !windows
+
+package platform
+
+import "syscall"
+
+func Unlink(name string) error {
+	err := syscall.Unlink(name)
+	if err = UnwrapOSError(err); err == syscall.EPERM {
+		err = syscall.EISDIR
+	}
+	return err
+}
diff --git a/internal/platform/unlink_windows.go b/internal/platform/unlink_windows.go
new file mode 100644
index 00000000..e5be95fd
--- /dev/null
+++ b/internal/platform/unlink_windows.go
@@ -0,0 +1,25 @@
+//go:build windows
+
+package platform
+
+import (
+	"os"
+	"syscall"
+)
+
+func Unlink(name string) (err error) {
+	err = syscall.Unlink(name)
+	if err == nil {
+		return
+	}
+	err = UnwrapOSError(err)
+	if err == syscall.EPERM {
+		lstat, errLstat := os.Lstat(name)
+		if errLstat == nil && lstat.Mode()&os.ModeSymlink != 0 {
+			err = UnwrapOSError(os.Remove(name))
+		} else {
+			err = syscall.EISDIR
+		}
+	}
+	return
+}
diff --git a/internal/sysfs/dirfs.go b/internal/sysfs/dirfs.go
index 027ebf51..06d28ab7 100644
--- a/internal/sysfs/dirfs.go
+++ b/internal/sysfs/dirfs.go
@@ -110,11 +110,7 @@ func (d *dirFS) Rmdir(name string) error {
 
 // Unlink implements FS.Unlink
 func (d *dirFS) Unlink(name string) (err error) {
-	err = syscall.Unlink(d.join(name))
-	if err = platform.UnwrapOSError(err); err == syscall.EPERM {
-		err = syscall.EISDIR
-	}
-	return
+	return platform.Unlink(d.join(name))
 }
 
 // Symlink implements FS.Symlink
diff --git a/internal/sysfs/sysfs.go b/internal/sysfs/sysfs.go
index 4baf5b8e..a6e9db3f 100644
--- a/internal/sysfs/sysfs.go
+++ b/internal/sysfs/sysfs.go
@@ -153,6 +153,12 @@ type FS interface {
 	//   - syscall.EINVAL: `path` is invalid.
 	//   - syscall.ENOENT: `path` doesn't exist.
 	//   - syscall.EISDIR: `path` exists, but is a directory.
+	//
+	// # Notes
+	//
+	//   - On Windows, syscall.Unlink doesn't delete symlink to directory unlike other platforms. Implementations might
+	//     want to combine syscall.RemoveDirectory with syscall.Unlink in order to delete such links on Windows.
+	//     See https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-removedirectorya
 	Unlink(path string) error
 
 	// Link is similar to syscall.Link, except the path is relative to this


Test patch:
diff --git a/internal/platform/unlink_test.go b/internal/platform/unlink_test.go
new file mode 100644
index 00000000..3e9d8d29
--- /dev/null
+++ b/internal/platform/unlink_test.go
@@ -0,0 +1,58 @@
+package platform
+
+import (
+	"os"
+	"path"
+	"syscall"
+	"testing"
+
+	"github.com/tetratelabs/wazero/internal/testing/require"
+)
+
+func TestUnlink(t *testing.T) {
+	t.Run("doesn't exist", func(t *testing.T) {
+		name := "non-existent"
+		err := Unlink(name)
+		require.EqualErrno(t, syscall.ENOENT, err)
+	})
+
+	t.Run("target: dir", func(t *testing.T) {
+		tmpDir := t.TempDir()
+
+		dir := path.Join(tmpDir, "dir")
+		require.NoError(t, os.Mkdir(dir, 0o700))
+
+		err := Unlink(dir)
+		require.EqualErrno(t, syscall.EISDIR, err)
+
+		require.NoError(t, os.Remove(dir))
+	})
+
+	t.Run("target: symlink to dir", func(t *testing.T) {
+		tmpDir := t.TempDir()
+
+		// Create link target dir.
+		subDirRealPath := path.Join(tmpDir, "subdir")
+		require.NoError(t, os.Mkdir(subDirRealPath, 0o700))
+
+		// Create a symlink to the subdirectory.
+		const symlinkName = "symlink-to-dir"
+		require.NoError(t, os.Symlink("subdir", symlinkName))
+
+		// Unlinking the symlink should suceed.
+		err := Unlink(symlinkName)
+		require.NoError(t, err)
+	})
+
+	t.Run("file exists", func(t *testing.T) {
+		tmpDir := t.TempDir()
+
+		name := path.Join(tmpDir, "unlink")
+
+		require.NoError(t, os.WriteFile(name, []byte{}, 0o600))
+
+		require.NoError(t, Unlink(name))
+		_, err := os.Stat(name)
+		require.Error(t, err)
+	})
+}
diff --git a/internal/sysfs/dirfs_test.go b/internal/sysfs/dirfs_test.go
index f9f54de0..cb1763b2 100644
--- a/internal/sysfs/dirfs_test.go
+++ b/internal/sysfs/dirfs_test.go
@@ -336,18 +336,23 @@ func TestDirFS_Rename(t *testing.T) {
 }
 
 func TestDirFS_Rmdir(t *testing.T) {
-	tmpDir := t.TempDir()
-	testFS := NewDirFS(tmpDir)
+	t.Run("doesn't exist", func(t *testing.T) {
+		tmpDir := t.TempDir()
+		testFS := NewDirFS(tmpDir)
 
-	name := "rmdir"
-	realPath := pathutil.Join(tmpDir, name)
+		name := "rmdir"
 
-	t.Run("doesn't exist", func(t *testing.T) {
 		err := testFS.Rmdir(name)
 		require.EqualErrno(t, syscall.ENOENT, err)
 	})
 
 	t.Run("dir not empty", func(t *testing.T) {
+		tmpDir := t.TempDir()
+		testFS := NewDirFS(tmpDir)
+
+		name := "rmdir"
+		realPath := pathutil.Join(tmpDir, name)
+
 		require.NoError(t, os.Mkdir(realPath, 0o700))
 		fileInDir := pathutil.Join(realPath, "file")
 		require.NoError(t, os.WriteFile(fileInDir, []byte{}, 0o600))
@@ -358,13 +363,62 @@ func TestDirFS_Rmdir(t *testing.T) {
 		require.NoError(t, os.Remove(fileInDir))
 	})
 
+	t.Run("dir previously not empty", func(t *testing.T) {
+		tmpDir := t.TempDir()
+		testFS := NewDirFS(tmpDir)
+
+		name := "rmdir"
+		realPath := pathutil.Join(tmpDir, name)
+		require.NoError(t, os.Mkdir(realPath, 0o700))
+
+		// Create a file and then delete it.
+		fileInDir := pathutil.Join(realPath, "file")
+		require.NoError(t, os.WriteFile(fileInDir, []byte{}, 0o600))
+		require.NoError(t, os.Remove(fileInDir))
+
+		// After deletion, try removing directory.
+		err := testFS.Rmdir(name)
+		require.NoError(t, err)
+	})
+
 	t.Run("dir empty", func(t *testing.T) {
+		tmpDir := t.TempDir()
+		testFS := NewDirFS(tmpDir)
+
+		name := "rmdir"
+		realPath := pathutil.Join(tmpDir, name)
+		require.NoError(t, os.Mkdir(realPath, 0o700))
 		require.NoError(t, testFS.Rmdir(name))
 		_, err := os.Stat(realPath)
 		require.Error(t, err)
 	})
 
+	t.Run("dir empty while opening", func(t *testing.T) {
+		tmpDir := t.TempDir()
+		testFS := NewDirFS(tmpDir)
+
+		name := "rmdir"
+		realPath := pathutil.Join(tmpDir, name)
+		require.NoError(t, os.Mkdir(realPath, 0o700))
+
+		f, err := testFS.OpenFile(name, platform.O_DIRECTORY, 0o700)
+		require.NoError(t, err)
+		defer func() {
+			require.NoError(t, f.Close())
+		}()
+
+		require.NoError(t, testFS.Rmdir(name))
+		_, err = os.Stat(realPath)
+		require.Error(t, err)
+	})
+
 	t.Run("not directory", func(t *testing.T) {
+		tmpDir := t.TempDir()
+		testFS := NewDirFS(tmpDir)
+
+		name := "rmdir"
+		realPath := pathutil.Join(tmpDir, name)
+
 		require.NoError(t, os.WriteFile(realPath, []byte{}, 0o600))
 
 		err := testFS.Rmdir(name)
@@ -375,27 +429,55 @@ func TestDirFS_Rmdir(t *testing.T) {
 }
 
 func TestDirFS_Unlink(t *testing.T) {
-	tmpDir := t.TempDir()
-	testFS := NewDirFS(tmpDir)
-
-	name := "unlink"
-	realPath := pathutil.Join(tmpDir, name)
-
 	t.Run("doesn't exist", func(t *testing.T) {
+		tmpDir := t.TempDir()
+		testFS := NewDirFS(tmpDir)
+		name := "unlink"
+
 		err := testFS.Unlink(name)
 		require.EqualErrno(t, syscall.ENOENT, err)
 	})
 
-	t.Run("not file", func(t *testing.T) {
+	t.Run("target: dir", func(t *testing.T) {
+		tmpDir := t.TempDir()
+		testFS := NewDirFS(tmpDir)
+
+		dir := "dir"
+		realPath := pathutil.Join(tmpDir, dir)
+
 		require.NoError(t, os.Mkdir(realPath, 0o700))
 
-		err := testFS.Unlink(name)
+		err := testFS.Unlink(dir)
 		require.EqualErrno(t, syscall.EISDIR, err)
 
 		require.NoError(t, os.Remove(realPath))
 	})
 
+	t.Run("target: symlink to dir", func(t *testing.T) {
+		tmpDir := t.TempDir()
+		testFS := NewDirFS(tmpDir)
+
+		// Create link target dir.
+		subDirName := "subdir"
+		subDirRealPath := pathutil.Join(tmpDir, subDirName)
+		require.NoError(t, os.Mkdir(subDirRealPath, 0o700))
+
+		// Create a symlink to the subdirectory.
+		const symlinkName = "symlink-to-dir"
+		require.NoError(t, testFS.Symlink("subdir", symlinkName))
+
+		// Unlinking the symlink should suceed.
+		err := testFS.Unlink(symlinkName)
+		require.NoError(t, err)
+	})
+
 	t.Run("file exists", func(t *testing.T) {
+		tmpDir := t.TempDir()
+		testFS := NewDirFS(tmpDir)
+
+		name := "unlink"
+		realPath := pathutil.Join(tmpDir, name)
+
 		require.NoError(t, os.WriteFile(realPath, []byte{}, 0o600))
 
 		require.NoError(t, testFS.Unlink(name))