Update tests; run fuzzing (#294)

Run fuzzing tests in CI. Use race detector when running tests. Add missing `t.Helper()` calls. Update test helpers in `pkg/bindfilter` to use `RtlGetNtVersionNumbers` instead of reading registry, and skip tests if not running as admin. Signed-off-by: Hamza El-Saawy <[email protected]>
microsoft · Jul 21, 2023 · 19a9f65 · 19a9f65
1 parent 10d5703
commit 19a9f65
Show file tree

Hide file tree

Showing 11 changed files with 86 additions and 46 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -23,7 +23,7 @@ jobs:
       - name: Run golangci-lint
         uses: golangci/golangci-lint-action@v3
         with:
-          version: v1.52
+          version: v1.53
           args: >-
             --verbose
             --timeout=5m
@@ -73,6 +73,7 @@ jobs:
       - go-generate
     runs-on: ${{ matrix.os }}
     strategy:
+      fail-fast: false
       matrix:
         os: [windows-2019, windows-2022, ubuntu-latest]
     steps:
@@ -88,7 +89,15 @@ jobs:
         run: go install gotest.tools/gotestsum@${{ env.GOTESTSUM_VERSION }}
 
       - name: Test repo
-        run: gotestsum --format standard-verbose --debug -- -gcflags=all=-d=checkptr -v ./...
+        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"
 
   build:
     name: Build Repo

diff --git a/.golangci.yml b/.golangci.yml
@@ -20,6 +20,7 @@ linters:
     - gofmt # files are gofmt'ed
     - gosec # security
     - nilerr # returns nil even with non-nil error
+    - thelper #  test helpers without t.Helper()
     - unparam # unused function params
 
 issues:

diff --git a/backuptar/tar_test.go b/backuptar/tar_test.go
@@ -17,6 +17,8 @@ import (
 )
 
 func ensurePresent(t *testing.T, m map[string]string, keys ...string) {
+	t.Helper()
+
 	for _, k := range keys {
 		if _, ok := m[k]; !ok {
 			t.Error(k, "not present in tar header")
@@ -25,13 +27,17 @@ func ensurePresent(t *testing.T, m map[string]string, keys ...string) {
 }
 
 func setSparse(t *testing.T, f *os.File) {
+	t.Helper()
+
 	if err := windows.DeviceIoControl(windows.Handle(f.Fd()), windows.FSCTL_SET_SPARSE, nil, 0, nil, 0, nil, nil); err != nil {
 		t.Fatal(err)
 	}
 }
 
 // compareReaders validates that two readers contain the exact same data.
 func compareReaders(t *testing.T, rActual io.Reader, rExpected io.Reader) {
+	t.Helper()
+
 	const size = 8 * 1024
 	var bufExpected, bufActual [size]byte
 	var readCount int64
@@ -71,13 +77,17 @@ func TestRoundTrip(t *testing.T) {
 	//nolint:gosec // G306: Expect WriteFile permissions to be 0600 or less
 	for name, setup := range map[string]func(*testing.T) string{
 		"normalFile": func(t *testing.T) string {
+			t.Helper()
+
 			path := filepath.Join(t.TempDir(), "foo.txt")
 			if err := os.WriteFile(path, []byte("testing 1 2 3\n"), 0644); err != nil {
 				t.Fatal(err)
 			}
 			return path
 		},
 		"normalFileEmpty": func(t *testing.T) string {
+			t.Helper()
+
 			path := filepath.Join(t.TempDir(), "foo.txt")
 			f, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY, 0644)
 			if err != nil {
@@ -87,6 +97,8 @@ func TestRoundTrip(t *testing.T) {
 			return path
 		},
 		"sparseFileEmpty": func(t *testing.T) string {
+			t.Helper()
+
 			path := filepath.Join(t.TempDir(), "foo.txt")
 			f, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY, 0644)
 			if err != nil {
@@ -97,6 +109,8 @@ func TestRoundTrip(t *testing.T) {
 			return path
 		},
 		"sparseFileWithNoAllocatedRanges": func(t *testing.T) string {
+			t.Helper()
+
 			path := filepath.Join(t.TempDir(), "foo.txt")
 			f, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY, 0644)
 			if err != nil {
@@ -112,6 +126,8 @@ func TestRoundTrip(t *testing.T) {
 			return path
 		},
 		"sparseFileWithOneAllocatedRange": func(t *testing.T) string {
+			t.Helper()
+
 			path := filepath.Join(t.TempDir(), "foo.txt")
 			f, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY, 0644)
 			if err != nil {
@@ -125,6 +141,8 @@ func TestRoundTrip(t *testing.T) {
 			return path
 		},
 		"sparseFileWithMultipleAllocatedRanges": func(t *testing.T) string {
+			t.Helper()
+
 			path := filepath.Join(t.TempDir(), "foo.txt")
 			f, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY, 0644)
 			if err != nil {

diff --git a/file.go b/file.go
@@ -21,6 +21,8 @@ import (
 //sys setFileCompletionNotificationModes(h syscall.Handle, flags uint8) (err error) = SetFileCompletionNotificationModes
 //sys wsaGetOverlappedResult(h syscall.Handle, o *syscall.Overlapped, bytes *uint32, wait bool, flags *uint32) (err error) = ws2_32.WSAGetOverlappedResult
 
+//todo (go1.19): switch to [atomic.Bool]
+
 type atomicBool int32
 
 func (b *atomicBool) isSet() bool { return atomic.LoadInt32((*int32)(b)) != 0 }

diff --git a/fileinfo_test.go b/fileinfo_test.go
@@ -14,6 +14,8 @@ import (
 // so we check that the current.AllocationSize is >= expected.AllocationSize.
 // https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-fscc/5afa7f66-619c-48f3-955f-68c4ece704ae
 func checkFileStandardInfo(t *testing.T, current, expected *FileStandardInfo) {
+	t.Helper()
+
 	if current.AllocationSize < expected.AllocationSize {
 		t.Fatalf("FileStandardInfo unexpectedly had AllocationSize %d, expecting >=%d", current.AllocationSize, expected.AllocationSize)
 	}

diff --git a/hvsock_test.go b/hvsock_test.go
@@ -37,6 +37,7 @@ func serverListen(u testUtil) (l *HvsockListener, a *HvsockAddr) {
 			u.T.Logf("address collision %v", a)
 			continue
 		}
+		u.T.Logf("listening on %v", a)
 		break
 	}
 	u.Must(err, "could not listen")
@@ -579,9 +580,11 @@ type testUtil struct {
 	T testing.TB
 }
 
-func newUtil(t testing.TB) testUtil {
+func newUtil(tb testing.TB) testUtil {
+	tb.Helper()
+
 	return testUtil{
-		T: t,
+		T: tb,
 	}
 }
 

diff --git a/pipe_test.go b/pipe_test.go
@@ -223,6 +223,8 @@ func TestCloseAbortsListen(t *testing.T) {
 }
 
 func ensureEOFOnClose(t *testing.T, r io.Reader, w io.Closer) {
+	t.Helper()
+
 	b := make([]byte, 10)
 	w.Close()
 	n, err := r.Read(b)

diff --git a/pkg/bindfilter/bind_filter_test.go b/pkg/bindfilter/bind_filter_test.go
@@ -8,14 +8,15 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
-	"strconv"
 	"strings"
 	"testing"
 
-	"golang.org/x/sys/windows/registry"
+	"golang.org/x/sys/windows"
 )
 
 func TestApplyFileBinding(t *testing.T) {
+	requireElevated(t)
+
 	source := t.TempDir()
 	destination := t.TempDir()
 	fileName := "testFile.txt"
@@ -55,12 +56,15 @@ func TestApplyFileBinding(t *testing.T) {
 }
 
 func removeFileBinding(t *testing.T, mountpoint string) {
+	t.Helper()
 	if err := RemoveFileBinding(mountpoint); err != nil {
 		t.Logf("failed to remove file binding from %s: %q", mountpoint, err)
 	}
 }
 
 func TestApplyFileBindingReadOnly(t *testing.T) {
+	requireElevated(t)
+
 	source := t.TempDir()
 	destination := t.TempDir()
 	fileName := "testFile.txt"
@@ -108,19 +112,14 @@ func TestApplyFileBindingReadOnly(t *testing.T) {
 }
 
 func TestEnsureOnlyOneTargetCanBeMounted(t *testing.T) {
-	version, err := getWindowsBuildNumber()
-	if err != nil {
-		t.Fatalf("couldn't get version number: %s", err)
-	}
+	requireElevated(t)
+	requireBuild(t, RS5+1) // support added after RS5
 
-	if version <= 17763 {
-		t.Skip("not supported on RS5 or earlier")
-	}
 	source := t.TempDir()
 	secondarySource := t.TempDir()
 	destination := t.TempDir()
 
-	err = ApplyFileBinding(destination, source, false)
+	err := ApplyFileBinding(destination, source, false)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -159,14 +158,9 @@ func checkSourceIsMountedOnDestination(src, dst string) (bool, error) {
 }
 
 func TestGetBindMappings(t *testing.T) {
-	version, err := getWindowsBuildNumber()
-	if err != nil {
-		t.Fatalf("couldn't get version number: %s", err)
-	}
+	requireElevated(t)
+	requireBuild(t, RS5+1) // support added after RS5
 
-	if version <= 17763 {
-		t.Skip("not supported on RS5 or earlier")
-	}
 	// GetBindMappings will expand short paths like ADMINI~1 and PROGRA~1 to their
 	// full names. In order to properly match the names later, we expand them here.
 	srcShort := t.TempDir()
@@ -198,6 +192,8 @@ func TestGetBindMappings(t *testing.T) {
 }
 
 func TestRemoveFileBinding(t *testing.T) {
+	requireElevated(t)
+
 	srcShort := t.TempDir()
 	source, err := getFinalPath(srcShort)
 	if err != nil {
@@ -238,32 +234,9 @@ func TestRemoveFileBinding(t *testing.T) {
 	}
 }
 
-func getWindowsBuildNumber() (int, error) {
-	k, err := registry.OpenKey(registry.LOCAL_MACHINE, `SOFTWARE\Microsoft\Windows NT\CurrentVersion`, registry.QUERY_VALUE)
-	if err != nil {
-		return 0, fmt.Errorf("read CurrentVersion reg key: %w", err)
-	}
-	defer k.Close()
-	buildNumStr, _, err := k.GetStringValue("CurrentBuild")
-	if err != nil {
-		return 0, fmt.Errorf("read CurrentBuild reg value: %w", err)
-	}
-	buildNum, err := strconv.Atoi(buildNumStr)
-	if err != nil {
-		return 0, err
-	}
-	return buildNum, nil
-}
-
 func TestGetBindMappingsSymlinks(t *testing.T) {
-	version, err := getWindowsBuildNumber()
-	if err != nil {
-		t.Fatalf("couldn't get version number: %s", err)
-	}
-
-	if version <= 17763 {
-		t.Skip("not supported on RS5 or earlier")
-	}
+	requireElevated(t)
+	requireBuild(t, RS5+1) // support added after RS5
 
 	srcShort := t.TempDir()
 	sourceNested := filepath.Join(srcShort, "source")
@@ -307,3 +280,23 @@ func TestGetBindMappingsSymlinks(t *testing.T) {
 		t.Fatalf("expected to find %s mounted on %s, but could not", source, destination)
 	}
 }
+
+func requireElevated(tb testing.TB) {
+	tb.Helper()
+	if !windows.GetCurrentProcessToken().IsElevated() {
+		tb.Skip("requires elevated privileges")
+	}
+}
+
+const RS5 = 17763
+
+//todo: also check that `bindfltapi.dll` exists
+
+// require current build to be >= build
+func requireBuild(tb testing.TB, build uint32) {
+	tb.Helper()
+	_, _, b := windows.RtlGetNtVersionNumbers()
+	if b < build {
+		tb.Skipf("requires build %d+; current build is %d", build, b)
+	}
+}
diff --git a/pkg/etw/provider_test.go b/pkg/etw/provider_test.go
@@ -10,6 +10,8 @@ import (
 )
 
 func mustGUIDFromString(t *testing.T, s string) guid.GUID {
+	t.Helper()
+
 	g, err := guid.FromString(s)
 	if err != nil {
 		t.Fatal(err)

diff --git a/pkg/guid/guid_test.go b/pkg/guid/guid_test.go
@@ -7,6 +7,8 @@ import (
 )
 
 func mustNewV4(t *testing.T) GUID {
+	t.Helper()
+
 	g, err := NewV4()
 	if err != nil {
 		t.Fatal(err)
@@ -15,6 +17,8 @@ func mustNewV4(t *testing.T) GUID {
 }
 
 func mustNewV5(t *testing.T, namespace GUID, name []byte) GUID {
+	t.Helper()
+
 	g, err := NewV5(namespace, name)
 	if err != nil {
 		t.Fatal(err)
@@ -23,6 +27,8 @@ func mustNewV5(t *testing.T, namespace GUID, name []byte) GUID {
 }
 
 func mustFromString(t *testing.T, s string) GUID {
+	t.Helper()
+
 	g, err := FromString(s)
 	if err != nil {
 		t.Fatal(err)

diff --git a/pkg/security/grantvmgroupaccess_test.go b/pkg/security/grantvmgroupaccess_test.go
@@ -87,6 +87,8 @@ func TestGrantVmGroupAccess(t *testing.T) {
 }
 
 func verifyVMAccountDACLs(t *testing.T, name string, permissions []string) {
+	t.Helper()
+
 	cmd := exec.Command("icacls", name)
 	outb, err := cmd.CombinedOutput()
 	if err != nil {