Skip to content

Commit

Permalink
Bug: Close hvsock handle on listen error; fix tests
Browse files Browse the repository at this point in the history
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 `filepath.EvalSymlinks` and
`github.com/Microsoft/go-winio/pkg/fs.GetFileSystemType`.

Update tests accordingly.

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 <hamzaelsaawy@microsoft.com>
  • Loading branch information
helsaawy committed Nov 21, 2023
1 parent 553a715 commit fe55675
Show file tree
Hide file tree
Showing 5 changed files with 68 additions and 14 deletions.
38 changes: 27 additions & 11 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
with:
go-version: ${{ env.GO_VERSION }}
cache: false

- name: Run golangci-lint
uses: golangci/golangci-lint-action@v3
Expand All @@ -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
with:
go-version: ${{ env.GO_VERSION }}
# don't really need to cache Go packages, since go generate doesn't download much.
# otherwise, the cache used in the `test` stage will be (basically) empty.
cache: false

- name: Run go generate
shell: pwsh
Expand Down Expand Up @@ -78,26 +86,32 @@ 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
with:
go-version: ${{ env.GO_VERSION }}

# avoid needing to download packages during test runs
- name: Pre-fill Module Cache
shell: powershell
run: |
go mod download
- 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
Expand All @@ -106,7 +120,9 @@ 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
Expand Down
10 changes: 9 additions & 1 deletion hvsock.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
7 changes: 7 additions & 0 deletions hvsock_go118_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 " +
Expand Down
4 changes: 3 additions & 1 deletion pkg/fs/fs_windows_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
23 changes: 22 additions & 1 deletion pkg/fs/resolve_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -209,14 +219,25 @@ 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.
if strings.HasPrefix(strings.ToLower(actual), `\\?\volume{`) {
// [filepath.EvalSymlinks] calls [filepath.VolumeName], which does not
// (as of go1.20.5, see: https://go-review.googlesource.com/c/go/+/540277) recognize
// `\\?\Volume{<GUID>}\` paths.
// Avoid calling [filepath.EvalSymlinks] on those paths, since it will error.
return
}
t.Logf("filepath.EvalSymlinks(%s)", actual)
p, err := filepath.EvalSymlinks(actual)
if err != nil {
t.Fatalf("EvalSymlinks should return no error, but %v", err)
Expand Down

0 comments on commit fe55675

Please sign in to comment.