From bc421d9108ade8ae2f76c8ef3e90cd4ee690e2c0 Mon Sep 17 00:00:00 2001 From: Hamza El-Saawy <84944216+helsaawy@users.noreply.github.com> Date: Mon, 18 Dec 2023 11:11:12 -0500 Subject: [PATCH] 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 * PR: unskip TestResolvePath Signed-off-by: Hamza El-Saawy --------- Signed-off-by: Hamza El-Saawy --- .github/workflows/ci.yml | 44 ++++++++++++++++++++++++++------------- hvsock.go | 10 ++++++++- hvsock_go118_test.go | 7 +++++++ pkg/fs/fs_windows_test.go | 4 +++- pkg/fs/resolve_test.go | 16 +++++++++++++- 5 files changed, 63 insertions(+), 18 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cb1d757..05d4bf1 100644 --- 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 index 1357ab4..c4fdd9d 100644 --- 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 index dd52914..942bfe4 100644 --- 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 index f5e4c6e..a4cdef4 100644 --- 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 index f9be69f..9d64e8c 100644 --- 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)