Bug: Close hvsock handle on listen error; fix tests (#310)

* Bug: Close hvsock handle on listen error; fix tests Close the socket created in `github.com/Microsoft/go-winio/pkg/ListenHvsock` if either the `Bind` or `Listen` calls fail. Go changed `filepath.VolumeName` code, resulting in different behavior in `github.com/Microsoft/go-winio/pkg/fs.GetFileSystemType`. Update test accordingly. Also add more debug logs to `pkg\fs\resolve_test.go`. Also, move add skip for fuzzing on WS2019 or older to `FuzzHvSockRxTx` code directly, instead of in ci.yml. See: https://go-review.googlesource.com/c/go/+/540277 Signed-off-by: Hamza El-Saawy <[email protected]> * PR: unskip TestResolvePath Signed-off-by: Hamza El-Saawy <[email protected]> --------- Signed-off-by: Hamza El-Saawy <[email protected]>
microsoft · Dec 18, 2023 · bc421d9 · bc421d9
1 parent 553a715
commit bc421d9
Show file tree

Hide file tree

Showing 5 changed files with 63 additions and 18 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -13,12 +13,15 @@ jobs:
     runs-on: windows-2019
     steps:
       - name: Checkout
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4
+        with:
+          show-progress: false
 
       - name: Install go
-        uses: actions/setup-go@v4
+        uses: actions/setup-go@v5
         with:
           go-version: ${{ env.GO_VERSION }}
+          cache: false
 
       - name: Run golangci-lint
         uses: golangci/golangci-lint-action@v3
@@ -37,12 +40,17 @@ jobs:
     runs-on: windows-2019
     steps:
       - name: Checkout
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4
+        with:
+          show-progress: false
 
       - name: Install go
-        uses: actions/setup-go@v4
+        uses: actions/setup-go@v5
         with:
           go-version: ${{ env.GO_VERSION }}
+          # don't really need to cache Go packages, since go generate doesn't require much.
+          # otherwise, the cache used in the `test` stage will be (basically) empty.
+          cache: false
 
       - name: Run go generate
         shell: pwsh
@@ -78,26 +86,30 @@ jobs:
         os: [windows-2019, windows-2022, ubuntu-latest]
     steps:
       - name: Checkout
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4
+        with:
+          show-progress: false
 
       - name: Install go
-        uses: actions/setup-go@v4
+        uses: actions/setup-go@v5
         with:
           go-version: ${{ env.GO_VERSION }}
 
+      # avoid needing to download packages during test runs
+      - name: Pre-fill Module Cache
+        run: go mod download -x
+
       - name: Install gotestsum
         run: go install gotest.tools/gotestsum@${{ env.GOTESTSUM_VERSION }}
 
       - name: Test repo
         run: gotestsum --format standard-verbose --debug -- -gcflags=all=-d=checkptr -race -v ./...
 
-      # Fuzzing was added in go1.18, so all stable/supported versions of go should support it.
-      # hvsock fuzzing fails on windows 2019, even though tests pass.
-      #
-      # If fuzzing tests are added to different packages, add them here.
-      - name: Fuzz repo
-        if: ${{ matrix.os == 'windows-2022' }}
-        run: gotestsum --format standard-verbose --debug -- -run "^#" -fuzztime 500x -fuzz "FuzzHvSock"
+      # !NOTE:
+      # Fuzzing cannot be run across multiple packages, (ie, `go -fuzz "^Fuzz" ./...` fails).
+      # If new fuzzing tests are added, exec additional runs for each package.
+      - name: Fuzz root package
+        run: gotestsum --format standard-verbose --debug -- -run "^#" -fuzztime 1m -fuzz "^Fuzz"
 
   build:
     name: Build Repo
@@ -106,10 +118,12 @@ jobs:
     runs-on: "windows-2019"
     steps:
       - name: Checkout
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4
+        with:
+          show-progress: false
 
       - name: Install go
-        uses: actions/setup-go@v4
+        uses: actions/setup-go@v5
         with:
           go-version: ${{ env.GO_VERSION }}
 

diff --git a/hvsock.go b/hvsock.go
@@ -196,10 +196,18 @@ func newHVSocket() (*win32File, error) {
 // ListenHvsock listens for connections on the specified hvsock address.
 func ListenHvsock(addr *HvsockAddr) (_ *HvsockListener, err error) {
 	l := &HvsockListener{addr: *addr}
-	sock, err := newHVSocket()
+
+	var sock *win32File
+	sock, err = newHVSocket()
 	if err != nil {
 		return nil, l.opErr("listen", err)
 	}
+	defer func() {
+		if err != nil {
+			_ = sock.Close()
+		}
+	}()
+
 	sa := addr.raw()
 	err = socket.Bind(sock.handle, &sa)
 	if err != nil {

diff --git a/hvsock_go118_test.go b/hvsock_go118_test.go
@@ -7,9 +7,16 @@ import (
 	"fmt"
 	"testing"
 	"time"
+
+	"golang.org/x/sys/windows"
 )
 
 func FuzzHvSockRxTx(f *testing.F) {
+	//  fuzzing fails on windows 2019 for some reason, even though tests pass
+	if _, _, build := windows.RtlGetNtVersionNumbers(); build <= 17763 {
+		f.Skipf("build (%d) must be > %d", build, 17763)
+	}
+
 	for _, b := range [][]byte{
 		[]byte("hello?"),
 		[]byte("This is a really long string that should be a good example of the really long " +

diff --git a/pkg/fs/fs_windows_test.go b/pkg/fs/fs_windows_test.go
@@ -18,7 +18,9 @@ func TestGetFSTypeOfKnownDrive(t *testing.T) {
 }
 
 func TestGetFSTypeOfInvalidPath(t *testing.T) {
-	_, err := GetFileSystemType("7:\\")
+	// [filepath.VolumeName] doesn't mandate that the drive letters matches [a-zA-Z].
+	// Instead, use non-character drive.
+	_, err := GetFileSystemType(`No:\`)
 	if !errors.Is(err, ErrInvalidPath) {
 		t.Fatalf("Expected `ErrInvalidPath`, got %v", err)
 	}

diff --git a/pkg/fs/resolve_test.go b/pkg/fs/resolve_test.go
@@ -25,9 +25,19 @@ func getWindowsBuildNumber() uint32 {
 func makeSymlink(t *testing.T, oldName string, newName string) {
 	t.Helper()
 
+	t.Logf("make symlink: %s -> %s", oldName, newName)
+
+	if _, err := os.Lstat(oldName); err != nil {
+		t.Fatalf("could not open file %q: %v", oldName, err)
+	}
+
 	if err := os.Symlink(oldName, newName); err != nil {
 		t.Fatalf("creating symlink: %s", err)
 	}
+
+	if _, err := os.Lstat(newName); err != nil {
+		t.Fatalf("could not open file %q: %v", newName, err)
+	}
 }
 
 func getVolumeGUIDPath(t *testing.T, path string) string {
@@ -209,14 +219,18 @@ func TestResolvePath(t *testing.T) {
 		{filepath.Join(dir, "lnk4"), filepath.Join(volumePathVHD2, "data.txt"), "symlink to volume with mount point"},
 	} {
 		t.Run(tc.description, func(t *testing.T) {
+			t.Logf("resolving: %s -> %s", tc.input, tc.expected)
+
 			actual, err := ResolvePath(tc.input)
 			if err != nil {
-				t.Fatalf("resolvePath should return no error, but %v", err)
+				t.Fatalf("ResolvePath should return no error, but: %v", err)
 			}
 			if actual != tc.expected {
 				t.Fatalf("expected %v but got %v", tc.expected, actual)
 			}
+
 			// Make sure EvalSymlinks works with the resolved path, as an extra safety measure.
+			t.Logf("filepath.EvalSymlinks(%s)", actual)
 			p, err := filepath.EvalSymlinks(actual)
 			if err != nil {
 				t.Fatalf("EvalSymlinks should return no error, but %v", err)